Error out if the user explicitly tries to test a non-test tgt. #8402

benjyw · 2019-10-07T02:37:24Z

Previously we would apply the union filter to all targets.
So ./pants test path/to:non-test-target would noop.

Now we only apply the filter to targets that originated in a glob spec,
such as path/to/targets::. If the user tries to run tests on
a non-test target via a single address spec then we don't apply
the filter, and instead fail on attempting to cast the non-test
target to the TestTarget union (which yields a sensible error message).

This gives us the best of both worlds: We can give a sensible error,
instead of nooping, when the user specifies a single target, but
treat globs in the expected way.

This required introducing the concept of the "provenance" of a
BuildFileAddress. That is, the Spec it originated from. The term
"source" is overloaded in this context, so we use "provenance"
instead.

Eric-Arellano · 2019-10-07T02:45:22Z

Awesome! I'll more closely review tomorrow.

Question in the meantime: do we care about parity with V1 behavior? V1 will still no-op, whereas V2 will have a nice error. I think this is fine—and I'm not sure where we'd even start to change this in V1—but worth discussing.

src/python/pants/build_graph/address.py

src/python/pants/engine/build_files.py

stuhood · 2019-10-07T03:02:21Z

src/python/pants/rules/core/test.py

  # TODO(#6004): when streaming to live TTY, rely on V2 UI for this information. When not a
  # live TTY, periodically dump heavy hitters to stderr. See
  # https://github.com/pantsbuild/pants/issues/6004#issuecomment-492699898.
-  if union_membership.is_member(TestTarget, target.adaptor):
+  if (provenance_map.is_single_address(target.address) or


Neat!

I can't find the tickets that discuss this, but: one approach that would be parallel to how we implement "glob" validation would be to require that each Spec has at least one match. So even a glob like ./pants test ${dir}:: would fail if there were no tests in the directory.

...well, other than #221 and #3651, I suppose.

Hmm, I'm not convinced that specs should be required to match at least one "relevant" target. I'm not convinced of the converse either. :)

benjyw · 2019-10-07T03:21:22Z

Awesome! I'll more closely review tomorrow.

Question in the meantime: do we care about parity with V1 behavior? V1 will still no-op, whereas V2 will have a nice error. I think this is fine—and I'm not sure where we'd even start to change this in V1—but worth discussing.

Yeah, I don't think parity is expected or required...

Previously we would apply the union filter to all targets. So `./pants test path/to:non-test-target` would noop. Now we only apply the filter to targets that originated in a glob spec, such as path/to/targets::. If the user tries to run tests on a non-test target via a single address spec then we don't apply the filter, and instead fail on attempting to cast the non-test target to the TestTarget union (which yields a sensible error message). This gives us the best of both worlds: We can give a sensible error, instead of nooping, when the user specifies a single target, but treat globs in the expected way. This required introducing the concept of the "provenance" of a BuildFileAddress. That is, the Spec it originated from. The term "source" is overloaded in this context, so we use "provenance" instead.

benjyw · 2019-10-08T00:13:20Z

Ping, now that this is green :)

stuhood · 2019-10-08T02:33:57Z

src/python/pants/engine/build_files.py

-  key, value = entry
-  return key
+    key, value = entry
+    return key




Unfortunately, this file seems to have been formatted with black =(

Skipping review of this because the diff is unclear to me.

Hmm I'm not sure what happened here, I thought I'd reverted my attempts to apply black. Will fix it up.

Eric-Arellano · 2019-10-08T02:32:15Z

src/python/pants/base/specs.py

+_specificity = {
+  SingleAddress: 0,
+  SiblingAddresses: 1,
+  AscendantAddresses: 2,


Is it possible for an end-user to type an ascendant address through the CLI, or is this only used in internal code? I'm wondering if this should even belong here.

Also, what is the reasoning for an ascendant address being more specific than a descendant address? I'm not able to give a good technical reason for this, but a descendant address "feels" more specific intuitively to me.

I don't really know what ascendant addresses are for. I just know that they're there, so I had to do something with them. To me they are less specific because they span multiple levels, whereas siblings are all in the same dir.

Does it fail if we remove them from the dictionary? I suppose we should regardless have it as a matter of defensive programming.

Agreed that ascendant is less specific than sibling. I mean that ascendant also seems less specific than descendent. I would order it:

single > sibling > descendant > ascendant

Eric-Arellano · 2019-10-08T02:33:25Z

src/python/pants/base/specs.py

+}
+
+
+def more_specific(spec1, spec2):


Type hints appreciated if you know them. I think this would be:

Suggested change

def more_specific(spec1, spec2):

def more_specific(spec1: Spec, spec2: Spec) -> bool:

Eric-Arellano · 2019-10-08T02:34:51Z

src/python/pants/base/test_specs.py

+)
+
+
+def test_more_specific():


+1 for using the Pytest assert strategy rather than subclassing unittest

Eric-Arellano · 2019-10-08T02:37:02Z

src/python/pants/engine/build_files.py

-  key, value = entry
-  return key
+    key, value = entry
+    return key




Skipping review of this because the diff is unclear to me.

Eric-Arellano · 2019-10-08T02:37:40Z

src/python/pants/rules/core/test.py

  # TODO(#6004): when streaming to live TTY, rely on V2 UI for this information. When not a
  # live TTY, periodically dump heavy hitters to stderr. See
  # https://github.com/pantsbuild/pants/issues/6004#issuecomment-492699898.
-  if union_membership.is_member(TestTarget, target.adaptor):
+  if (provenance_map.is_single_address(target.address) or


src/python/pants/base/specs.py

illicitonion · 2019-10-08T14:57:28Z

tests/python/pants_test/rules/test_test.py

+    bfaddr = BuildFileAddress(None, 'bin', 'some/dir')
+    target_adaptor = PythonBinaryAdaptor(type_alias='python_binary')
+    with self.captured_logging(logging.INFO):
+      # Note that this is not the same error message the end user will see, as we're resolving


I'd be interested in an integration test which actually shows that the error we should the user is a good one...

In general I worry that testing @console_rules as unit tests is kind of hard to reason about, and would maybe be interested in an integration test for the globbed case too, but in this one particularly I don't love that we're asserting that an unhelpful error is shown, and commenting to say that the user won't see such a bad error...

Yeah, I don't love this either. But I'm pretty against integration tests for very specific bits of functionality. They have incredibly high overhead and we should be using them much more sparingly than we do. In so many cases they exist because some underlying functionality is too hard to test in isolation.

I'd prefer that we figure out a way to have run_rule apply the same union casting logic as in prod, and get the right error message that way.

benjyw requested review from stuhood, Eric-Arellano, illicitonion and gshuflin October 7, 2019 02:37

benjyw force-pushed the provenanced_addresses2 branch from 50ab4cf to f27519c Compare October 7, 2019 02:40

stuhood reviewed Oct 7, 2019

View reviewed changes

benjyw added 3 commits October 7, 2019 09:31

Address code review feedback.

a0d6651

Merge branch 'master' into provenanced_addresses2

a1f8f66

benjyw force-pushed the provenanced_addresses2 branch from 4fdfeaa to a1f8f66 Compare October 7, 2019 18:10

benjyw requested a review from stuhood October 7, 2019 18:11

benjyw added 2 commits October 7, 2019 12:43

Lint

a6c4030

Fix test.

db53183

stuhood approved these changes Oct 8, 2019

View reviewed changes

Eric-Arellano approved these changes Oct 8, 2019

View reviewed changes

Fix indents.

0d6edba

illicitonion reviewed Oct 8, 2019

View reviewed changes

benjyw added 3 commits October 8, 2019 09:22

isort

fdbd2c6

Address code review comment.

7f540ee

Merge branch 'master' into provenanced_addresses2

2e3bacd

benjyw merged commit 7db7c7b into pantsbuild:master Oct 9, 2019

benjyw deleted the provenanced_addresses2 branch October 9, 2019 21:59

stuhood mentioned this pull request Dec 19, 2019

A v2 backend for generating AWS Lambdas. #8835

Merged

stuhood mentioned this pull request Nov 10, 2020

Fix test results showing up twice by removing TargetsWithOrigin #11128

Merged

stuhood mentioned this pull request Mar 30, 2022

Don't WARN when a spec matches no relevant targets. #14904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error out if the user explicitly tries to test a non-test tgt. #8402

Error out if the user explicitly tries to test a non-test tgt. #8402

benjyw commented Oct 7, 2019 •

edited

Loading

Eric-Arellano commented Oct 7, 2019

stuhood Oct 7, 2019

stuhood Oct 7, 2019

benjyw Oct 7, 2019

Eric-Arellano Oct 8, 2019

benjyw commented Oct 7, 2019

benjyw commented Oct 8, 2019

stuhood Oct 8, 2019

Eric-Arellano Oct 8, 2019

benjyw Oct 8, 2019

Eric-Arellano Oct 8, 2019

benjyw Oct 8, 2019

Eric-Arellano Oct 8, 2019

Eric-Arellano Oct 8, 2019

Eric-Arellano Oct 8, 2019

Eric-Arellano Oct 8, 2019

Eric-Arellano Oct 8, 2019

illicitonion Oct 8, 2019

benjyw Oct 8, 2019

	def more_specific(spec1, spec2):
	def more_specific(spec1: Spec, spec2: Spec) -> bool:

Error out if the user explicitly tries to test a non-test tgt. #8402

Error out if the user explicitly tries to test a non-test tgt. #8402

Conversation

benjyw commented Oct 7, 2019 • edited Loading

Eric-Arellano commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Oct 7, 2019

benjyw commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Oct 7, 2019 •

edited

Loading