assert: add snapshot assertion by MoLow · Pull Request #44095 · nodejs/node · GitHub

this is a proposal to add to node:assert a new class
assisting with snapshot assertion (e.g compare a values with a snapshot saved to a file (/any other WritableStream))
as a demonstration how it can be used, I migrated some of the test/message files to use this, see this diff to compare how the current python implementation with this new class

CC @nodejs/assert @nodejs/test_runner
I believe that there adding some minimalistic pieces into core can make testing with zero dependencies a common practice,
this was also raised in the original issue proposing a built in test runner

another example of an (internal) implentation for this: https://github.com/nodejs/node-core-test/blob/main/test/message.js, besides tests/message

I think it's useful to add snapshot tests but I would expect a simpler API.

As such, I would expect no transforms. This should be done by the users before passing any value to be compared. That way we are also aligned with our other assertions that are kept super simple.

Adding a functionality to let the user decide where to put the snapshot is also nice but I would again suggest to go for the simpler implementation: by default, save the snapshot in a file and let the user define the snapshot optionally. That way it's possible for them to receive the snapshot from where ever they want to save it.

The overall API would look like:

assert.snapshot(input[, snapshot])

Running it once is going to serialize the input with util.inspect(), if no snapshot argument is provided (the input is compared with the snapshot by serializing the snapshot).

If the user would want to transform something or use a different spot to save the snapshot they can:

const transformedInput = input.toLowerCase()

const snapshot = await createReadStream(path)

assert.snapshot(transformedInput, snapshot)

The added benefit is not only the simpler API but also that there's a clear error to the user in case it's not possible to read the read stream (or what ever they use to receive the snapshot).

I like this and the idea.

I like Ruben's suggested API (no classes, single function, less options) better.

(Docs and other stuff is obviously still missing)

+1 to supporting this with a simpler API. Could you please add tests to verify:

Snapshotting with file paths to make sure Windows and non-Windows work.
Snapshotting with values that change with every run - for example a Date or timestamp.

As such, I would expect no transforms. This should be done by the users before passing any value to be compared. That way we are also aligned with our other assertions that are kept super simple.

yeah, that sounds right - probably most of the transforms I wrote should go somewhere along test/common as they were written to prove this is useful for test/message

Adding a functionality to let the user decide where to put the snapshot is also nice but I would again suggest to go for the simpler implementation: by default, save the snapshot in a file and let the user define the snapshot optionally. That way it's possible for them to receive the snapshot from where ever they want to save it.

in the API I have implemented - all options are optional, including source and target - the default in my implementation is to read and write to a file.
so the main change is not to use a class (which makes sense to me), did I understand correctly?

The overall API would look like:
assert.snapshot(input[, snapshot])

this accepts an option specifying where to read the snapshot from, what about an option specifying where to write to?

Running it once is going to serialize the input with util.inspect()

if transforms are done in userland why shouldn't serialization be performed there as well?

The added benefit is not only the simpler API but also that there's a clear error to the user in case it's not possible to read the read stream (or what ever they use to receive the snapshot).

yes, that is the case in my implementation as well. probably because ALL of my tests passe a source and a target it seemed like it is not optional (and due to lack of documentation).
as @cjihrig suggested - tests that use the default will be added as well

so the main change is not to use a class (which makes sense to me), did I understand correctly?

Yes, and no options.

this accepts an option specifying where to read the snapshot from, what about an option specifying where to write to?

It has no option where to read from the snapshot, only the actual snapshot content (e.g., an object or a string). That way reading and writing is up to the user. Inline snapshots become easy that way.

if transforms are done in userland why shouldn't serialization be performed there as well?

The first argument is the actual input (e.g., a complex object). This has to be serialized on our side or we would only be able to accept strings as input. That does not seem ideal from the usability side. If we serialize the input, it is consistent to also serialize the snapshot, if provided.

It has no option where to read from the snapshot, only the actual snapshot content (e.g., an object or a string). That way reading and writing are up to the user. Inline snapshots become easy that way.

in case the snapshot is known in advance (a.k.a inline snapshot) - running assert.snapshot(input, snapshot) will be exactly the same as running assert.strictEqual(input, snapshot) meaning the snapshot parameter is redundant.
taking that a step forward - you are basically suggesting that assert.snapshot will only have one default behavior of reading and writing to the file system.

on the other hand, I can see use cases for extending the API and providing source and target.
for example, reading from a remote file system or from an HTTP server (e.g minio/s3) etc.

We can start with assert.snapshot(input) and decide upon further details later.

We can start with assert.snapshot(input) and decide upon further details later.

That will prevent even basic usecases such as specifying a location for the snapshot file.
If options are optional - and we agree on the defaults, I am not sure there is a downside

That will prevent [...] specifying a location for the snapshot file.

That is correct. The main functionality should however already be implemented. Let's iterate upon the implemention in smaller steps.

+1 to not having any options in the initial implementation, with good defaults we have a useful platform for iteration

@BridgeAR @benjamingr @cjihrig @mscdex @juliangruber
I implemented the minimal implemetation discussed above - can you please take a look?

Looks mostly good, why the experimental warning though? I think making this experimental (in the docs) should be enough.

Missing "." and "the"?

I think this is a VERY tricky feature to add. There are many snapshot heuristics and no one cohesive userland solution, and node shouldn't be opinionated here.

What happens with recursion? What happens with custom values, like jsx elements? It's very unclear to me from this PR how this feature works right not.

why would ordering be non-determinstic??

If you run concurrent tests for example?

oof, that's a great reason to never do that :-/

maybe we should just require the name then, rather than opening the footgun?

maybe in a further iteration we can use asyncLocalStorage to automatically use the test name when running inside node test runner, but that should not happen in this iteration anyway

@benjamingr concurrency can optionally also be per test - and assert.snapshot should work independently of how node:test test works

just as an example, this will break with any other test runner or program that runs things in parallel:

describre({ concurrency: true }, () => {
	it('1', async () => {
		await setTimeout(random());
		assert.snapshot(thing);
	});

	it('2', async () => {
		await setTimeout(random());
		assert.snapshot(otherThing);
	});
});

Ok, I'm not sure I agree how common/feasible of a footgun it is and it feels to me people would have to work pretty hard to run into it.

That said if the smallest unit of concurrency is the test (and not the file) I think appending the test name to the snapshot could be an easy fix then?

sure - or, requiring users of this API to provide a name, and then we could make it be "the test name" in a followon.

The simplest API - which is appropriate for a new feature - is when everything is explicit and required, and nothing is implicit or optional.

There were objections raised to that above I think? I think using the test name is a good workaround to keep everyone content with the API.

There's a lot of comments; can you link to it? I'd love to have those who objected explain why an implicit optional name is simpler than an explicit required one.

Is this to check if we're in the REPL? wouldn't it be better to just check if there is no process.mainModule?.filename?

esm does not have process.mainModule either, so argv[1] is used instead

I haven't read the code, but reminder that process.argv[1] == false when doing stdin eval and --eval, not just in REPL.

Yes, it is documented to not be supported at this stage.
In a further stage we can support the source and target options that were in the original commit

It's kind of a bummer that this does string comparison and not deepStrictEqual or something similar with better dx here but I guess since it's util.inspect it's customizable. I think this can be revisited later/in a future PR.

Left some comments mostly lgtm

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

ping @benjamingr this needs a fresh approval

Landed in 8f9d1ab

PR-URL: #44095 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

@MoLow Please add the semver-minor label to PRs that add new features.

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Aug 2, 2022

mscdex reviewed Aug 2, 2022

View reviewed changes

Comment thread lib/assert.js Outdated

benjamingr reviewed Aug 2, 2022

View reviewed changes

MoLow marked this pull request as ready for review August 3, 2022 12:21