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

cjihrig · 2022-08-28T19:33:19Z

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.

MoLow

thanks for this

MoLow · 2022-08-28T19:44:34Z

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?

cjihrig · 2022-08-28T19:47:20Z

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

tniessen · 2022-08-29T12:33:12Z

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

tniessen · 2022-08-29T12:42:08Z

doc/api/cli.md

@@ -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()`][].


This should probably be marked as experimental.

tniessen · 2022-08-29T13:06:52Z

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 · 2022-08-29T19:58:24Z

@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 · 2022-08-29T19:58:42Z

@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 · 2022-08-29T20:09:24Z

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 · 2022-08-29T20:14:09Z

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 · 2022-08-29T20:17:46Z

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 · 2022-08-29T20:20:11Z

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 · 2022-08-29T20:40:09Z

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 · 2022-08-30T23:29:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46330/

nodejs-github-bot · 2022-08-31T01:26:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46332/ 💛

nodejs-github-bot · 2022-08-31T02:34:40Z

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

tniessen · 2022-08-31T15:47:29Z

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.

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>

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>

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>