Introduce "minimal" test reporter#1563
Conversation
HT154
left a comment
HT154
left a comment
There was a problem hiding this comment.
Just a couple nits, approving to unblock
| private void reportResults(TestSectionResults section, AnsiStringBuilder builder) { | ||
| if (!section.results().isEmpty()) { | ||
| var results = | ||
| showOnlyFailed | ||
| ? section.results().stream().filter(TestResult::isFailure).toList() | ||
| : section.results(); | ||
| if (!results.isEmpty()) { | ||
| builder.append(" ").append(section.name()).append('\n'); | ||
| StringUtils.joinToStringBuilder( | ||
| builder, section.results(), "\n", res -> reportResult(res, builder)); | ||
| StringUtils.joinToStringBuilder(builder, results, "\n", res -> reportResult(res, builder)); | ||
| builder.append('\n'); | ||
| } | ||
| } |
There was a problem hiding this comment.
(Not a blocker) This result in output that includes the test module name but no further details:
❯ ../pkl/pkl-cli/build/executable/jpkl test --project-dir packages/com.github.dependabot --show-only-failed
module com.github.dependabot.tests.Dependabot
module com.github.dependabot.tests.examples
100.0% tests pass [2 passed], 100.0% asserts pass [2 passed]Is this really desired? It seems like it might be nice to either omit module names with only passing tests or indicate somehow that all tests in the module passed.
There was a problem hiding this comment.
Is this really desired?
Yes. The idea is to show all modules being tests. I think hiding the module names might be confusing.
We could show the green passed checkmark after the tests if we are hiding all the results and there's no failure. Open to ideas.
There was a problem hiding this comment.
This does seem strange to me too. Folks that use this option likely want to minimize their terminal output. But this will still give them potentially quite long test output.
I think it should be fine to only show the summary if everything has passed.
There was a problem hiding this comment.
The new minimal reporter doesn't show module names for modules with only passing tests/examples.
So in case of no errors/failures, only the summary is shown.
bioball
left a comment
bioball
left a comment
There was a problem hiding this comment.
It makes sense to have this functionality, although I'm thinking that this should be introduced as a test reporter.
Something like:
pkl test --test-reporter=minimal
We considered introducing a test reporter API before, and now is probably the right time to add it. If we introduce test reports later, the --show-only-failed is likely to be deprecated.
If we have a test reporter option, it also allow us to introduce reporters like "json", "dot", "tap", which are all somewhat common amongst test frameworks. And, our current reporter can be called the "spec" reporter (our test reports are modeled after Mocha, and that's what Mocha calls theirs).
| private void reportResults(TestSectionResults section, AnsiStringBuilder builder) { | ||
| if (!section.results().isEmpty()) { | ||
| var results = | ||
| showOnlyFailed | ||
| ? section.results().stream().filter(TestResult::isFailure).toList() | ||
| : section.results(); | ||
| if (!results.isEmpty()) { | ||
| builder.append(" ").append(section.name()).append('\n'); | ||
| StringUtils.joinToStringBuilder( | ||
| builder, section.results(), "\n", res -> reportResult(res, builder)); | ||
| StringUtils.joinToStringBuilder(builder, results, "\n", res -> reportResult(res, builder)); | ||
| builder.append('\n'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This does seem strange to me too. Folks that use this option likely want to minimize their terminal output. But this will still give them potentially quite long test output.
I think it should be fine to only show the summary if everything has passed.
| import org.pkl.core.util.StringUtils; | ||
|
|
||
| /** Minimal reporter. Only reports failures and errors. */ | ||
| public final class MinimalReport implements TestReport { |
There was a problem hiding this comment.
There's a lot of duplication with SimpleReport here. But I don't think it's worth DRYing it because future reports (like JSON, for example) might not share code with them.
bioball
changed the title
3 participants
No description provided.