node
node copied to clipboard
nodejs
assert: add snapshot assertion
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
there is still some work to be done, and I want to receive some initial feedback before proceeding
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).
+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.
I'd still love an explanation (per https://github.com/nodejs/node/pull/44095#discussion_r937425378) for why having the snapshot name be implicit/optional is "simpler" than requiring an explicit one for this first iteration.
I'd still love an explanation (per #44095 (comment)) for why having the snapshot name be implicit/optional is "simpler" than requiring an explicit one for this first iteration.
Sure, lets make it required as not much feedback on that
I'd still love an explanation (per #44095 (comment)) for why having the snapshot name be implicit/optional is "simpler" than requiring an explicit one for this first iteration.
Sure, lets make it required as not much feedback on that
Documentation has to be updated too.
