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

Pre-filter TestTarget @union members #8368

Merged
merged 15 commits into from
Oct 6, 2019

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Sep 30, 2019

Allows running tests over globbed targets without erroring on
any non-test targets matched by the glob.

Provides a mechanism that any other console_rule can use for similar
effect if needed.

Allows running tests over globbed targets without erroring on
any non-test targets matched by the glob.

Provides a mechanism that any console_rule can use for similar
effects if needed.
def fast_test(console: Console, addresses: BuildFileAddresses,
build_config: BuildConfiguration) -> Test:
all_targets = yield Get(HydratedTargets, BuildFileAddresses, addresses)
filtered_targets = build_config.union_members(TestTarget, all_targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

I mildly prefer:

test_results = yield [
  Get(TestResult, HydratedTarget, tgt)
  for tgt in targets
  if build_config.is_union_member(TestTarget, tgt) # or tgt.adaptor
]

but not enough to argue about it if you don't :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, but each call to is_union_member would end up being a linear scan of the (probably very short) list of union members for the union base. Unless we change the data structures inside BuildConfiguration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, hm, looks like it's already an OrderedSet in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And just realized we need the list of filtered targets to zip with the test_results (my current code in this PR is broken there).

@benjyw
Copy link
Contributor Author

benjyw commented Sep 30, 2019

OK, pushed an update. PTAL.

@@ -25,33 +24,37 @@ class Test(Goal):


@console_rule
def fast_test(console: Console, addresses: BuildFileAddresses) -> Test:
test_results = yield [Get(TestResult, Address, address.to_address()) for address in addresses]
def fast_test(console: Console, targets: HydratedTargets, build_config: BuildConfiguration) -> Test:
Copy link
Member

Choose a reason for hiding this comment

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

Although this horse is already out of the barn, it would be great not to rely on the BuildConfiguration like this... there is way too much "stuff" in there, and I'd be worried about getting into a Context-has-all-of-the-things situation. The BuildConfiguration is currently injected for the purposes of options parsing (I thought here? but perhaps elsewhere), and I hope that we can shrink the requirements / surface area of those @rules over time.

"Mirror" might not be the best name, but it would be good to expose some smaller purposeful type, even if that means installing:

@rule
def mirror(build_config: BuildConfiguration) -> Mirror:
   ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. PTAL.

Limits the exposed surface area.
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

src/python/pants/build_graph/build_configuration.py Outdated Show resolved Hide resolved
src/python/pants/engine/rules.py Outdated Show resolved Hide resolved
src/python/pants/engine/rules.py Outdated Show resolved Hide resolved
from textwrap import dedent
from typing import Any, Callable, Type, cast
from typing import Any, Callable, Dict, Iterable, Type, cast
Copy link
Contributor

Choose a reason for hiding this comment

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

Bad change with Iterable. That's now overriding line 11.

def fast_test(console: Console, addresses: BuildFileAddresses) -> Test:
test_results = yield [Get(TestResult, Address, address.to_address()) for address in addresses]
def fast_test(console: Console,
targets: HydratedTargets,
Copy link
Member

Choose a reason for hiding this comment

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

So, there is a minor optimization here which might not matter, but which might be worth a TODO.

The effect of moving HydratedTargets into the signature like this is that fast_test will not start running until source globs have been expanded to hydrate all targets.

The previous formulation would only parse BUILD files, and would then request a test result per Address: the effect of that was that tests might be able to start running while source glob expansion was still ongoing. To preserve that behaviour, you could request Optional.of(TestResult) or some other newtype, and have coordinator_of_tests produce that and do the filtering.

I'd also feel a little bit better about that because of the TODO/NB in coordinator_of_tests: it would be nice to have the filtering logic closer to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! I'll take a swipe at this tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, did it now. I like it better this way! PTAL.

@@ -22,20 +26,25 @@ class Test(Goal):
name = 'test'


@dataclass
Copy link
Contributor

Choose a reason for hiding this comment

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

Need frozen=True

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@benjyw benjyw merged commit 30165bb into pantsbuild:master Oct 6, 2019
@benjyw benjyw deleted the test_target_selection branch October 6, 2019 00:56
@stuhood stuhood mentioned this pull request Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants