-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: Add GitHub Actions Reporter #11320
Conversation
Hi @ockham! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thanks for picking this up again. I'm not really familiar with the Jest code base, but from what I can tell this looks good. Just one thing, when running Jest in another CI enviroment than GitHub Actions we might don't want to print such logs. Luckily there is a env variable for that.
Just for the recorder / issue watchers, the initial feature request was made in #10309 |
BTW, giving this some real-world testing over at WordPress/gutenberg#31041. Already found a few minor issues through that 😬 |
Amazing, let me know if I can help. |
Thank you! I think I'll polish the other PR first, and will then carry my modifications back over here 🙂 |
I've merged the real-world PR (WordPress/gutenberg#31041) -- it contains a few screenshots to showcase how the annotations work. I've carried over a few changes (including the As for coloring the output: The Jest failure messages clearly include ANSI color codes (see test fixture), so it seems that they get stripped off at a later stage (possibly by GitHub Actions?). At a closer look, this doesn't seem to be limited to this reporter -- Jest would normally also color its overall test results, but those colors never seem to make it to the GH Actions workflow run console: So I won't be investigating the missing colors anymore for now, since it seems like the problem isn't specific to this reporter (but to how GH Actions process Jest output). (I've Googled a bit and found e.g. #3877 -- indicating that we might try to use the |
Edit: Ignore 🙄 GH Actions support was added in |
Hey @ockham! I lost track of this. What's the current state? 🙂 Would love to ship this in Jest 28! From my own look (beyond the boring stuff like rebase, changelog, and signing the CLA) this looks close to complete |
_contexts?: Set<Context>, | ||
aggregatedResults?: AggregatedResult, | ||
): void { | ||
if (!process.env.GITHUB_ACTIONS) { |
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 think we should use ci-info
instead of manually checking for the env. should make the test cleaner as well as you can use jest.mock
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.
Hi @SimenB! I'd love if we could get this into Jest 28. Unfortunately, I'm going to be a bit busy next week. Can you tell me when's the deadline for getting this PR merged to make it into v28? In any case, I'll prod our legal dept to put me on our company CLA list. I'll also try to polish the PR a bit tomorrow if I manage to find some time 😅 |
08a3a39
to
97a7b1c
Compare
Eh, hard to say. Couple of weeks, probably. But if we make sure not to activate it by default, then we can land it outside of a major and people can use it (via |
I've pushed a few more commits to address the feedback I've gotten (thanks!). I'll be out for most of next week, but I should be able to pick up work again around March 7. If that's soon enough (or the PR is good enough as-is 😅), we might still get it merged in time for the major 🤞 |
Co-authored-by: Simen Bekkhus <[email protected]>
@SimenB 👋 Just checking in to say that I addressed pretty much all feedback (and had a question or two). My company's legal team is in touch with Meta's to add me to the corporate CLA, I hope there will be some progress soon. LMK if there's anything else you need from me! 🙂 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
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.
(gh forces me to comment here, but the feedback in is the threads 🙂)
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.
thanks!
AMAZING 😍 Thanks @ockham, @mariusGundersen and everyone else involved for putting time and effort to make this happen. |
Co-authored-by: Marius Gundersen <[email protected]> Co-authored-by: Stefan Buck <[email protected]>
This reverts commit b20c324.
that said, no real way of activating it - we should fix that 🙂 |
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. |
Summary
Based on @mariusGundersen's #10741, which is in turn based on @stefanbuck's stefanbuck/jest-matcher#2.
Add a Jest reporter to output a format that will be interpreted by GitHub Actions as annotations -- i.e. any errors reported by Jest will show up in a PR's diff below the offending line. This should make for significantly improved Developer Experience, since developers no longer have to scan CI logs for errors, but instead will be notified of them at the location where they happen.
All credit really goes to @stefanbuck and @mariusGundersen. I've only added a simple test (as was requested by @SimenB over at #10741, which seems to have gone stale) based on
SummaryReporter
's, ran the reporter througheslint
, and made a few minor fixes.This still needs a bit of tweaking (e.g. of the output format), hence setting the PR to draft status.
Closes #10741
Test plan
Run