GeezerVison

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?

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

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

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

@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 😄

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

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.

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)

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.

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.

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.

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

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

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.

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