-
Notifications
You must be signed in to change notification settings - Fork 1.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: BuildPulse flaky test detection #11967
Conversation
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
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.
👋 @janpio: A couple suggestions to make sure that forks don't run into any issues attempting to upload test results. I hope this helps. If you have any questions, just let me know. 🙇
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
The last thing left @jasonrudolph is to get the check for the OS working (before it was always skipped for some reason) until you have the Windows reporter ready (and then we will need to adapt the script to download the correct binary of course). Then I think we would be good for a first merge to get actual test run data. |
@janpio: That sounds right to me. We need to teach these two lines to only run for macOS: prisma/.github/workflows/test.yml Line 727 in 2e140ec
prisma/.github/workflows/test.yml Line 813 in 2e140ec
I'll see if I can figure out the right syntax, and I'll let you know what I find out. |
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.
Following up on #11967 (comment), I think these suggestions will do the trick.
It turns out that runner.os
has the values documented at https://docs.github.com/en/actions/learn-github-actions/contexts#runner-context, so we need to check for macOS
as the value.
If we were using matrix.os
, then we'd check for macos-latest
as the value.
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
Co-authored-by: Jason Rudolph <jason@jasonrudolph.com>
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
Thanks @jasonrudolph, that worked as intended - in the last commit we have a failure on Windows and one on MacOS - and only the MacOS one triggered the BuildPulse step and successfully reported. We'll try to review and merge this now and see if we can get BuildPulse to recognize some of these as flaky (vs. currently "always broken" I guess 😛). Then we can also try to add Windows (when you have it working) and Linux via the other builds. |
@janpio: That's great! Thanks for taking the time to get things set up. ⚡ |
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.
Nice, thank you! Let's try this new tool out 🚀
This PR adds BuildPulse integration to our test GitHub Actions workflow:
That sounds like something we can use right now.
Currently it adds the reporter only to our no-docker MacOS runs, where I think we have the most flaky tests right now.
To make the BuildPulse integration smoother, I also extracted the
Client
tests from the currentno-docker
job into ano-docker-client
at the cost of adding 4 jobs per workflow run. I think that is worth it to get a result quicker. (Along the way I resorted ourjest.config.js
s to be the same order of properties)PS: BuildPulse is currently working on a Windows reporter, as soon as that is available we can also add it for the Windows matrix runs. If Buildpulse turns out to work correctly, we can also add the reporting to our other Linux jobs of course.
Thanks @jasonrudolph for the concierge service with this.