test: migrate test runner message tests to snapshot #47392

MoLow · 2023-04-03T22:11:51Z

Ref: #47370 (comment)
this PR introduces an internal tool for snapshot testing.
today we use python for that and regenerate snapshots manually.

this is not a full-blown snapshot testing solution and has a few limitations that can be resolved/improved in the future if we have any more use cases or if we want to re-introduce assert.snapshot:

it only accepts strings as an input
each snapshot file stores a single snapshot, multiple snapshot points require multiple files

MoLow · 2023-04-03T22:16:11Z

CC @GeoffreyBooth @nodejs/test_runner

GeoffreyBooth · 2023-04-03T22:30:47Z

@MoLow Thank you for doing this! This is much more robust than I was envisioning. On quick skim, it seems like this takes care not only of my concern about moving the regex comparison into JavaScript and out of Python, but it also automates the snapshot generation?

One thing I had to deal with with the message tests (part of the reason I dislike them so much) is that people would often include full stack traces when those were unnecessary, and those stack traces would include the call stack of the module loader which was very annoying whenever we wanted to change anything in the module loader. See https://github.com/orgs/nodejs/discussions/44168. In #44197 I added Error.stackTraceLimit to many of these tests to shrink down the generated stack traces to just the number of frames relevant to the system being tested, so that the module loader files weren’t output in the first place. I don’t know if this could be automated at all in what you’ve built here (and it certainly doesn’t need to happen in this PR) but I wanted to plant the idea in your head in case any solutions are obvious to you. Maybe something as simple as Error.stackTraceLimit = 0 as a default value in the snapshot generation code, so that stack traces are excluded from the snapshots unless the limit is explicitly set?

GeoffreyBooth · 2023-04-03T22:32:07Z

.github/CODEOWNERS

@@ -134,7 +134,6 @@

 # Test runner

-/test/message/test_runner_* @nodejs/test_runner
 /test/parallel/test-runner-* @nodejs/test_runner


Not in this PR, but perhaps we should move all the test runner stuff into /test/test-runner?

I'm +0 on that

test/common/README.md

test/common/index.js

test/common/snap.js

MoLow · 2023-04-03T22:48:51Z

On quick skim, it seems like this takes care not only of my concern about moving the regex comparison into JavaScript and out of Python, but it also automates the snapshot generation?

correct

One thing I had to deal with with the message tests (part of the reason I dislike them so much) is that people would often include full stack traces when those were unnecessary, and those stack traces would include the call stack of the module loader which was very annoying whenever we wanted to change anything in the module loader. See github.com/orgs/nodejs/discussions/44168. In #44197 I added Error.stackTraceLimit to many of these tests to shrink down the generated stack traces to just the number of frames relevant to the system being tested, so that the module loader files weren’t output in the first place. I don’t know if this could be automated at all in what you’ve built here (and it certainly doesn’t need to happen in this PR) but I wanted to plant the idea in your head in case any solutions are obvious to you. Maybe something as simple as Error.stackTraceLimit = 0 as a default value in the snapshot generation code, so that stack traces are excluded from the snapshots unless the limit is explicitly set?

this PR uses a transformer function replacing anything detected as a stack trace with an asterisk

test/common/README.md

tniessen

There's already a snapshot.js file in test/common, and the file name snap.js does not really help distinguish between the two. Maybe just put these new functions into test/common/fixtures.js? All the actual snapshots are stored in test/fixtures anyway, as far as I can tell, so that seems appropriate.

test/common/README.md

test/common/snap.js

tniessen · 2023-04-04T23:13:06Z

test/fixtures/test-runner/output/hooks.snapshot

@@ -181,7 +181,7 @@ not ok 4 - beforeEach throws
        *
        *
        *
-        *
+        async Promise.all (index 0)


Sorry, I don't really know what the test runner is doing. But why does this particular stack frame exist now? What's going on here?

this diff is here since previously the .out files where generated manually, and now they are generated automatically.
so stack trace lines are detected using a regex and replaced with *.
these specific lines are not detected as stack trace lines and thus are not replaces which is ok (since what usually should be ignored is file paths and location in the file etc)

test/common/assertSnapshot.js

nodejs-github-bot · 2023-04-09T18:44:38Z

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

nodejs-github-bot · 2023-04-09T19:57:48Z

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

nodejs-github-bot · 2023-04-09T22:58:35Z

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

PR-URL: nodejs#47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow · 2023-04-10T00:07:15Z

Landed in 57508fb

PR-URL: #47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 3, 2023

MoLow force-pushed the test-runner-output branch 2 times, most recently from cd45963 to 1c28b68 Compare April 3, 2023 22:22

GeoffreyBooth reviewed Apr 3, 2023

View reviewed changes

GeoffreyBooth approved these changes Apr 3, 2023

View reviewed changes

test/common/README.md Outdated Show resolved Hide resolved

MoLow force-pushed the test-runner-output branch 2 times, most recently from 669aa40 to cada058 Compare April 4, 2023 09:35

GeoffreyBooth approved these changes Apr 4, 2023

View reviewed changes

MoLow requested a review from cjihrig April 4, 2023 19:20

benjamingr approved these changes Apr 4, 2023

View reviewed changes

tniessen reviewed Apr 4, 2023

View reviewed changes

MoLow force-pushed the test-runner-output branch 2 times, most recently from eef9e86 to 346ce00 Compare April 9, 2023 18:06

GeoffreyBooth approved these changes Apr 9, 2023

View reviewed changes

test/common/assertSnapshot.js Outdated Show resolved Hide resolved

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2023

This comment was marked as outdated.

Sign in to view

test: migrate test runner message tests to snapshot

57508fb

PR-URL: nodejs#47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow force-pushed the test-runner-output branch from 8daec45 to 57508fb Compare April 10, 2023 00:06

MoLow merged commit 57508fb into nodejs:main Apr 10, 2023

MoLow deleted the test-runner-output branch April 10, 2023 00:07

RafaelGSS mentioned this pull request Apr 11, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

This was referenced Apr 12, 2023

CI Reliability 2023-04-12 nodejs/reliability#541

Open

CI Reliability 2023-04-13 nodejs/reliability#542

Open

RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023

test: migrate test runner message tests to snapshot

757a586

PR-URL: #47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jtoar mentioned this pull request Apr 28, 2023

chore(CRWA): convert to ESM redwoodjs/redwood#8159

Closed

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

test: migrate test runner message tests to snapshot

efd8b37

PR-URL: #47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023

test: migrate test runner message tests to snapshot

d0bda90

PR-URL: nodejs#47392 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: migrate test runner message tests to snapshot #47392

test: migrate test runner message tests to snapshot #47392

MoLow commented Apr 3, 2023

MoLow commented Apr 3, 2023

GeoffreyBooth commented Apr 3, 2023

GeoffreyBooth Apr 3, 2023

MoLow Apr 3, 2023

MoLow commented Apr 3, 2023

tniessen left a comment

tniessen Apr 4, 2023

MoLow Apr 8, 2023

nodejs-github-bot commented Apr 9, 2023

nodejs-github-bot commented Apr 9, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 9, 2023

MoLow commented Apr 10, 2023

test: migrate test runner message tests to snapshot #47392

test: migrate test runner message tests to snapshot #47392

Conversation

MoLow commented Apr 3, 2023

MoLow commented Apr 3, 2023

GeoffreyBooth commented Apr 3, 2023

GeoffreyBooth Apr 3, 2023

Choose a reason for hiding this comment

MoLow Apr 3, 2023

Choose a reason for hiding this comment

MoLow commented Apr 3, 2023

tniessen left a comment

Choose a reason for hiding this comment

tniessen Apr 4, 2023

Choose a reason for hiding this comment

MoLow Apr 8, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 9, 2023

nodejs-github-bot commented Apr 9, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 9, 2023

MoLow commented Apr 10, 2023