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.

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

Fix broken engine_error testutil decorator. #15818

Merged
merged 6 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/python/pants/backend/docker/goals/package_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from dataclasses import dataclass
from functools import partial
from itertools import chain
from typing import Iterator
from typing import Iterator, cast

# Re-exporting BuiltDockerImage here, as it has its natural home here, but has moved out to resolve
# a dependency cycle from docker_build_context.
Expand Down Expand Up @@ -170,7 +170,9 @@ def get_context_root(self, default_context_root: str) -> str:
if self.context_root.value is not None:
context_root = self.context_root.value
else:
context_root = default_context_root
context_root = cast(
str, self.context_root.compute_value(default_context_root, self.address)
)
if context_root.startswith("./"):
context_root = os.path.join(self.address.spec_path, context_root)
return os.path.normpath(context_root)
Expand Down
26 changes: 9 additions & 17 deletions src/python/pants/backend/docker/goals/package_image_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,7 @@
from pants.option.global_options import GlobalOptions, ProcessCleanupOption
from pants.testutil.option_util import create_subsystem
from pants.testutil.pytest_util import assert_logged, no_exception
from pants.testutil.rule_runner import (
MockGet,
QueryRule,
RuleRunner,
engine_error,
run_rule_with_mocks,
)
from pants.testutil.rule_runner import MockGet, QueryRule, RuleRunner, run_rule_with_mocks
from pants.util.frozendict import FrozenDict


Expand Down Expand Up @@ -896,20 +890,20 @@ def test_invalid_build_target_stage(rule_runner: RuleRunner) -> None:
(
"/",
None,
engine_error(
pytest.raises(
InvalidFieldException,
contains=("Use '' for a path relative to the build root, or './' for"),
match=r"Use '' for a path relative to the build root, or '\./' for",
),
),
(
"/src",
None,
engine_error(
pytest.raises(
InvalidFieldException,
contains=(
"The `context_root` field in target src/docker:image must be a relative path, but was "
"'/src'. Use 'src' for a path relative to the build root, or './src' for a path "
"relative to the BUILD file (i.e. 'src/docker/src')."
match=(
r"The `context_root` field in target src/docker:image must be a relative path, "
r"but was '/src'\. Use 'src' for a path relative to the build root, or '\./src' "
r"for a path relative to the BUILD file \(i\.e\. 'src/docker/src'\)\."
),
),
),
Expand All @@ -931,7 +925,6 @@ def test_get_context_root(
raises = cast("ContextManager", no_exception())
else:
raises = expected_context_root
expected_context_root = ""

with raises:
docker_options = create_subsystem(
Expand All @@ -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.

assert actual_context_root == expected_context_root


@pytest.mark.parametrize(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ def test_invalid_go_sum(rule_runner: RuleRunner) -> None:
rule_runner.request(AllThirdPartyPackages, [AllThirdPartyPackagesRequest(digest, "go.mod")])


@pytest.mark.skip("broken test")
def test_missing_go_sum(rule_runner: RuleRunner) -> None:
digest = set_up_go_mod(
rule_runner,
Expand All @@ -284,8 +285,10 @@ def test_missing_go_sum(rule_runner: RuleRunner) -> None:
)
with engine_error(contains="github.com/google/uuid@v1.3.0: missing go.sum entry"):
rule_runner.request(AllThirdPartyPackages, [AllThirdPartyPackagesRequest(digest, "go.mod")])
assert False, "should not get here!"


@pytest.mark.skip("broken test")
def test_stale_go_mod(rule_runner: RuleRunner) -> None:
digest = set_up_go_mod(
rule_runner,
Expand All @@ -309,6 +312,7 @@ def test_stale_go_mod(rule_runner: RuleRunner) -> None:
)
with engine_error(ProcessExecutionFailure, contains="updates to go.mod needed"):
rule_runner.request(AllThirdPartyPackages, [AllThirdPartyPackagesRequest(digest, "go.mod")])
assert False, "should not get here!"


def test_pkg_missing(rule_runner: RuleRunner) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def rule_runner() -> RuleRunner:
)


@pytest.mark.skip("broken test")
def test_choose_compatible_resolve(rule_runner: RuleRunner) -> None:
def create_target_files(
directory: str, *, req_resolve: str, source_resolve: str, test_resolve: str
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ def test_resolve_with_inexact_coord(rule_runner: RuleRunner) -> None:
)


@pytest.mark.skip("broken test")
@maybe_skip_jdk_test
def test_resolve_conflicting(rule_runner: RuleRunner) -> None:
with engine_error(
Expand Down
5 changes: 5 additions & 0 deletions src/python/pants/testutil/rule_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ def engine_error(
f"expected: {contains}\n\n"
f"exception: {underlying}"
)
else:
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..



# -----------------------------------------------------------------------------------------------
Expand Down