-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
test console_task which aggregates test results #6646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I guess that testrunner_test_mixin.TestResult
is fairly fluffy, so not much use in preserving it.
for test_result in test_results: | ||
wrote_any_stdout |= bool(test_result.stdout) | ||
# Assume \n-terminated | ||
console.write_stdout(test_result.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have so far been assuming (and I think it was in the v2 HLD) that we would render test runs completing as soon as they did. So the test stderr/stdout rendering would happen in #6003 by writing to a logger, and the run would "slow fail"... return a failing test result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the HLD said we would aggregate, and that we would run the thing running the test itself in some kind of interactive mode if we wanted things more eagerly...
I also think we started talking about introducing a "select first" version of Get
so that we could be more eager if we wanted to be...
console.write_stdout('\n') | ||
for (hydrated_target, test_result) in zip(hydrated_targets, test_results): | ||
console.print_stdout('{0:80}.....{1:>10}'.format(hydrated_target.address.reference(), test_result.status)) | ||
# TODO: Communicate an exit code out of the console_rule (if tests failed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule should raise an exception for failures.
|
||
|
||
# TODO: Move to production code | ||
@console_rule('test', [Select(Console), Select(TestResult)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should Select(HydratedTargets)
probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downside of this unit test harness, I suppose... very ducktyped.
21e8856
to
9c6a801
Compare
Everything except the HEAD commit are dependant PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome.
|
||
# This would be called TestResult, but then PyTest outputs a warning that it detected TestResult | ||
# was a test class, but couldn't treat it like a test class, because it has a constructor. | ||
class GenericTestResult(datatype([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular thing you're suggesting here?
I'm not sure how the existing TestResult
class doesn't fall foul of this, given I can't see any obvious pytest config in the repo...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting renaming this to TestResult
and marking it:
__test__ = false
...to resolve the comment you have about naming here.
from pants.engine.selectors import Select | ||
from pants.rules.core.core_test_model import GenericTestResult, Status | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think I just remembered that #6652 isn't quite accurate: commented there.
You should be able to declare this in src/python/pants/backend/python/register.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move tomorrow :)
|
||
|
||
# This class currently exists so that other rules could be added which turned a HydratedTarget into | ||
# a language-specific test result, and could be installed alongside run_python_test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can reference #4535 (or not).
|
||
@rule(GenericTestResult, [Select(HydratedTarget)]) | ||
def test_coordinator(target): | ||
# This should do an instance match, or canonicalise the adaptor type, or something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definitely reference #4535.
src/python/pants/rules/core/test.py
Outdated
|
||
@console_rule('test', [Select(Console), Select(HydratedTargets)]) | ||
def fast_test(console, targets): | ||
test_results = yield [Get(GenericTestResult, HydratedTarget, target) for target in targets] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I believe that further decoupling is possible here.
It should be possible for this rule to Select(BuildFileAddresses)
(which is the first stage of parsing: "addresses for the spec roots"), and then select a TestResult for each of those. That should allow build file parsing to operate in parallel with test runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
from builtins import str | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should maybe go under tests/python/pants_test/rules/
...? Maybe?
fd11d13
to
2d388a1
Compare
Rebased. |
2d388a1
to
da57200
Compare
da57200
to
a962fb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I made a minor tweak to remove a stale comment. Merging.
First cut at #6006