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

Pre-filter TestTarget @union members #8368

Merged
merged 15 commits into from
Oct 6, 2019
Merged

Pre-filter TestTarget @union members #8368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8e33ec1
Pre-filter TestTarget @union members.
benjyw Sep 30, 2019
15c4c3a
Address code review comments.
benjyw Sep 30, 2019
6720729
Merge branch 'master' into test_target_selection
benjyw Sep 30, 2019
770a36c
Don't use BuildConfiguration directly.
benjyw Oct 1, 2019
b194f32
Address code review comments.
benjyw Oct 1, 2019
8e100cd
Merge branch 'master' into test_target_selection
benjyw Oct 2, 2019
5b5f6de
Merge branch 'master' into test_target_selection
benjyw Oct 4, 2019
ca465ac
Lint
benjyw Oct 4, 2019
c9fe6e9
Merge branch 'master' into test_target_selection
benjyw Oct 5, 2019
7ec2092
Fix bad import collision.
benjyw Oct 5, 2019
85d54cc
Address code review feedback.
benjyw Oct 5, 2019
8f13377
Lint fix.
benjyw Oct 5, 2019
a6507f1
Fix tests
benjyw Oct 5, 2019
8b181b8
Dataclass frozen fix.
benjyw Oct 5, 2019
07281c0
Lint.
benjyw Oct 5, 2019
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: 6 additions & 0 deletions src/python/pants/build_graph/build_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,12 @@ def union_rules(self):
"""
return self._union_rules

def union_members(self, union_type, targets):
if union_type not in self._union_rules:
raise TypeError(f'Not a registered union type: {union_type}')
test_target_adaptors = set(self._union_rules[union_type])
return [tgt for tgt in targets if type(tgt.adaptor) in test_target_adaptors]

@memoized_method
def _get_addressable_factory(self, target_type, alias):
return TargetAddressable.factory(target_type=target_type, alias=alias)
Expand Down
12 changes: 7 additions & 5 deletions src/python/pants/rules/core/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,15 @@
import logging

from pants.base.exiter import PANTS_FAILED_EXIT_CODE, PANTS_SUCCEEDED_EXIT_CODE
from pants.build_graph.address import Address
from pants.build_graph.build_configuration import BuildConfiguration
from pants.engine.addressable import BuildFileAddresses
from pants.engine.console import Console
from pants.engine.goal import Goal
from pants.engine.legacy.graph import HydratedTarget
from pants.engine.legacy.graph import HydratedTarget, HydratedTargets
from pants.engine.rules import console_rule, rule
from pants.engine.selectors import Get
from pants.rules.core.core_test_model import Status, TestResult, TestTarget


# TODO(#6004): use proper Logging singleton, rather than static logger.
logger = logging.getLogger(__name__)

Expand All @@ -25,8 +24,11 @@ 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, 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).

test_results = yield [Get(TestResult, HydratedTarget, tgt) for tgt in filtered_targets]
did_any_fail = False
for address, test_result in zip(addresses, test_results):
if test_result.status == Status.FAILURE:
Expand Down