Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix broken engine_error testutil decorator. #15818

Merged
merged 6 commits into from
Jun 15, 2022
Merged

Fix broken engine_error testutil decorator. #15818

merged 6 commits into from
Jun 15, 2022

Conversation

kaos
Copy link
Member

@kaos kaos commented Jun 14, 2022

The decorator did not throw an assertion error in case there was no ExecutionError as expected.

Fixing this surfaced a few tests that does not throw when expected. I've fixed the tests in the Docker backend while adding skip marks for the others. We can either merge them as skipped and address them separately, or fix them directly in this PR..

kaos added 2 commits June 14, 2022 13:35
@kaos
engine_error decorator did not catch missing exceptions.
f3c3296 
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@kaos
Fix broken engine_error testutil decorator.
bbe1182 
[ci skip-rust]

[ci skip-build-wheels]
@kaos kaos added the category:internal CI, fixes for not-yet-released features, etc. label Jun 14, 2022
@kaos kaos requested a review from Eric-Arellano June 14, 2022 12:32
kaos
kaos commented Jun 14, 2022
View reviewed changes
src/python/pants/testutil/rule_runner.py
raise AssertionError(
"DID NOT RAISE ExecutionError with underlying exception type "
f"{expected_underlying_exception}."
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eric-Arellano I completely missed this in the review of #13108, I guess I got lost during one of the major refactorings in that PR..

kaos
kaos commented Jun 14, 2022
View reviewed changes
src/python/pants/backend/docker/goals/package_image_test.py
@@ -942,8 +935,7 @@ def test_get_context_root(
tgt = DockerImageTarget({"context_root": context_root}, address)
fs = DockerFieldSet.create(tgt)
actual_context_root = fs.get_context_root(docker_options.default_context_root)
if expected_context_root:
assert actual_context_root == expected_context_root
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should've caught this issue when I wrote this... alas I did not :P

I did catch this issue now, however, when using the engine_error decorator on the visibility tests.

Eric-Arellano
Eric-Arellano approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Thanks

@Eric-Arellano
Copy link
Contributor

Can you please open an issue to track the failing tests and then use TODO(#...) in the skip decorators? That way we have a way to track which tests need to be fixed.

@kaos kaos mentioned this pull request Jun 14, 2022
Open
kaos added 2 commits June 14, 2022 19:00
@kaos
Add pointer to pantsbuild#15824 for broken tests.
23866e2 
[ci skip-rust]

[ci skip-build-wheels]
@kaos
Merge remote-tracking branch 'upstream/main' into engine_error
9274773 
[ci skip-rust]

[ci skip-build-wheels]
kaos
kaos commented Jun 14, 2022
View reviewed changes
src/python/pants/backend/python/util_rules/pex_from_targets_test.py
@@ -69,6 +69,7 @@ def rule_runner() -> RuleRunner:
)


@pytest.mark.skip(reason="TODO(#15824)")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the correct way to skip tests?

I got this:

Forbidden skipped test - Skipped: TODO(#15824)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @pytest.mark.no_error_if_skipped

423:    @pytest.mark.skip(reason="flaky: https://github.com/pantsbuild/pants/issues/8193")
424-    @pytest.mark.no_error_if_skipped
425-    def test_pantsd_memory_usage(self):

cc @thejcannon if the error message could be improved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comes from

pants/conftest.py

Line 21 in 95f28ba

rep.longrepr = f"Forbidden skipped test - {r.message}"
so we have the keys

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there it is. That explains why I didn't find anything on --noskip in the pytest docs 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how customizable pytest is.

@kaos kaos merged commit 1c65b9c into pantsbuild:main Jun 15, 2022
@kaos kaos deleted the engine_error branch June 15, 2022 21:12
illicitonion added a commit to illicitonion/pants that referenced this pull request Jun 17, 2022
@illicitonion
Bump to 2.13.0a0
fe5e790 
Internal changes:

* Add `@alonsodomin` to maintainers ([pantsbuild#15869](pantsbuild#15869))

* Scala parser improvements ([pantsbuild#15839](pantsbuild#15839))

* Refactor valid field aliases calculation for targets into a classmethod for easier re-use. ([pantsbuild#15861](pantsbuild#15861))

* Update Media page with Pycon ([pantsbuild#15852](pantsbuild#15852))

* Use Readme.com's new Markdown syntax ([pantsbuild#15845](pantsbuild#15845))

* [internal] better align test JVM lockfile support with pytest concepts ([pantsbuild#15804](pantsbuild#15804))

* Fix broken `engine_error` testutil decorator. ([pantsbuild#15818](pantsbuild#15818))

* Add `description_of_origin` for Docker address parsing ([pantsbuild#15814](pantsbuild#15814))

* Use `MultiGet` in Go rule ([pantsbuild#15838](pantsbuild#15838))

* Switch from warn to warning ([pantsbuild#15837](pantsbuild#15837))

* Bump clap from 3.1.18 to 3.2.4 in /src/rust/engine ([pantsbuild#15834](pantsbuild#15834))

* Bump reqwest from 0.11.10 to 0.11.11 in /src/rust/engine ([pantsbuild#15831](pantsbuild#15831))

* Bump strum from 0.24.0 to 0.24.1 in /src/rust/engine ([pantsbuild#15833](pantsbuild#15833))

* Bump strum_macros from 0.24.0 to 0.24.1 in /src/rust/engine ([pantsbuild#15835](pantsbuild#15835))

* Remove flake8-pantsbuild plugin ([pantsbuild#15830](pantsbuild#15830))

* go: support third-party package embeds ([pantsbuild#15827](pantsbuild#15827))

* Move `gshuflin` to Maintainers Emeritus ([pantsbuild#15828](pantsbuild#15828))

* Adds current contents of readme.com docs to repo and adds notes on how to sync. ([pantsbuild#15810](pantsbuild#15810))

* Remove unnecessary `WrappedNode` method indirection ([pantsbuild#15813](pantsbuild#15813))

* Handle anchors in doc_url() correctly. ([pantsbuild#15812](pantsbuild#15812))

* Move the macOS ARM64 build job into the existing test workflows. ([pantsbuild#15805](pantsbuild#15805))

* Add Lablup to users list. ([pantsbuild#15807](pantsbuild#15807))

* Add `description_of_origin` for several places finding Addresses ([pantsbuild#15797](pantsbuild#15797))

* [internal] Register `platform_specific_behavior` marker with Pytest ([pantsbuild#12679](pantsbuild#12679))

* Bump http from 0.2.7 to 0.2.8 in /src/rust/engine ([pantsbuild#15780](pantsbuild#15780))

* Update the `notify` crate. ([pantsbuild#15783](pantsbuild#15783))

* Bump tokio-stream from 0.1.8 to 0.1.9 in /src/rust/engine ([pantsbuild#15777](pantsbuild#15777))

* Bump indexmap from 1.8.1 to 1.8.2 in /src/rust/engine ([pantsbuild#15778](pantsbuild#15778))

* Bump uuid from 1.0.0 to 1.1.1 in /src/rust/engine ([pantsbuild#15779](pantsbuild#15779))

* Bump hyper from 0.14.18 to 0.14.19 in /src/rust/engine ([pantsbuild#15781](pantsbuild#15781))

* Update `crossbeam-utils` to `0.8.8`. ([pantsbuild#15782](pantsbuild#15782))
@illicitonion illicitonion mentioned this pull request Jun 17, 2022
Merged
illicitonion added a commit that referenced this pull request Jun 17, 2022
@illicitonion
Bump to 2.13.0a0 (#15871)
d1dd32a 
Internal changes:

* Add `@alonsodomin` to maintainers ([#15869](#15869))

* Scala parser improvements ([#15839](#15839))

* Refactor valid field aliases calculation for targets into a classmethod for easier re-use. ([#15861](#15861))

* Update Media page with Pycon ([#15852](#15852))

* Use Readme.com's new Markdown syntax ([#15845](#15845))

* [internal] better align test JVM lockfile support with pytest concepts ([#15804](#15804))

* Fix broken `engine_error` testutil decorator. ([#15818](#15818))

* Add `description_of_origin` for Docker address parsing ([#15814](#15814))

* Use `MultiGet` in Go rule ([#15838](#15838))

* Switch from warn to warning ([#15837](#15837))

* Bump clap from 3.1.18 to 3.2.4 in /src/rust/engine ([#15834](#15834))

* Bump reqwest from 0.11.10 to 0.11.11 in /src/rust/engine ([#15831](#15831))

* Bump strum from 0.24.0 to 0.24.1 in /src/rust/engine ([#15833](#15833))

* Bump strum_macros from 0.24.0 to 0.24.1 in /src/rust/engine ([#15835](#15835))

* Remove flake8-pantsbuild plugin ([#15830](#15830))

* go: support third-party package embeds ([#15827](#15827))

* Move `gshuflin` to Maintainers Emeritus ([#15828](#15828))

* Adds current contents of readme.com docs to repo and adds notes on how to sync. ([#15810](#15810))

* Remove unnecessary `WrappedNode` method indirection ([#15813](#15813))

* Handle anchors in doc_url() correctly. ([#15812](#15812))

* Move the macOS ARM64 build job into the existing test workflows. ([#15805](#15805))

* Add Lablup to users list. ([#15807](#15807))

* Add `description_of_origin` for several places finding Addresses ([#15797](#15797))

* [internal] Register `platform_specific_behavior` marker with Pytest ([#12679](#12679))

* Bump http from 0.2.7 to 0.2.8 in /src/rust/engine ([#15780](#15780))

* Update the `notify` crate. ([#15783](#15783))

* Bump tokio-stream from 0.1.8 to 0.1.9 in /src/rust/engine ([#15777](#15777))

* Bump indexmap from 1.8.1 to 1.8.2 in /src/rust/engine ([#15778](#15778))

* Bump uuid from 1.0.0 to 1.1.1 in /src/rust/engine ([#15779](#15779))

* Bump hyper from 0.14.18 to 0.14.19 in /src/rust/engine ([#15781](#15781))

* Update `crossbeam-utils` to `0.8.8`. ([#15782](#15782))
@wisechengyi wisechengyi mentioned this pull request Jun 25, 2022
Merged
wisechengyi added a commit that referenced this pull request Jun 26, 2022
@wisechengyi
Prepare 2.14.0.dev0 (#15944)
9355d6e 
### Internal (put these in a PR comment for review, not the release notes)

* Bump uuid from 1.1.1 to 1.1.2 in /src/rust/engine ([#15832](#15832))

* Tests for Docker from arg not a upstream target ([#15874](#15874))

* Fix outstanding todo for moved fields defaults support. ([#15923](#15923))

* Move generic debug adapter settings to dedicated subsystem ([#15928](#15928))

* Prepare `2.12.0rc3`. ([#15913](#15913))

* Prepare `2.11.1rc3`. ([#15912](#15912))

* Upgrade default mypy to 0.961 and default pylint to v2.13.9 ([#15886](#15886))

* Bump clap from 3.2.4 to 3.2.6 in /src/rust/engine ([#15909](#15909))

* Bump indexmap from 1.8.2 to 1.9.1 in /src/rust/engine ([#15894](#15894))

* Bump time from 0.3.9 to 0.3.11 in /src/rust/engine ([#15895](#15895))

* Bump tower-service from 0.3.1 to 0.3.2 in /src/rust/engine ([#15896](#15896))

* Bump crossbeam-channel from 0.5.4 to 0.5.5 in /src/rust/engine ([#15893](#15893))

* [internal] remove TestCoursierWrapper ([#15902](#15902))

* Silence `[scala-infer].force_add_siblings_as_dependencies` deprecation. ([#15898](#15898))

* Add unit tests to GraphQL queries ([#15792](#15792))

* Release script fixups ([#15875](#15875))

* [internal] jvm: port remaining tests to use test lockfiles ([#15870](#15870))

* Bump to 2.13.0a0 ([#15871](#15871))

* Add `@alonsodomin` to maintainers ([#15869](#15869))

* Scala parser improvements ([#15839](#15839))

* Refactor valid field aliases calculation for targets into a classmethod for easier re-use. ([#15861](#15861))

* Update Media page with Pycon ([#15852](#15852))

* Use Readme.com's new Markdown syntax ([#15845](#15845))

* [internal] better align test JVM lockfile support with pytest concepts ([#15804](#15804))

* Fix broken `engine_error` testutil decorator. ([#15818](#15818))

* Add `description_of_origin` for Docker address parsing ([#15814](#15814))

* Use `MultiGet` in Go rule ([#15838](#15838))

* Switch from warn to warning ([#15837](#15837))

* Bump clap from 3.1.18 to 3.2.4 in /src/rust/engine ([#15834](#15834))

* Bump reqwest from 0.11.10 to 0.11.11 in /src/rust/engine ([#15831](#15831))

* Bump strum from 0.24.0 to 0.24.1 in /src/rust/engine ([#15833](#15833))

* Bump strum_macros from 0.24.0 to 0.24.1 in /src/rust/engine ([#15835](#15835))

* Remove flake8-pantsbuild plugin ([#15830](#15830))

* go: support third-party package embeds ([#15827](#15827))

* Move `gshuflin` to Maintainers Emeritus ([#15828](#15828))

* Adds current contents of readme.com docs to repo and adds notes on how to sync. ([#15810](#15810))

* Remove unnecessary `WrappedNode` method indirection ([#15813](#15813))

* Handle anchors in doc_url() correctly. ([#15812](#15812))

* Move the macOS ARM64 build job into the existing test workflows. ([#15805](#15805))

* Add Lablup to users list. ([#15807](#15807))

* Add `description_of_origin` for several places finding Addresses ([#15797](#15797))

* [internal] Register `platform_specific_behavior` marker with Pytest ([#12679](#12679))

* Bump http from 0.2.7 to 0.2.8 in /src/rust/engine ([#15780](#15780))

* Update the `notify` crate. ([#15783](#15783))

* Bump tokio-stream from 0.1.8 to 0.1.9 in /src/rust/engine ([#15777](#15777))

* Bump indexmap from 1.8.1 to 1.8.2 in /src/rust/engine ([#15778](#15778))

* Bump uuid from 1.0.0 to 1.1.1 in /src/rust/engine ([#15779](#15779))

* Bump hyper from 0.14.18 to 0.14.19 in /src/rust/engine ([#15781](#15781))

* Update `crossbeam-utils` to `0.8.8`. ([#15782](#15782))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thejcannon thejcannon thejcannon left review comments

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaos @Eric-Arellano @thejcannon