Skip to content

Commit

Permalink
Fix defaulting of parameters in explicitly specified deps on `paramet…
Browse files Browse the repository at this point in the history 
…rize`d targets for AsyncFieldMixin (Cherry-pick of #16176) (#16180)

Although the tests for #14519 were reasonably good, they tested with `StringField`, rather than additionally with `AsyncFieldMixin`. The latter changes the definition of equality for the `Field` to include its address, meaning that comparing the `Field` instance itself will never match for `Field`s from different targets. That made #14519... not particularly useful in production.

This change fixes `Field` value comparisons to use `field.value`, references the newly opened #16175 (which is out of scope for now, given that it will likely be impacted by `__defaults__` and would be much harder to cherry-pick), and adds an additional test.

Fixes #14519.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
@stuhood
stuhood committed Jul 14, 2022
1 parent 213ac73 commit 1bc7a28
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 68 deletions.
128 changes: 63 additions & 65 deletions src/python/pants/engine/internals/graph_test.py
View file
Expand Up @@ -81,7 +81,7 @@ class SpecialCasedDeps2(SpecialCasedDependencies):
alias = "special_cased_deps2"


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


Expand Down Expand Up @@ -831,9 +831,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 @@ -845,11 +845,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 @@ -867,7 +870,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 @@ -956,7 +959,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 @@ -1027,7 +1030,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 @@ -1058,14 +1061,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 @@ -1085,27 +1088,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 @@ -1114,7 +1097,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 @@ -1134,37 +1119,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 @@ -1185,6 +1140,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
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
for field_type, field in candidate.field_values.items()
if field_type.alias in unspecified_param_field_names
)

Expand Down

0 comments on commit 1bc7a28

Please sign in to comment.