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

Add new default reporter for github actions #13626

Merged
merged 20 commits into from
Jan 26, 2023
Merged

Conversation

MatteoH2O1999
Copy link
Contributor

@MatteoH2O1999 MatteoH2O1999 commented Nov 18, 2022

Summary

As requested by @SimenB in jest-community/awesome-jest#115, this pull request aims to add a new default reporter for CI/CD use with Github Actions.

The solution proposed here is only a proposition: I remain open to all your suggestion in order to better integrate the new reporter into Jest.

Test plan

Unit tests

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.

Thanks for getting this started!

packages/jest-core/package.json Outdated Show resolved Hide resolved
@mrazauskas
Copy link
Contributor

From #13626 (comment)

I think ideally the group commands should be emitted by the DefaultReporter (which lives in @jest/reporters).

As you noticed, there is no need to have groups in verbose output. So in VerboseReporter this feature should probably be disabled somehow.

And note that SummaryReporter handles summary output. There is no need to take care of the summary additionally.


By the way, GitHubActionsReporter is interesting reporter. It outputs ::error ..., ::warning ... annotations. And does this without involving @actions/core:

https://github.com/facebook/jest/blob/6e5b1d60a1214e792b5229993b5475445e9c1a6e/packages/jest-reporters/src/GitHubActionsReporter.ts#L81

The workflow commands API is documented by GitHub (see the link above), it is stable and safe to use. Just trying to explain why it is good idea to avoid adding a dependency (;

@MatteoH2O1999
Copy link
Contributor Author

@mrazauskas
Thank you for your comment, this is the first time I'm working with such a large codebase so all the advice is appreciated ;)

One last question: what about the license? Do I have to put the license I used in my package somewere or is it unnecessary given the small amount of code?

@mrazauskas
Copy link
Contributor

Not sure I fully understand what you mean. Robot shows that you have signed CLA (or Contributor License Agreement). That’s the agreement which covers all what you contribute to the repo.

@MatteoH2O1999
Copy link
Contributor Author

MatteoH2O1999 commented Nov 24, 2022

@mrazauskas
Given that I would pretty much copy my implementation from the original repo to this one and the fact that it was already licenced (so it would not be an original contribution) I was just wondering how the CLA applies to that. Please do not read malicious intent behind this, I really am completely ignorant about this and just want to be sure everything is alright on the legal side.

On another topic, this is now pretty much done so if you could give it a look and tell me if there's something you want me to change I would then proceed to move this pull request from the draft status.

Thank you for your patience

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.

sorry for the radio silence - I've been away on holiday 🙂

packages/jest-core/src/TestScheduler.ts Outdated Show resolved Hide resolved
packages/jest-reporters/src/GithubActionsLogsReporter.ts Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Jan 17, 2023

@MatteoH2O1999 is this complete from your side (pending CI which I just kicked off)? Asking as it's in draft 🙂

@mrazauskas
Copy link
Contributor

CI jobs ran, so I was looking at the output:

  1. What if the default would be {verbose: false} instead of {silent: false}? For instance, a user could have reporters: ['default', 'github-actions'] in Jest config. In this case current implementation (with {silent: false} option) is a breaking change. It will force the user to change the config to: reporters: ['github-actions'], or: reporters: ['default', ['github-actions', {silent: true}]].
  2. The output is broken (also note the "A worker process has failed..." warning, strangely it appeared in all runs which I checked):

Screenshot 2023-01-17 at 16 17 02

Screenshot 2023-01-17 at 16 17 44

@MatteoH2O1999
Copy link
Contributor Author

@mrazauskas

  1. Changed silent default value to true
  2. The broken output is caused by the dots printed by the silent reporter. Those are not printed with \n so the ::group is printed as .::group. The worker process warning was caused by a little issue in the duration calculation (just fixed it).

@MatteoH2O1999
Copy link
Contributor Author

@SimenB with these last fixes I think this is ready for integration

@MatteoH2O1999 MatteoH2O1999 marked this pull request as ready for review January 17, 2023 18:37
@MatteoH2O1999 MatteoH2O1999 marked this pull request as draft January 17, 2023 18:38
@mrazauskas
Copy link
Contributor

Thanks. Indeed there was a conflict with jest-silent-reporter. This is what made me think that default should be changed.

Did you notice that in the second screenshot there were no dots and no test tree at all? Only summary got printed. Was this because of the issues you mentioned?

@MatteoH2O1999
Copy link
Contributor Author

Yes, if a test suite had an undefined duration the reporter would crash, thus leading to that error. It is fixed now

@MatteoH2O1999 MatteoH2O1999 marked this pull request as ready for review January 17, 2023 18:45
@SimenB
Copy link
Member

SimenB commented Jan 17, 2023

I'm not sure if expanding the capabilities of the bultin reporter should be considered breaking even if it might mean duplicate messages depending on user config. However, I'm happy to be a bit defensive and make a more explicit breaking change for our next major.

@SimenB
Copy link
Member

SimenB commented Jan 26, 2023

I'm not allowed to push to this PR - @MatteoH2O1999 could you merge in main and apply this diff?

diff --git c/CHANGELOG.md w/CHANGELOG.md
index 1a8e6b128f..3914ef60ec 100644
--- c/CHANGELOG.md
+++ w/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ### Features
 
+- `[@jest/reporters]` New functionality for Github Actions Reporter: automatic log folding ([#13626](https://github.com/facebook/jest/pull/13626))
+
 ### Fixes
 
 - `[@jest/expect-utils]` `toMatchObject` diffs should include `Symbol` properties ([#13810](https://github.com/facebook/jest/pull/13810))
@@ -23,7 +25,6 @@
 - `[jest-runtime]` Add `jest.isEnvironmentTornDown` function ([#13741](https://github.com/facebook/jest/pull/13741))
 - `[jest-test-result]` Added `skipped` and `focused` status to `FormattedTestResult` ([#13700](https://github.com/facebook/jest/pull/13700))
 - `[jest-transform]` Support for asynchronous `createTransformer` ([#13762](https://github.com/facebook/jest/pull/13762))
-- `[@jest/reporters]` New functionality for Github Actions Reporter: automatic log folding ([#13626](https://github.com/facebook/jest/pull/13626))
 
 ### Fixes
 
diff --git c/website/versioned_docs/version-29.4/Configuration.md w/website/versioned_docs/version-29.4/Configuration.md
index 0a071c26c7..eebddd8138 100644
--- c/website/versioned_docs/version-29.4/Configuration.md
+++ w/website/versioned_docs/version-29.4/Configuration.md
@@ -1278,12 +1278,12 @@ export default config;
 
 #### GitHub Actions Reporter
 
-If included in the list, the built-in GitHub Actions Reporter will annotate changed files with test failure messages:
+If included in the list, the built-in GitHub Actions Reporter will annotate changed files with test failure messages and (if used with `'silent: false'`) print logs with github group features for easy navigation. Note that `'default'` should not be used in this case as `'github-actions'` will handle that already, so remember to also include `'summary'`. If you wish to use it only for annotations simply leave only the reporter without options as the default value of `'silent'` is `'true'`:
 
 ```js tab
 /** @type {import('jest').Config} */
 const config = {
-  reporters: ['default', 'github-actions'],
+  reporters: [['github-actions', {silent: false}], 'summary'],
 };
 
 module.exports = config;
@@ -1293,7 +1293,7 @@ module.exports = config;
 import type {Config} from 'jest';
 
 const config: Config = {
-  reporters: ['default', 'github-actions'],
+  reporters: [['github-actions', {silent: false}], 'summary'],
 };
 
 export default config;

I forgot to merge this before releasing 29.4, so I'm thinking to make a quick patch release with this 🙂

@MatteoH2O1999
Copy link
Contributor Author

@SimenB I think now you should be able to push to the branch. Sorry about that

@SimenB
Copy link
Member

SimenB commented Jan 26, 2023

Indeed, thanks!

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.

Thanks @MatteoH2O1999, this is awesome stuff! 👍

@SimenB SimenB merged commit 6ab67bf into jestjs:main Jan 26, 2023
@SimenB
Copy link
Member

SimenB commented Jan 26, 2023

@github-actions
Copy link

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 Feb 26, 2023
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.

4 participants