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

Use @union to make the v2 test runner generic #7661

Merged

Conversation

Projects
None yet
6 participants
@stuhood
Copy link
Member

commented May 5, 2019

Problem

Fixing #4535 moved us closer to supporting a Target API for v2, but did not begin to use @union and UnionRule to make the v2 test @console_rule generic.

Solution

Add @union TestTarget, and consume it in the v2 test @console_rule. The result is that a language implementer that wants to add support for testing a target type MyTestTarget can declare a UnionRule(TestTarget, MyTestTarget) in their registered rules, which would in turn require a declared @rule to produce a TestResult for MyTestTarget.

In order to use PythonTestAdaptor as a member of the TestTarget union, the bottom commit refactors the SymbolTable manipulation that we do to preserve the concrete TargetAdaptor classes.

Result

Although we're not quite ready to "bless" TargetAdaptor by requiring it in the SymbolTable or coupling it to the v1 Target class, this change represents an incremental step in the direction of using (a likely renamed) TargetAdaptor incarnation as the v2 Target class.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

The commits are useful to review independently.

@stuhood stuhood force-pushed the twitter:stuhood/test-runner-rules-use-unions branch 2 times, most recently from 6cec590 to de006a8 May 5, 2019

@benjyw

benjyw approved these changes May 5, 2019

Copy link
Contributor

left a comment

Looks great!

@stuhood stuhood force-pushed the twitter:stuhood/test-runner-rules-use-unions branch from de006a8 to f4abaa9 May 7, 2019

@jsirois

jsirois approved these changes May 7, 2019

Show resolved Hide resolved src/python/pants/backend/python/rules/python_test_runner.py Outdated
Show resolved Hide resolved src/python/pants/backend/python/rules/python_test_runner.py Outdated
Show resolved Hide resolved src/python/pants/rules/core/test.py
})

self.assertEqual(result, TestResult(status=Status.FAILURE, stdout='foo'))

def test_coordinator_unknown_test(self):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 8, 2019

Contributor

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."?

This comment has been minimized.

Copy link
@stuhood

stuhood May 16, 2019

Author Member

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).

This comment has been minimized.

Copy link
@stuhood

stuhood May 16, 2019

Author Member

I suspect that if we continue to improve that error message code path by either/or:

  1. using more readable @rule signatures (as in #7509),
  2. 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.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 16, 2019

Contributor

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 :)

Preserve named TargetAdaptor classes when applying default globs, in …
…order to allow matching on them in @rule definitions.

@stuhood stuhood force-pushed the twitter:stuhood/test-runner-rules-use-unions branch from f4abaa9 to 91054de May 16, 2019

stuhood added some commits May 4, 2019

Allow empty unions by using whether a type is marked as a union (rath…
…er than the existence of members) to decide whether it is concrete.

@stuhood stuhood force-pushed the twitter:stuhood/test-runner-rules-use-unions branch from 91054de to 60d96c2 May 16, 2019

@illicitonion
Copy link
Contributor

left a comment

Looking great! Thanks!

@stuhood stuhood merged commit 155077e into pantsbuild:master May 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/test-runner-rules-use-unions branch May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.