Skip to content

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 26, 2024

test_runner: add context.fullName

This commit adds a fullName getter to the TestContext and
SuiteContext classes. This is similar to the existing name getter,
but also includes the name of all ancestor tests/suites.

doc: add context.assert docs

When context.assert was added, no docs were added. This commit
adds initial documentation for context.assert because the
snapshot() function requires them.

Refs: #52860

test_runner: add snapshot testing

This commit adds a t.assert.snapshot() method that implements
snapshot testing. Serialization uses JSON.stringify() by default,
but users can configure the serialization to meet their needs.

Fixes: #48260

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 26, 2024
@cjihrig cjihrig added semver-minor PRs that contain new features and should be released in the next minor version. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels May 27, 2024
@benjamingr
Copy link
Member

@targos

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.

Nice work, good tests, left some comments, want to see how people feel about this but personally I'm +1

@JakobJingleheimer
Copy link
Member

I believe snapshot does not belong in core because it's too specific to a domain.

Comment on lines +963 to +966
exports[`suite of snapshot tests > snapshot test 1`] = `
{
"value1": 1,
"value2": 2

Choose a reason for hiding this comment

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

@cjihrig It seems like this is missing backtick consistency for string values in snapshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 28, 2024

I believe snapshot does not belong in core because it's too specific to a domain.

I think this is a little too vague. What domain? What can't we do that most other test runners have figured out how to do?

@nodejs-github-bot
Copy link
Collaborator

@DylanPiercey
Copy link

DylanPiercey commented Jun 11, 2024

@cjihrig is there any reason JSON.stringify was chosen here over util.inspect? The inspect api supports a bunch more data types, circular properties, and allows defining custom overrides.

Seems like it'd be a good fit here?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 11, 2024

Yes. Per the util.inspect() docs:

The output of util.inspect may change at any time and should not be depended upon programmatically.

It's not a suitable default for Node core, but you can use it for your own serialization if you want.

@DylanPiercey
Copy link

Ah, lame. Alright, thanks for the heads up!

It'd be nice if the stance was changed on that api to only change output across majors. It seems reasonable to me that you might have to update your snapshots when upgrading to a new major. Oh well!

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 11, 2024

I would also love to see someone get a better and reliable serialization format in place!

@benjamingr
Copy link
Member

util.inspect was actually used in the past iteration of snapshot testing we had and it caused problems people complained about (about how it defines equality and semverness)

@ChALkeR
Copy link
Member

ChALkeR commented Jul 13, 2024

Looking at that code, this method appears duplicated:

function getFullName(test) {
let fullName = test.name;
for (let t = test.parent; t !== t.root; t = t.parent) {
fullName = `${t.name} > ${fullName}`;
}
return fullName;
}

get fullName() {
return getFullName(this.#test);
}

Could be simplified with

getTestNameWithAncestors() {
if (!this.parent) return '';
return `${this.parent.getTestNameWithAncestors()} ${this.name}`;
}

I.e. the test could have just had a .getPath which was joined with different strings likely? Perhaps I'll recheck

@marco-ippolito marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Snapshot testing

10 participants