https://pantsbuild.org/ logo
Powered by
h

happy-kitchen-89482

09/27/2019, 5:42 PM
Exception: Type PythonTargetAdaptor is not a member of the TestTarget @union ("A union for registration of a testable target type.")
a

aloof-angle-91616

09/27/2019, 5:43 PM
that's relevant to another hack i've added for binary creation
(1) you should be able to add a 
UnionRule(PythonTargetAdaptor, TestTarget)
in that file and it should work
(2) the way we assign target adaptors in 
engine_initializer.py
is confusing and looks very legacy and i just deleted some of that logic in my hack branch
h

happy-kitchen-89482

09/27/2019, 5:45 PM
"In that file" ?
a

aloof-angle-91616

09/27/2019, 5:46 PM
sorry, as in, adding that 
UnionRule(...)
expression in any file's 
def rules()
somewhere should fix the issue
h

happy-kitchen-89482

09/27/2019, 5:46 PM
OK, so I'm trying to run the v2 pytest runner on some of pants's own tests, which I assume is supposed to work...
a

aloof-angle-91616

09/27/2019, 5:46 PM
which target type is it?
h

happy-kitchen-89482

09/27/2019, 5:46 PM
I mean, isn't that what we're doing in CI right now?
w

witty-crayon-22786

09/27/2019, 5:46 PM
@happy-kitchen-89482: one of those targets is not a test
a

aloof-angle-91616

09/27/2019, 5:46 PM
@hundreds-father-404 ^
h

happy-kitchen-89482

09/27/2019, 5:46 PM
Ah
a

aloof-angle-91616

09/27/2019, 5:46 PM
oh ok thanks stu
h

happy-kitchen-89482

09/27/2019, 5:47 PM
Hmm
w

witty-crayon-22786

09/27/2019, 5:47 PM
relates to https://github.com/pantsbuild/pants/issues/4535 (because target types)
h

happy-kitchen-89482

09/27/2019, 5:48 PM
that makes sense
But is also not a good user experience. A user will expect 
./pants test src/python::
to do the right thing.
👍 1
w

witty-crayon-22786

09/27/2019, 5:48 PM
but also to another question that is implicit right now: 'do we want to preserve the behaviour that we noop when something "isn't testable"?' or do we want to fail
a

aloof-angle-91616

09/27/2019, 5:48 PM
yes
h

happy-kitchen-89482

09/27/2019, 5:48 PM
I think we want to noop
Otherwise there is no reasonable way to "run all the tests under this dir"
w

witty-crayon-22786

09/27/2019, 5:49 PM
i'm sure we have github threads about this question.
h

happy-kitchen-89482

09/27/2019, 5:50 PM
Yeah, btw sorry for rehashing conversations that have already been had (and I appreciate the issue links to those conversations). But I'm finally getting around to personally hacking on the engine myself, so I have some catching up to do...
w

witty-crayon-22786

09/27/2019, 5:50 PM
but yea. regardless, connected to #4535. because deciding on the safest way to noop will relate to target types pretty deeply.
a

aloof-angle-91616

09/27/2019, 5:51 PM
i will have a PR up that completely changes this exact experience sometime this weekend (not sure if it will be mergeable, but it's quite interesting)
h

hundreds-father-404

09/27/2019, 6:30 PM
Agreed that it should noop. Here, the major usability gain should trump correctness imo
w

witty-crayon-22786

09/27/2019, 6:54 PM
the place where noops are more questionable is where nothing matches ... ie 
./pants binary ${thing}::
on a directory that isn't packagable.
👍 1
h

happy-kitchen-89482

09/27/2019, 7:19 PM
Any pointers on how to go about this? How does a rule decide which targets it can or can't operate on?
w

witty-crayon-22786

09/27/2019, 7:20 PM
go about... changing this behaviour in general? or how to run tests locally?
a

aloof-angle-91616

09/27/2019, 7:21 PM
UnionRule is how it decides!
w

witty-crayon-22786

09/27/2019, 7:24 PM
if you want to change the behaviour in general, some sketching on https://github.com/pantsbuild/pants/issues/4535 (with an eye on https://github.com/pantsbuild/pants/issues/8343) would be good. and yea, what we want to do to "seal" the graph to indicate that it is safe to skip needing an implementation of "testing for target type X" is related to UnionRule, and how and where to install a "default" implementation
and if not a "default" implementation, then a runtime check for "is this type in the union" to let the 
@rule
decide to skip doing something based on union-memberness
h

happy-kitchen-89482

09/27/2019, 7:26 PM
Right, I was wondering if this is a per-rule thing.
w

witty-crayon-22786

09/27/2019, 7:26 PM
@happy-kitchen-89482: in general, rules run based on types
h

happy-kitchen-89482

09/27/2019, 7:27 PM
Also, it may be the case that the user still expects 
./pants test path/to/a/non-test/target
to fail
w

witty-crayon-22786

09/27/2019, 7:27 PM
yes.
h

happy-kitchen-89482

09/27/2019, 7:27 PM
I.e., the correct capturing of user intent is that the glob itself should mean "all the things that are members of the corresponding union"
w

witty-crayon-22786

09/27/2019, 7:27 PM
https://github.com/pantsbuild/pants/blob/44d4ab5e272094314bff498ef14013a53df3a814/src/python/pants/rules/core/test.py#L65-L78 is where we "cast" something to see if it is a member of the 
TestTarget
union. but it isn't fallible, so the run fails if it isn't.
h

happy-kitchen-89482

09/27/2019, 7:28 PM
In other words, it may be that the current rule behavior is correct, but the interpretation of the glob is not. But that opens up a whole can of worms, especially wrt multiple goals on the cmd line.
w

witty-crayon-22786

09/27/2019, 7:29 PM
so two fundamentally different potential approaches 1) add support for a "default" implementation with UnionRules, so that you noop there, 2) add support for checking whether something is in a union at runtime
@happy-kitchen-89482: multiple goals on the cmd line just run one after the other. so a console rule would decide which to operate on.
so two fundamentally different potential approaches 1) add support for a "default" implementation with UnionRules, so that you noop there, 2) add support for checking whether something is in a union at runtime
^ is key
h

happy-kitchen-89482

09/27/2019, 7:30 PM
right, but we construct the glob once for all goals.
w

witty-crayon-22786

09/27/2019, 7:30 PM
@happy-kitchen-89482: but each of them chooses what to do with it.
./pants roots
doesn't consume it.
./pants list
does, but not transitively
h

happy-kitchen-89482

09/27/2019, 7:31 PM
I'm just wondering if the behavior needs to be different depending on whether the source of the target set was a glob or not.
I.e., 
./pants test <glob>
should run on however many test targets there were, including zero.
But 
./pants test /not/a/test/target
should fail
w

witty-crayon-22786

09/27/2019, 7:32 PM
depends on the goal/`@console_rule`, probably.
h

happy-kitchen-89482

09/27/2019, 7:32 PM
But can it know?
I guess it can
(can it know the origin of the targets, glob or not, I mean)
w

witty-crayon-22786

09/27/2019, 7:33 PM
yea. could add that as something computed from the 
Specs
but again, maybe related to #4535. for example: perhaps you want `@console_rule`s to be able to request 
PythonTargets
as their roots.
...mm. bad example, because 
@console_rule
is generic.
but perhaps, members of the 
TestTarget
union as roots
so the signature of https://github.com/pantsbuild/pants/blob/44d4ab5e272094314bff498ef14013a53df3a814/src/python/pants/rules/core/test.py#L27-L28 would become
Copy code
@console_rule
def fast_test(console: Console, test_targets: TestTargets) -> Test:
... and 
TestTargets
would be pre-filtered from the roots.
(as an example.)
BUT, i think that either way, that still relates to my fundamental point above
how should we go about filtering to members of a union
Powered by