-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
lib: adds colors to X's and .'s in test_runner dot reporter #52278
Conversation
Review requested:
|
Please fix the commit message, and update snapshots ( |
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.
Good start, see Moshe's comments on how to proceed, thanks 🙏
Hi, I'm getting a zsh: Permission Denied when attempting to update the snapshots. |
perhaps you checked out the repo with another user/with sudo? you might need to |
5a34ba4
to
8dc7c8a
Compare
Just reworked the commit message and ran the command given after being able to get access. I'm unsure of the purpose of snapshots - it ended up returning errors that were contained in a file I had no touched and I'm unsure of what to do with the statements returned. Any advice would be appreciated, thanks for all the help! Errors returned from snapshot: Thanks! |
Snapshot tests are in place to validate that the output of test runner reporters stays the same across commits. |
Yes, the error persists on the main branch. |
Awesome!
As Moshe explained, snapshots are files containing the expected output for a test. We use these to ensure the reporters work as expected as well as other things, and as this PR proposes a change to the
Does rebuilding the Nodejs executable work? ( |
Calling make -j4 returns an error of: Sorry for all of the issues! I appreciate all of the aid given and hopefully theres an easy solution hidden in plain sight that I'm just missing, I'll keep digging for it. |
Could you try pasting the entire output, or the error throughout the process? |
Fortunately the make -j4 appears to have fixed the issue and the Does this mean that the code is able to be submitted? There are still a few checks that appear to be failing within the pull request. |
you should commit all changed snapshot files that are changed by |
Hey @Aidan7757 are you still interested in implementing this? |
Closing in favor of #53450, which implements a similar change, and has (an) approval(s). |
Refs: #51770
First time contributing - if there are any procedural or code changes that need to be made let me know. Thanks!
Aidan Chadha