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

doc,test: improvements and cleanup for assert.snapshot() #44429

Merged
merged 3 commits into from Sep 1, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 28, 2022

General improvements and cleanup of the docs and tests recently added for assert.snapshot().

Side note: it feels pretty unfortunate that assert.snapshot() returns a Promise, but that is not changed here.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 28, 2022
MoLow
MoLow approved these changes Aug 28, 2022
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

thanks for this

@MoLow
Copy link
Member

MoLow commented Aug 28, 2022

Side note: it feels pretty unfortunate that assert.snapshot() returns a Promise, but that is not changed here.

we can add assert.snapshotSync if it makes sense?

@MoLow MoLow added the assert Issues and PRs related to the assert subsystem. label Aug 28, 2022
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 28, 2022

I don't think we should add a snapshotSync() method.

@tniessen
Copy link
Member

Side note: it feels pretty unfortunate that assert.snapshot() returns a Promise, but that is not changed here.

Luckily, assert.snapshot() is experimental so we are (mostly) free to change it. I agree with @ljharb in that it is a tricky feature (#44095 (review)).

Other things that surprise me:

  • The made-up serialization format seems questionable to me. I don't expect it to be reliable in the presence of things such as
    { [util.inspect.custom]() { return '#*#*#*#*#*#*#*#*#*#*#*#'; } }
  • util.inspect() potentially omits information, leading to false negatives:
    const a = new ArrayBuffer(101);
    const snapshot1 = util.inspect(a);
    new Uint8Array(a)[a.byteLength - 1] = 1;
    const snapshot2 = util.inspect(a);
    assert.strictEqual(snapshot1, snapshot2);
    // passes even though the inspected value changed!
  • util.inspect() is affected by util.inspect.defaultOptions, which means that two snapshots of the same value can be different, leading to false positives.
  • Looking at the change history for util.inspect(), it seems that the default options changed between versions. This would potentially also break existing .snapshot files, leading to false positives.
  • Reading and potentially writing a file at some opinionated location during a call to an assert API seems very unusual to me, especially because it causes the API to become async.

I really think we should put a focus on correctness instead of rapidly adding new APIs (even if experimental).

@@ -1512,7 +1512,7 @@ loading phase, it will always raise it as an uncaught exception.
added: v18.8.0
-->

Force updating snapshot files for [`assert.snapshot()`][]
Updates snapshot files used by [`assert.snapshot()`][].
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be marked as experimental.

@tniessen
Copy link
Member

Also, adding to my previous comment, the documentation of util.inspect() literally says:

The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

This seems contradictory to assert.snapshot().

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 29, 2022

@tniessen I think it's worth opening one or more separate issues with the details you've outlined here - particularly the required contract change to util.inspect().

Just don't add the tsc-agenda label yet so that we can hopefully avoid meeting this week 😄

@benjamingr
Copy link
Member

@tniessen

util.inspect() potentially omits information, leading to false negatives:

I think we generally just missed this in the original PR and hadn't considered the point you're making here and I think that after I've read your point it I agree with you. We should absolutely change the serialization to not always use util.inspect because of the false negatives/positives that might produce.

The fact the feature is tricky doesn't mean (IMO) we shouldn't add it and if the trickiness causes changes and iterations that's totally a good thing IMO.

@cjihrig

Side note: it feels pretty unfortunate that assert.snapshot() returns a Promise, but that is not changed here.

That can also be revisited.


As a side note, while the feedback on the feature is very valuable I would like you to consider giving such feedback (if possible) during the PR lifecycle. The original PR was open for more than a week for feedback (and if you need more time to review - it's totally fine to "request changes" and ask). During that week multiple iterations incorporating feedback were performed. It's frustrating to have to do (or even just review) duplicate work so the earlier the feedback is - the better (not even considering the userland impact).

@ljharb
Copy link
Member

ljharb commented Aug 29, 2022

imo trickiness should mean, though, that nothing gets added before the design space is thoroughly explored.

@benjamingr that feedback was explicitly provided ( #44095 (review) ): but this project has a strong bias to merge PRs.

@MoLow
Copy link
Member

MoLow commented Aug 29, 2022

I generally agree that util.inspect was not the correct way to implement this API,
it was suggested as part of a review, but I do think the correct thing to do is to remove any serialization. CC @BridgeAR

I don't think we should add a snapshotSync() method.

why not? do you think we should change snapshot to be synchronous instead? (I don't intend on opening an entirely separate discussion about this right here - so we can take this off-line, or open a separate issue if needed)

@benjamingr
Copy link
Member

imo trickiness should mean, though, that nothing gets added before the design space is thoroughly explored.

I disagree with that assertion. I think experimenting is a great way to nail down the API and experimental APIs are a good way to prototype with new features and figure out the design space.

Being allowed to not get it perfect the first time and iterate is a really important property IMO.

that feedback was explicitly provided ( #44095 (review) ):

Are you sure you linked to the right comment? The one you linked to just says it's tricky and mentions recursion - it doesn't bring up Tobias's point about util.inspect nor Colin's point about the API being sync maybe being better.

but this project has a strong bias to merge PRs.

I sure hope it does - though if we stopped and tried figuring out how to do things perfect the first time nothing would land and we still wouldn't have had a test runner. I understand "let's not land stuff that isn't 100% ready" is a valid perspective - it's just not one I share about the project.

@ljharb
Copy link
Member

ljharb commented Aug 29, 2022

While I don't think a slippery slope argument is valid, it's not on topic for this PR, so I won't debate it :-) you're right that my linked comment doesn't mention util.inspect, but i also don't see any of those questions addressed.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 29, 2022

why not? do you think we should change snapshot to be synchronous instead? (I don't intend on opening an entirely separate discussion about this right here - so we can take this off-line, or open a separate issue if needed)

Yea, I think it should just be a single synchronous method. This is just my opinion, and others might disagree, but my reasoning is:

  • The assert module is synchronous, with the exception of the few methods that are explicitly for dealing with Promises.
  • Making it return a Promise automatically makes the API slightly more difficult to deal with than a synchronous one.

Looking at a few implementations of snapshot testing, it looks like there are sync and async implementations in the wild, so this is not a hill I will die on. I'm also much happier having async snapshot testing than no snapshot testing at all.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Aug 31, 2022

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Aug 31, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 31, 2022
@nodejs-github-bot
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/44429
✔  Done loading data for nodejs/node/pull/44429
----------------------------------- PR info ------------------------------------
Title      doc,test: improvements and cleanup for assert.snapshot() (#44429)
Author     Colin Ihrig  (@cjihrig)
Branch     cjihrig:cleanup -> nodejs:main
Labels     assert, author ready
Commits    3
 - doc: improve assert.snapshot() docs
 - test: style updates for assert.snapshot()
 - doc: add --update-assert-snapshot to node.1
Committers 1
 - cjihrig 
PR-URL: https://github.com/nodejs/node/pull/44429
Reviewed-By: Moshe Atlow 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44429
Reviewed-By: Moshe Atlow 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 28 Aug 2022 19:33:19 GMT
   ✔  Approvals: 2
   ✔  - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/44429#pullrequestreview-1087905122
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/44429#pullrequestreview-1089275817
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-08-31T01:26:04Z: https://ci.nodejs.org/job/node-test-pull-request/46332/
- Querying data for job/node-test-pull-request/46332/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 44429
From https://github.com/nodejs/node
 * branch                  refs/pull/44429/merge -> FETCH_HEAD
✔  Fetched commits as 4d8674b50f60..11c0ecd7965e
--------------------------------------------------------------------------------
[main bdfac40055] doc: improve assert.snapshot() docs
 Author: cjihrig 
 Date: Sun Aug 28 14:59:28 2022 -0400
 2 files changed, 29 insertions(+), 13 deletions(-)
[main 4415adaf6d] test: style updates for assert.snapshot()
 Author: cjihrig 
 Date: Sun Aug 28 15:02:52 2022 -0400
 5 files changed, 7 insertions(+), 7 deletions(-)
[main aaade5c256] doc: add --update-assert-snapshot to node.1
 Author: cjihrig 
 Date: Sun Aug 28 15:26:33 2022 -0400
 1 file changed, 3 insertions(+)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: improve assert.snapshot() docs

PR-URL: #44429
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 35ff053cc4] doc: improve assert.snapshot() docs
Author: cjihrig cjihrig@gmail.com
Date: Sun Aug 28 14:59:28 2022 -0400
2 files changed, 29 insertions(+), 13 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: style updates for assert.snapshot()

This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: #44429
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD c3e4254a16] test: style updates for assert.snapshot()
Author: cjihrig cjihrig@gmail.com
Date: Sun Aug 28 15:02:52 2022 -0400
5 files changed, 7 insertions(+), 7 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: add --update-assert-snapshot to node.1

PR-URL: #44429
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD b206401141] doc: add --update-assert-snapshot to node.1
Author: cjihrig cjihrig@gmail.com
Date: Sun Aug 28 15:26:33 2022 -0400
1 file changed, 3 insertions(+)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/2960763125

@cjihrig cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 31, 2022
@tniessen
Copy link
Member

tniessen commented Aug 31, 2022

As a side note, while the feedback on the feature is very valuable I would like you to consider giving such feedback (if possible) during the PR lifecycle. The original PR was open for more than a week for feedback (and if you need more time to review - it's totally fine to "request changes" and ask). During that week multiple iterations incorporating feedback were performed. It's frustrating to have to do (or even just review) duplicate work so the earlier the feedback is - the better (not even considering the userland impact).

@benjamingr I try to give feedback on as many PRs as possible, but I simply don't have the time or energy to look at every single PR.

I agree though, at least some of these problems should have been brought up during review. The multiple problems with the implementation and design that I mentioned here are not my personal opinion; they could have been brought up by others during the original review cycle just as well. Those who approve a PR take at least partial responsibility for its quality.


@cjihrig You need to add either commit-queue-squash or commit-queue-rebase in order for this to be merged by the commit queue.

@cjihrig cjihrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Aug 31, 2022
cjihrig added 3 commits Sep 1, 2022
PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@cjihrig cjihrig merged commit cf7539a into nodejs:main Sep 1, 2022
18 checks passed
@cjihrig cjihrig deleted the cleanup branch Sep 1, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
PR-URL: #44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
This commit updates the assert.snapshot() fixtures to better
match the code style of the rest of the codebase.

PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44429
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@juanarbol juanarbol added the dont-land-on-v16.x PRs that should not land on the v16.x-staging branch and should not be released in v16.x. label Sep 30, 2022
@juanarbol
Copy link
Member

The assert.snapshot() is not part of v16.x yet; feel free to remove the label if I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v16.x PRs that should not land on the v16.x-staging branch and should not be released in v16.x.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants