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

assert: add snapshot assertion #44095

Merged
merged 12 commits into from
Aug 11, 2022
Merged

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Aug 2, 2022

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

@nodejs-github-bot 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
@MoLow
Copy link
Member Author

MoLow commented Aug 2, 2022

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

@MoLow
Copy link
Member Author

MoLow commented Aug 2, 2022

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

lib/assert.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

BridgeAR commented Aug 2, 2022

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

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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)

@cjihrig
Copy link
Contributor

cjihrig commented Aug 2, 2022

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

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

@MoLow
Copy link
Member Author

MoLow commented Aug 2, 2022

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

@BridgeAR
Copy link
Member

BridgeAR commented Aug 2, 2022

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.

@MoLow
Copy link
Member Author

MoLow commented Aug 2, 2022

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.

@BridgeAR
Copy link
Member

BridgeAR commented Aug 2, 2022

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

@MoLow
Copy link
Member Author

MoLow commented Aug 3, 2022

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

@BridgeAR
Copy link
Member

BridgeAR commented Aug 3, 2022

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.

@juliangruber
Copy link
Member

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

@MoLow
Copy link
Member Author

MoLow commented Aug 3, 2022

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

@MoLow MoLow marked this pull request as ready for review August 3, 2022 12:21
@benjamingr
Copy link
Member

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

doc/api/errors.md Outdated Show resolved Hide resolved
### `ERR_ASSERT_SNAPSHOT_NOT_SUPPORTED`

An attempt was maid to use `assert.snapshot()` in an environment that
does not support snapshots, such as REPL, or when using `node --eval`
Copy link
Member

Choose a reason for hiding this comment

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

Missing "." and "the"?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

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.

lib/internal/assert/snapshot.js Outdated Show resolved Hide resolved

* `value` {string} the value to snapshot
* `name` {string} the name of snapshot.
in case order of snapshots is non-deterministic,
Copy link
Member

Choose a reason for hiding this comment

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

why would ordering be non-determinstic??

Copy link
Member Author

Choose a reason for hiding this comment

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

If you run concurrent tests for example?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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);
	});
});

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

const { dir, name } = path.parse(process.mainModule.filename);
return path.join(dir, `${name}.snapshot`);
}
if (!process.argv[1]) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} else if (snapshot.has(name)) {
const expected = snapshot.get(name);
// eslint-disable-next-line no-restricted-syntax
assert.strictEqual(value, expected);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Left some comments mostly lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2022
@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2022
@MoLow
Copy link
Member Author

MoLow commented Aug 11, 2022

ping @benjamingr this needs a fresh approval

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8f9d1ab into nodejs:main Aug 11, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8f9d1ab

@MoLow MoLow deleted the assert-snapshot branch August 11, 2022 13:08
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
PR-URL: #44095
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
PR-URL: #44095
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v16.x labels Sep 5, 2022
@targos
Copy link
Member

targos commented Sep 5, 2022

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

Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44095
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants