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

test: print instruction for creating missing snapshot in assertSnapshot #48914

Conversation

rluvaton
Copy link
Member

No description provided.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 25, 2023
@rluvaton rluvaton changed the title when snapshot is missing print how to generate one test: when snapshot is missing print how to generate one Jul 25, 2023
@rluvaton rluvaton force-pushed the log-helpful-message-when-missing-snapshot branch from a47acde to 699f9c2 Compare July 25, 2023 08:32
Comment on lines +42 to +44
console.log(
'Snapshot file does not exist. You can create a new one by running the test with NODE_REGENERATE_SNAPSHOTS=1',
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(
'Snapshot file does not exist. You can create a new one by running the test with NODE_REGENERATE_SNAPSHOTS=1',
);
await fs.writeFile(snapshot, actual);
return;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about it...

@MoLow MoLow changed the title test: when snapshot is missing print how to generate one test: generate snapshot if it is missing Jul 25, 2023
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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.

this is a good point, @MoLow what do you say?

@MoLow
Copy link
Member

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?
I don't view this as something super important, and I don't want to over complicate - so I am also ok with the original approach of only printing a message if this becomes to complicated

@tniessen
Copy link
Member

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
Copy link
Member Author

I think it will overly complicate, reverting to use the log

@tniessen
Copy link
Member

(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.)

@rluvaton rluvaton force-pushed the log-helpful-message-when-missing-snapshot branch 2 times, most recently from 1a0b3b2 to f6cd240 Compare July 26, 2023 14:43
@rluvaton
Copy link
Member Author

@tniessen updated :)

@tniessen
Copy link
Member

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 :)

@rluvaton rluvaton force-pushed the log-helpful-message-when-missing-snapshot branch from f6cd240 to 8af0b1f Compare July 26, 2023 15:44
@rluvaton rluvaton force-pushed the log-helpful-message-when-missing-snapshot branch from 8af0b1f to ef9dae5 Compare July 26, 2023 15:46
@rluvaton
Copy link
Member Author

updated, if it can be improved I would appreciate what commit message would you write

@rluvaton
Copy link
Member Author

Coverage for some reason failed even thought I did not changed any source file not removed test files

@tniessen tniessen changed the title test: generate snapshot if it is missing test: print instruction for creating missing snapshot in assertSnapshot Jul 27, 2023
@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@rluvaton
Copy link
Member Author

rluvaton commented Aug 9, 2023

Can we please add the request-ci label?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit dadbdaf into nodejs:main Aug 11, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in dadbdaf

@rluvaton rluvaton deleted the log-helpful-message-when-missing-snapshot branch August 11, 2023 08:48
martenrichter pushed a commit to martenrichter/node that referenced this pull request Aug 13, 2023
PR-URL: nodejs#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
rluvaton added a commit to rluvaton/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48914
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants