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

refactor(@jest/reporters): improve annotation formatting of GitHubActionsReporter #12826

Merged
merged 22 commits into from May 9, 2022

Conversation

mrazauskas
Copy link
Contributor

Split from #12822

Summary

As mentioned, annotations produced by GitHubActionsReporter is very nice improvement. Few details could make them even better:

  • add annotations after each test case (instead of at the end of run);
  • strip internal stack traces (similar to CLI output does);
  • and use test title as annotation title.

Test plan

Reworked unit tests. And some screenshots from temporary failing tests.

@mrazauskas
Copy link
Contributor Author

Absolute paths are still bugging me. Will try one more time. Otherwise, looking good:

Screenshot 2022-05-08 at 21 33 16

@mrazauskas
Copy link
Contributor Author

That’s much better – macOS, ubuntu, windows:

Screenshot 2022-05-08 at 22 26 52

@mrazauskas
Copy link
Contributor Author

Visual difference is very minimal, just a bit cleaner. Colors would help, but they are not supported at the moment.

Before:

28-gh-actions-reporter-ec61c45e83c6d8a1c6c4fbe49b6b9626

After:

Screenshot 2022-05-08 at 22 24 14

@mrazauskas mrazauskas marked this pull request as ready for review May 8, 2022 19:42
failureMessages.forEach(failure => {
const {message, stack} = separateMessageFromStack(stripAnsi(failure));

const relativeTestPath = slash(relative('', test.path));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, rootDir is not pointing to the right place. See #12822 (comment).

In the other hand, in CI it feels right to keep paths relative to root of the repo.

@mrazauskas mrazauskas marked this pull request as draft May 9, 2022 04:34
@mrazauskas
Copy link
Contributor Author

Just made sure that annotations look similar to the one in CLI and path normalization works as expected.

If some helper throws outside a test file:

Screenshot 2022-05-09 at 08 03 41

That all looks fine with toThrow() matcher:

Screenshot 2022-05-09 at 08 02 42

Also snapshots:

Screenshot 2022-05-09 at 08 01 58

And a nested title case:

Screenshot 2022-05-09 at 08 02 21

@mrazauskas mrazauskas marked this pull request as ready for review May 9, 2022 05:15
@mrazauskas mrazauskas marked this pull request as draft May 9, 2022 05:38
@mrazauskas
Copy link
Contributor Author

One more case to do: would be nice to annotate retryReasons as warnings instead of errors.

@SimenB
Copy link
Member

SimenB commented May 9, 2022

Exciting! 😀

@mrazauskas
Copy link
Contributor Author

mrazauskas commented May 9, 2022

Done. Annotates retry messages as warnings, if logErrorsBeforeRetry: true is set. In case if a retried test still does not pass, error annotations are place under:

Screenshot 2022-05-09 at 15 35 09

@mrazauskas mrazauskas marked this pull request as ready for review May 9, 2022 12:39
line: string,
) => {
config: StackTraceConfig,
relativeTestPath: string | null = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps fine to have null as default, it just enables more styling.

aggregatedResults?: AggregatedResult,
): void {
const messages = getMessages(aggregatedResults?.testResults);
onTestFileResult({context}: Test, {testResults}: TestResult): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onTestFileResult is needed to annotate retries. I was trying to use onTestCaseResult first, but it reports retried tests as failures. (By the way, strangely TestContext from onTestCaseResult has wrong rootDir, but all is good with onTestFileResult.)

Copy link
Member

Choose a reason for hiding this comment

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

By the way, strangely TestContext from onTestCaseResult has wrong rootDir, but all is good with onTestFileResult.)

can you file an issue for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to figure out what is the root of that. Locally all was fine, but not on CI. Strange thing. Let’s see.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

great stuff!

@SimenB SimenB merged commit 1004ee2 into jestjs:main May 9, 2022
@mrazauskas mrazauskas deleted the feat-github-actions-annotations branch May 9, 2022 14:54
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants