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 defaulting of parameters in explicitly specified deps on parametrized targets for AsyncFieldMixin #16176

Merged
merged 2 commits into from Jul 14, 2022
Merged

Fix defaulting of parameters in explicitly specified deps on parametrized targets for AsyncFieldMixin #16176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1e5a96
Allow for asserting only on dependency edges.
stuhood Jul 13, 2022
981bf2c
Test with AsyncFieldMixin, and fix field value comparison.
stuhood Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
128 changes: 63 additions & 65 deletions src/python/pants/engine/internals/graph_test.py
View file
Open in desktop
Expand Up @@ -85,7 +85,7 @@ class SpecialCasedDeps2(SpecialCasedDependencies):
alias = "special_cased_deps2"


class ResolveField(StringField):
class ResolveField(StringField, AsyncFieldMixin):
alias = "resolve"


Expand Down Expand Up @@ -839,9 +839,9 @@ def assert_generated(
address: Address,
build_content: str,
files: list[str],
expected: set[Target],
expected_targets: set[Target] | None = None,
*,
dependencies: dict[str, set[str]] | None = None,
expected_dependencies: dict[str, set[str]] | None = None,
) -> None:
rule_runner.write_files(
{
Expand All @@ -853,11 +853,14 @@ def assert_generated(
_TargetParametrizations,
[_TargetParametrizationsRequest(address, description_of_origin="tests")],
)
assert expected == {
t for parametrization in parametrizations for t in parametrization.parametrization.values()
}
if expected_targets is not None:
assert expected_targets == {
t
for parametrization in parametrizations
for t in parametrization.parametrization.values()
}

if dependencies is not None:
if expected_dependencies is not None:
# TODO: Adjust the `TransitiveTargets` API to expose the complete mapping.
# see https://github.com/pantsbuild/pants/issues/11270
specs = SpecsParser(rule_runner.build_root).parse_specs(
Expand All @@ -875,7 +878,7 @@ def assert_generated(
)
assert {
k.spec: {a.spec for a in v} for k, v in dependency_mapping.mapping.items()
} == dependencies
} == expected_dependencies


def test_generate_multiple(generated_targets_rule_runner: RuleRunner) -> None:
Expand Down Expand Up @@ -964,7 +967,7 @@ def test_parametrize(generated_targets_rule_runner: RuleRunner) -> None:
residence_dir="demo",
),
},
dependencies={
expected_dependencies={
"demo@tags=t1": {"demo/f1.ext@tags=t1"},
"demo@tags=t2": {"demo/f1.ext@tags=t2"},
"demo/f1.ext@tags=t1": set(),
Expand Down Expand Up @@ -1035,7 +1038,7 @@ def test_parametrize_overrides(generated_targets_rule_runner: RuleRunner) -> Non
residence_dir="demo",
),
},
dependencies={
expected_dependencies={
"demo:demo": {
"demo/f1.ext@resolve=a",
"demo/f1.ext@resolve=b",
Expand Down Expand Up @@ -1066,14 +1069,14 @@ def test_parametrize_atom(generated_targets_rule_runner: RuleRunner) -> None:
residence_dir="demo",
),
},
dependencies={
expected_dependencies={
"demo@resolve=a": set(),
"demo@resolve=b": set(),
},
)


def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) -> None:
def test_parametrize_partial_atom_to_atom(generated_targets_rule_runner: RuleRunner) -> None:
assert_generated(
generated_targets_rule_runner,
Address("demo", target_name="t2"),
Expand All @@ -1093,27 +1096,7 @@ def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) ->
"""
),
["f1.ext", "f2.ext"],
{
MockGeneratedTarget(
{
SingleSourceField.alias: "f2.ext",
ResolveField.alias: "a",
Dependencies.alias: [":t1"],
},
Address("demo", target_name="t2", parameters={"resolve": "a"}),
residence_dir="demo",
),
MockGeneratedTarget(
{
SingleSourceField.alias: "f2.ext",
ResolveField.alias: "b",
Dependencies.alias: [":t1"],
},
Address("demo", target_name="t2", parameters={"resolve": "b"}),
residence_dir="demo",
),
},
dependencies={
expected_dependencies={
"demo:t1@resolve=a": set(),
"demo:t1@resolve=b": set(),
"demo:t2@resolve=a": {"demo:t1@resolve=a"},
Expand All @@ -1122,7 +1105,9 @@ def test_parametrize_partial_atom(generated_targets_rule_runner: RuleRunner) ->
)


def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner) -> None:
def test_parametrize_partial_generator_to_generator(
generated_targets_rule_runner: RuleRunner,
) -> None:
assert_generated(
generated_targets_rule_runner,
Address("demo", target_name="t2"),
Expand All @@ -1142,37 +1127,7 @@ def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner
"""
),
["f1.ext", "f2.ext"],
{
MockGeneratedTarget(
{
SingleSourceField.alias: "f2.ext",
ResolveField.alias: "a",
Dependencies.alias: [":t1"],
},
Address(
"demo",
relative_file_path="f2.ext",
target_name="t2",
parameters={"resolve": "a"},
),
residence_dir="demo",
),
MockGeneratedTarget(
{
SingleSourceField.alias: "f2.ext",
ResolveField.alias: "b",
Dependencies.alias: [":t1"],
},
Address(
"demo",
relative_file_path="f2.ext",
target_name="t2",
parameters={"resolve": "b"},
),
residence_dir="demo",
),
},
dependencies={
expected_dependencies={
"demo/f1.ext:t1@resolve=a": set(),
"demo/f1.ext:t1@resolve=b": set(),
"demo/f2.ext:t2@resolve=a": {"demo:t1@resolve=a"},
Expand All @@ -1193,6 +1148,49 @@ def test_parametrize_partial_generator(generated_targets_rule_runner: RuleRunner
)


def test_parametrize_partial_generator_to_generated(
generated_targets_rule_runner: RuleRunner,
) -> None:
assert_generated(
generated_targets_rule_runner,
Address("demo", target_name="t2"),
dedent(
"""\
generator(
name='t1',
resolve=parametrize('a', 'b'),
sources=['f1.ext'],
)
generator(
name='t2',
resolve=parametrize('a', 'b'),
sources=['f2.ext'],
dependencies=['./f1.ext:t1'],
)
"""
),
["f1.ext", "f2.ext"],
expected_dependencies={
"demo/f1.ext:t1@resolve=a": set(),
"demo/f1.ext:t1@resolve=b": set(),
"demo/f2.ext:t2@resolve=a": {"demo/f1.ext:t1@resolve=a"},
"demo/f2.ext:t2@resolve=b": {"demo/f1.ext:t1@resolve=b"},
"demo:t1@resolve=a": {
"demo/f1.ext:t1@resolve=a",
},
"demo:t1@resolve=b": {
"demo/f1.ext:t1@resolve=b",
},
"demo:t2@resolve=a": {
"demo/f2.ext:t2@resolve=a",
},
"demo:t2@resolve=b": {
"demo/f2.ext:t2@resolve=b",
},
},
)


# -----------------------------------------------------------------------------------------------
# Test `SourcesField`. Also see `engine/target_test.py`.
# -----------------------------------------------------------------------------------------------
Expand Down
11 changes: 8 additions & 3 deletions src/python/pants/engine/internals/parametrize.py
View file
Open in desktop
Expand Up @@ -232,14 +232,19 @@ def get_subset(self, address: Address, consumer: Target) -> Target:
return instance

def remaining_fields_match(candidate: Target) -> bool:
"""Returns true if all Fields absent from the candidate's Address match the consumer."""
"""Returns true if all Fields absent from the candidate's Address match the consumer.

TODO: This does not account for the computed default values of Fields:
see https://github.com/pantsbuild/pants/issues/16175
"""

unspecified_param_field_names = {
key for key in candidate.address.parameters.keys() if key not in address.parameters
}

return all(
consumer.has_field(field_type) and consumer[field_type] == field_value
for field_type, field_value in candidate.field_values.items()
consumer.has_field(field_type) and consumer[field_type].value == field.value
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is the actual fix, and the addition of AsyncFieldMixin to the mock ResolveField in the test is the validation.

for field_type, field in candidate.field_values.items()
if field_type.alias in unspecified_param_field_names
)

Expand Down