test: migrate message tests to use assertSnapshot #47498

MoLow · 2023-04-10T21:20:37Z

this does not migrate all tests yet, but does migrate a big portion of them
I tried doing my best to reduce the git diff even in places where it is not essential for the test to succeed

test/common/assertSnapshot.js

test/common/index.js

test/fixtures/message/assert_throws_stack.js

test/parallel/test-node-output.mjs

test/common/assertSnapshot.js

GeoffreyBooth · 2023-04-14T16:13:40Z

test/fixtures/message/assert_throws_stack.snapshot

-      at assert.throws.bar (*assert_throws_stack.js:*)
-      at getActual (node:assert:*)
-      at Function.throws (node:assert:*)
-      at Object.<anonymous> (*assert_throws_stack.js:*:*)


This test is called assert_throws_stack; I would think that this explicit stack trace is intended as part of the test?

As a broader question, how do we handle cases like (maybe) this one where the test won’t be using the .snapshot file as generated? Just edit them by hand after generation and hope that no one regenerates them later and overwrites? Use spawnPromisified instead?

I am not sure I understood the question. editing the snapshot file will cause the test to fail

I am not sure I understood the question. editing the snapshot file will cause the test to fail

To use this case as an example, I assume the test/snapshot author wanted to test that the first line of the stack trace includes at assert.throws.bar. The generated snapshot just has * there, so the author would presumably replace * with something like the old .out file’s at assert.throws.bar (*assert_throws_stack.js:*). I would think that this shouldn’t cause the test to fail—we’re replacing a very loose match with a specific match, but the specific match still passes against the input. However the process by which we’ve created this more specific snapshot file involved manual editing; is this the process you would recommend for tests like this where part of what we want to assert isn’t getting output in the generated snapshot?

the snapshotting algorithm does not use regex or anything else to perform matching. matching is strict and all the replacements are just done before the comparison, so in this case we should change the string transforming to something less extensive

So what’s the answer to my question? If the test author desires a test more specific than the one the snapshot would provide, are we expecting the test author to manually change the snapshot after generation?

the test author would have to write some code transforming the output to ignore parts that are not important and keep parts that are, for example:

const lines = output.split('\n'); const important = lines.slice(0,5).join('\n'); const ignored = snapshot.replaceStackTrace(lines.slice(5).join('\n')); return `${important}${ignored}`

at this point of time my goal is to have the snapshots look pretty much the same as the current message tests,
but over time - if we see common patterns we can obviously make these transformations common

GeoffreyBooth · 2023-04-14T16:14:38Z

test/fixtures/message/assert_throws_stack.snapshot

+      at *
+      at *
+      at *
+      at node:internal*main*run_main_module:*:*,


This part of the stack trace might very well change in the future as we continue to merge the CommonJS and ESM module loaders. Can we find a way to filter out any stack trace references to the module loader(s)?

I guess we can str.replace(/node:internal.*/, '*') or something?
I want the diff of this PR to be minimal so perhaps a followup PR can handle this kind of changes

GeoffreyBooth · 2023-04-14T16:17:24Z

test/fixtures/message/source_map_eval.js

@@ -1,10 +1,10 @@
 // Flags:  --enable-source-maps


How is this a fixture? This file looks like it was intended to be run as the test itself.

since we never cared about the exit code or anything besides stdout and stderr of running this file

GeoffreyBooth · 2023-04-14T16:21:51Z

test/parallel/test-node-output.mjs

+  }
+});
+
+describe('vm', { concurrency: true }, () => {


My guess is that much earlier in the project, the subfolders under test/ corresponded with how the test was run: message (tools/test.py compared output), parallel (the tests ran in parallel) etc. But nowadays everything runs in parallel and we’re continuing to move logic out of test.py into the tests themselves, and many subfolders of test/ have to do with subsystems like test/es-module. This PR could be an opportunity to move all these former test/message tests into folders for their subsystems, like everything in this block could go under test/vm. Yes we could also do it in a follow-up but since this PR already moves dozens of files around, I kind of feel like we should decide how we want to organize the test/ folder going forward and just do it, rather than delaying.

+1 on reorganizing it, don't think it should be part of this PR

Can we at least split up the new huge test-node-output.mjs file into one each for each of the describe blocks you have there? Since each describe block is for a separate subsystem. Then the new files could cleanly move into the new folders when we reorganize.

Likewise, rather than create a new fixtures/message, can you create fixtures/async-error, fixtures/vm etc to put the new files in? We don’t need to reorganize the world in this PR, but the files we’re creating or moving could at least go into folders divided by subsystem.

👍🏻 perhaps I will break this up into multiple PRs

If you want, though I think it can all go into one. This PR also creates the new snapshot generation logic so if you split it up, you’d need a primary one that lands first with that code and then follow-ups with each batch of tests migrated. Feels like a lot of work when I think everyone is fine with landing this all as one.

benjamingr · 2023-04-14T17:01:56Z

This can be a good code-and-learn / good-first-issue

MoLow · 2023-04-15T17:21:54Z

This can be a good code-and-learn / good-first-issue

@benjamingr you mean moving the other tests?

MoLow · 2023-04-24T21:26:04Z

@GeoffreyBooth I'v removed all the files with a large diff so this PR is much simpler now, and we can do the more complicated cases in follow-ups.
is it ok to land this?

GeoffreyBooth

is it ok to land this?

It looks quite good. Looking through the notes, my preference would be to handle the following before landing, but I’ll approve now because I’m not opposed to these being follow-ups if you prefer:

Can test-node-output.mjs be split up into a few separate files? Like maybe one for each of the describe blocks you have here: test-node-output-console.mjs, test-node-output-errors.mjs, test-node-output-sourcemaps.mjs, test-node-output-vm.mjs.
Rather than creating a new fixtures/message folder, can we likewise get fixtures/console, fixtures/errors, fixtures/sourcemaps, fixtures/vm with the fixtures/message files redistributed accordingly?

test/fixtures/message/source_map_no_source_file.js

test/common/assertSnapshot.js

MoLow · 2023-04-24T22:13:16Z

my preference would be to handle the following before landing

done

GeoffreyBooth · 2023-04-24T22:29:27Z

@MoLow Thank you so much for your dedication to this! This has come out better than I could have hoped, and better than if I had attempted it myself 😄

MoLow · 2023-04-24T22:30:47Z

@MoLow Thank you so much for your dedication to this! This has come out better than I could have hoped, and better than if I had attempted it myself 😄

Sure!. thanks for the review. I really hope this evolves to assert.snapshot some day

nodejs-github-bot · 2023-04-27T06:42:00Z

Landed in b6738c1

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

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

targos · 2023-05-03T06:33:11Z

@MoLow Unfortunately, this breaks the test-tarball-linux job on the release proposal (not sure why): https://github.com/nodejs/node/actions/runs/4861395369/jobs/8670727044?pr=47820

MoLow · 2023-05-03T06:35:26Z

@targos seems like a path issue.
Lets drop it from the release and I will take a look

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

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

joyeecheung · 2023-10-10T00:23:33Z

test/fixtures/vm/vm_display_syntax_error.snapshot

 SyntaxError: Unexpected number
    at new Script (node:vm:*)
    at createScript (node:vm:*)
    at Object.runInThisContext (node:vm:*)
-    at Object.<anonymous> (*test*message*vm_display_syntax_error.js:*)
+    at Object.<anonymous> (*fixtures*vm*vm_display_syntax_error.js:31:6)


Why are these now exact line matches? I am running into an error locally with this test and many others and it seems the matching of the line numbers are quite arbitrary here. I am surprised that some of them demand exact line/column numbers (that is, * cannot match numbers). It also seems the new format is unable to match arbitrary lines, which makes the snapshots unnecessarily brittle compared to the original format..

Why are these now exact line matches?

I don’t think that was intentional, and I think the snapshot creation tool has been improved since this PR to avoid this unintentional precision. We should update the snapshots to remove these precise row/column numbers.

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

MoLow commented Apr 10, 2023

View reviewed changes

test/common/assertSnapshot.js Show resolved Hide resolved

MoLow commented Apr 10, 2023

View reviewed changes

test/common/index.js Show resolved Hide resolved

MoLow force-pushed the more-message-tests branch 2 times, most recently from a6a3b08 to 55fffdb Compare April 14, 2023 06:16

MoLow requested review from GeoffreyBooth and benjamingr April 14, 2023 06:16

aduh95 reviewed Apr 14, 2023

View reviewed changes

test/common/index.js Show resolved Hide resolved

test/fixtures/message/assert_throws_stack.js Outdated Show resolved Hide resolved

test/parallel/test-node-output.mjs Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Apr 14, 2023

View reviewed changes

test: migrate message tests to use assertSnapshot

30e9063

MoLow force-pushed the more-message-tests branch from 55fffdb to 30e9063 Compare April 24, 2023 21:24

MoLow mentioned this pull request Apr 24, 2023

message tests can be migrated from python to JS #47707

Open

GeoffreyBooth approved these changes Apr 24, 2023

View reviewed changes

test/fixtures/message/source_map_no_source_file.js Outdated Show resolved Hide resolved

test/common/assertSnapshot.js Show resolved Hide resolved

CR

9642476

MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 24, 2023

MoLow added 2 commits April 25, 2023 01:17

fix

15e9d07

reduce diff

b3863d8

MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 24, 2023

GeoffreyBooth approved these changes Apr 24, 2023

View reviewed changes

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

fix windows

cbcf3ec

GeoffreyBooth approved these changes Apr 24, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2023

nodejs-github-bot merged commit b6738c1 into nodejs:main Apr 27, 2023

MoLow deleted the more-message-tests branch April 27, 2023 06:43

github-actions bot mentioned this pull request Apr 28, 2023

CI Reliability 2023-04-28 nodejs/reliability#549

Open

29 tasks

yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023

test: migrate message tests to use assertSnapshot

00535ce

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

yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023

test: migrate message tests to use assertSnapshot

18cb71b

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

github-actions bot mentioned this pull request Apr 29, 2023

CI Reliability 2023-04-29 nodejs/reliability#550

Open

32 tasks

yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023

test: migrate message tests to use assertSnapshot

e6c98a5

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

github-actions bot mentioned this pull request Apr 30, 2023

CI Reliability 2023-04-30 nodejs/reliability#551

Open

33 tasks

MoLow mentioned this pull request Apr 30, 2023

tools: add node-pty as a dependency #47793

Closed

github-actions bot mentioned this pull request May 1, 2023

CI Reliability 2023-05-01 nodejs/reliability#552

Open

33 tasks

MoLow mentioned this pull request May 1, 2023

test: migrate a pseudo_tty test to use assertSnapshot #47803

Merged

github-actions bot mentioned this pull request May 2, 2023

CI Reliability 2023-05-02 nodejs/reliability#553

Open

32 tasks

targos pushed a commit that referenced this pull request May 2, 2023

test: migrate message tests to use assertSnapshot

f1dcbee

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

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

github-actions bot mentioned this pull request May 3, 2023

CI Reliability 2023-05-03 nodejs/reliability#554

Open

33 tasks

targos added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 3, 2023

targos mentioned this pull request May 3, 2023

tools: update eslint to 8.39.0 #47789

Closed

This was referenced May 3, 2023

test: fix output tests when path includes node version #47843

Merged

test: migrated test/message to JS fixture tests #47868

Open

targos removed the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 12, 2023

targos pushed a commit that referenced this pull request May 12, 2023

test: migrate message tests to use assertSnapshot

34bfb69

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

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

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

test: migrate message tests to use assertSnapshot

f2fcb94

PR-URL: #47498 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 message tests to use assertSnapshot

03dcf7b

PR-URL: nodejs#47498 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

joyeecheung reviewed Oct 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: migrate message tests to use assertSnapshot #47498

test: migrate message tests to use assertSnapshot #47498

MoLow commented Apr 10, 2023

GeoffreyBooth Apr 14, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 15, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 15, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 14, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 14, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 14, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 15, 2023

MoLow Apr 15, 2023

GeoffreyBooth Apr 15, 2023

benjamingr commented Apr 14, 2023

MoLow commented Apr 15, 2023

MoLow commented Apr 24, 2023

GeoffreyBooth left a comment

MoLow commented Apr 24, 2023

GeoffreyBooth commented Apr 24, 2023

MoLow commented Apr 24, 2023

nodejs-github-bot commented Apr 27, 2023

targos commented May 3, 2023

MoLow commented May 3, 2023

joyeecheung Oct 10, 2023

GeoffreyBooth Oct 10, 2023

test: migrate message tests to use assertSnapshot #47498

test: migrate message tests to use assertSnapshot #47498

Conversation

MoLow commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Apr 14, 2023

MoLow commented Apr 15, 2023

MoLow commented Apr 24, 2023

GeoffreyBooth left a comment

Choose a reason for hiding this comment

MoLow commented Apr 24, 2023

GeoffreyBooth commented Apr 24, 2023

MoLow commented Apr 24, 2023

nodejs-github-bot commented Apr 27, 2023

targos commented May 3, 2023

MoLow commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment