test: print instruction for creating missing snapshot in assertSnapshot#48914
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added needs-ci 
rluvaton changed the title rluvatonforce-pushed the log-helpful-message-when-missing-snapshot branch from a47acde to 699f9c2Compare
| console.log( | ||
| 'Snapshot file does not exist. You can create a new one by running the test with NODE_REGENERATE_SNAPSHOTS=1', | ||
| ); |
There was a problem hiding this comment.
| console.log( | |
| 'Snapshot file does not exist. You can create a new one by running the test with NODE_REGENERATE_SNAPSHOTS=1', | |
| ); | |
| awaitfs.writeFile(snapshot,actual); | |
| return; |
There was a problem hiding this comment.
I was on the fence about it...
MoLow changed the title 
tniessen left a comment
tniessen left a comment There was a problem hiding this comment.
I don't have a strong opinion on this and I realize that the suggestion came from the author of this file, but implicitly and unpromptedly writing to a source directory during a regular test run seems like a footgun to me. For example, if the snapshot file is excluded by some accidental .gitignore rule, this approach will silently create a new snapshot file in every repository checkout.
rluvaton commented Jul 25, 2023
this is a good point, @MoLow what do you say? |
MoLow commented Jul 25, 2023
Maybe we can create the file if stdin is a tty - indicating this is a personal computer and the test is not run via the Python test runner, otherwise, print the original message. @tniessen WDYT? |
tniessen commented Jul 25, 2023
As I said, no strong opinion. My understanding is that this will only benefit folks who add new snapshot tests to node core, which seems like a very small group. |
rluvaton commented Jul 25, 2023
I think it will overly complicate, reverting to use the log |
tniessen commented Jul 25, 2023
(On a side note, the commit message must be amended before landing. It is neither up-to-date nor does it comply with the commit message guidelines.) |
rluvatonforce-pushed the log-helpful-message-when-missing-snapshot branch 2 times, most recently from 1a0b3b2 to f6cd240Comparerluvaton commented Jul 26, 2023
@tniessen updated :) |
tniessen commented Jul 26, 2023
FWIW it still doesn't comply with the commit guidelines. Most importantly, it doesn't begin with an imperative verb after the prefix. In the end, the person merging this PR will be responsible for checking/this this :) |
rluvatonforce-pushed the log-helpful-message-when-missing-snapshot branch from f6cd240 to 8af0b1fCompare
rluvatonforce-pushed the log-helpful-message-when-missing-snapshot branch from 8af0b1f to ef9dae5Comparerluvaton commented Jul 26, 2023
updated, if it can be improved I would appreciate what commit message would you write |
rluvaton commented Jul 26, 2023
Coverage for some reason failed even thought I did not changed any source file not removed test files |
tniessen changed the title 
github-actionsbot removed the request-ci 
nodejs-github-bot commented Jul 31, 2023
rluvaton commented Aug 9, 2023
Can we please add the |
nodejs-github-bot commented Aug 10, 2023
nodejs-github-bot commented Aug 11, 2023
MoLow added the commit-queue nodejs-github-bot removed the commit-queue nodejs-github-bot commented Aug 11, 2023
Landed in dadbdaf |
PR-URL: nodejs#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs/node#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs/node#48914 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
6 participants
No description provided.