Use @union to make the v2 test runner generic #7661
Conversation
|
The commits are useful to review independently. |
stuhood
force-pushed
the
stuhood/test-runner-rules-use-unions
branch
2 times, most recently
from
6cec590 to
de006a8
Compare
...python/tests/python/pants_test/contrib/awslambda/python/test_python_awslambda_integration.py
Outdated
Show resolved
Hide resolved
stuhood
force-pushed
the
stuhood/test-runner-rules-use-unions
branch
from
de006a8 to
f4abaa9
Compare
| }) | ||
|
|
||
| self.assertEqual(result, TestResult(status=Status.FAILURE, stdout='foo')) | ||
|
|
||
| def test_coordinator_unknown_test(self): |
There was a problem hiding this comment.
Can you restore this test to validate the claim in the implementation that "If the TargetAdaptor is not a member of the union, it will fail at runtime with a useful error message."?
There was a problem hiding this comment.
Hah. You got me.
It will fail at runtime with a slightly awkward error message (that I attempted to improve over here: #7502)... namely, that there is no declared Get(TestResult, $type).
There was a problem hiding this comment.
I suspect that if we continue to improve that error message code path by either/or:
- using more readable
@rulesignatures (as in Call for proposals: improved @rule display #7509), - allowing
@unions to be directly included in error messages (which I encouraged @cosmicexplorer not to do in the original union PR, in order to keep things simpler)
... that this can become significantly more friendly.
With 2. in particular (and perhaps with something that consumes the docstring of the union, or takes a description to the union annotation?), something like:
Type $x is not a member of the TestTarget @union ($description).
... would be possible.
There was a problem hiding this comment.
I'm not quite sure whether this would be a mostly-rule-author-facing error, or whether it could also leak into general users... If the former, either of those sound good. If the latter, we should be a lot more verbose and explainy...
Either way, this sounds good. Happy to punt on this until some-time-this-week in the interests of getting this merged and avoiding merge conflicts for @Eric-Arellano, but I'd like us to come back to this soon :)
…order to allow matching on them in @rule definitions.
stuhood
force-pushed
the
stuhood/test-runner-rules-use-unions
branch
from
f4abaa9 to
91054de
Compare
…er than the existence of members) to decide whether it is concrete.
stuhood
force-pushed
the
stuhood/test-runner-rules-use-unions
branch
from
91054de to
60d96c2
Compare
Problem
Fixing #4535 moved us closer to supporting a
TargetAPI for v2, but did not begin to use@unionandUnionRuleto make the v2 test@console_rulegeneric.Solution
Add
@union TestTarget, and consume it in the v2test@console_rule. The result is that a language implementer that wants to add support for testing a target typeMyTestTargetcan declare aUnionRule(TestTarget, MyTestTarget)in their registered rules, which would in turn require a declared@ruleto produce aTestResultforMyTestTarget.In order to use
PythonTestAdaptoras a member of theTestTargetunion, the bottom commit refactors theSymbolTablemanipulation that we do to preserve the concreteTargetAdaptorclasses.Result
Although we're not quite ready to "bless"
TargetAdaptorby requiring it in theSymbolTableor coupling it to the v1Targetclass, this change represents an incremental step in the direction of using (a likely renamed)TargetAdaptorincarnation as the v2 Target class.