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

feat(cli): Add dot test reporter #19804

Merged
merged 29 commits into from
Aug 2, 2023

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Jul 11, 2023

This commit adds a "dot" reporter to "deno test" subcommand,
that can be activated using "--dot" flag.

It provides a concise output using:

  • "." for passing test
  • "," for ignored test
  • "!" for failing test

User output is silenced and not printed to the console.

In non-TTY environments each result is printed on a separate line.

Demo:

deno_test_dot_reporter_in_5_mins.mov

bartlomieju added a commit that referenced this pull request Jul 27, 2023
@bartlomieju
Copy link
Member Author

Note to self: update manual for this and JUnit reporter.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Not too sure about the flag name though.

@@ -1875,6 +1884,13 @@ Directory arguments are expanded to all contained files matching the glob
.require_equals(true)
.default_missing_value("-")
)
.arg(
Arg::new("dot-reporter")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be --reporter=dot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that as well, but with --reporter=junit how are we gonna specify the path? Add a separate --junit-path flag?

That is actually very valid in context of adding this configuration to deno.json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want junit output in parallel to the visual CLI reporter. So I think in that sense, it's not a replacement for the main reporter, but something additional. This would suggest that deno test --reporter=dot --junit-path=./junit.xml should be valid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable solution. I'll update the PR.

Copy link
Contributor

@marvinhagemeister marvinhagemeister Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on supporting multiple reporters at the same time. Jest allows you to pass the flag multiple times: jest --reporters="default" --reporters="jest-junit"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While pretty+junit and dot+junit make sense, pretty+dot doesn't. So I'm gonna err on what Luca suggested here.

cli/tools/test/reporters/dot.rs Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

I'm gonna land this PR as is and change how the flags are setup (and add support in config file) in a follow up PR.

@bartlomieju bartlomieju merged commit 029bdf0 into denoland:main Aug 2, 2023
13 checks passed
@bartlomieju bartlomieju deleted the test_dot_reporter branch August 2, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants