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

projectnoa · 2023-05-05T00:28:42Z

test: migrated tests/message to JS fixture tests

Migrated all test located in the test/message directory to the fixtures structure and verified the output to fit the .snapshot representation. All tests passed, except for the following, which were not touched. Documentation has been updated to not reference test/message as an example to run test and has been changed to test/report.

Failing tests:
out/Release/node --expose-internals /home/upx/Documents/Development/Javascript/node/test/parallel/test-fs-copyfile.js
out/Release/node /home/upx/Documents/Development/Javascript/node/test/parallel/test-fs-cp.mjs

Fixes: #47707
Refs: https://eslint.org/docs/rules/space-in-parens.html

deps/zlib/zconf.h.included

MoLow

please remove the changed zlib file inside dpes
nothing requires these added fixtures. a test file must execute them using common.assertSnapshot - see https://github.com/nodejs/node/blob/bd913bee367e1ecd308bb7a3e5f6869813add31e/test/parallel/test-node-output-console.mjs and the other tests added in test: migrate message tests to use assertSnapshot #47498

projectnoa · 2023-05-06T00:27:54Z

Hi @MoLow,

As requested, I reverted the changes to the zlib files and added the test runner files in the /parallel directory.

test-node-output-assertion.mjs
test-node-output-map.mjs
test-node-output-promise.mjs
test-node-output-tick.mjs

Additionally, I modified the test-node-output-console.mjs and test-node-output-errors.mjs to run some of the tests there. Of course, I made sure that the .snapshot files are congruent with the test output and all test succeed.

I was, however, unable to figure out how to make the test util-inspect-error-cause.js evaluate correctly with the snapshot since the output seems to have some special characters that don't play well with RegEx.

Here's an example:

`My attempts to turn the following:

    at Object.<anonymous> \x1B[90m(/\x1B[39mtest*util-inspect-error-cause.js:*:*\x1B[90m)\x1B[39m
\x1B[90m    at Module._compile (node:internal*modules*cjs*loader:*:*)\x1B[39m
\x1B[90m    at Module._extensions..js (node:internal*modules*cjs*loader:*:*)\x1B[39m
\x1B[90m    at Module.load (node:internal*modules*cjs*loader:*:*)\x1B[39m
\x1B[90m    at Module._load (node:internal*modules*cjs*loader:*:*)\x1B[39m
\x1B[90m    at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:*:*)\x1B[39m
\x1B[90m   at node:internal*main*run_main_module:*:*\x1B[39m

into:

    at *
*[90m    at *[39m
*[90m    at *[39m
*[90m    at *[39m
*[90m    at *[39m
*[90m    at *[39m
*[90m    at *[39m

where not successful.`

This is the RegEx I was using:

`
const regex = /(\x1B[\d+m|(.)|<.>|/|\w+:)(?=[\n])/g;
const outputString = inputString.replace(regex, '');

console.log(outputString);
`

I would appreciate the input and help.

Thanks!

test/parallel/test-node-output-errors.mjs

test/parallel/test-node-output-tick.mjs

test/parallel/test-node-output-promise.mjs

test/parallel/test-node-output-map.mjs

test/parallel/test-node-output-errors.mjs

test/parallel/test-node-output-assertion.mjs

test/fixtures/console/core_line_numbers.snapshot

targos · 2023-05-09T06:50:35Z

I don't know what should be the prefix of the test file names, but test-node- doesn't seem right.

projectnoa · 2023-05-09T17:18:24Z

I don't know what should be the prefix of the test file names, but test-node- doesn't seem right.

I totally agree, but I couldn't figure out a clear pattern or methodology for naming all these diverse tests. Open to suggestions. Thanks @targos!

MoLow · 2023-05-10T07:18:47Z

perhaps test-output , or test-message or test-snapshot?

projectnoa · 2023-05-15T21:44:35Z

@MoLow Test runner name prefix changed to test-output.

Let me know of any other observations.

Thanks!

MoLow · 2023-05-16T08:19:44Z

Let me know of any other observations.

see comment: https://github.com/nodejs/node/pull/47868/files#r1187744266
lets keep this PR lean with a minimal diff

MoLow · 2023-05-17T05:29:20Z

test/message/core_line_numbers.out

please revert all deleted files

MoLow · 2023-05-17T05:29:48Z

this PR will need to be squashed / remove all merge commits

projectnoa · 2023-05-18T01:50:41Z

this PR will need to be squashed / remove all merge commits

Apologies @MoLow but I am not versed enough in git to figure out how to squash the commits when there's a merge in between.

Could anybody help me here?

Appreciate the help and I'm sorry for my ignorance.

Thanks!

MoLow · 2023-05-18T07:01:05Z

You would probably want to:

git fetch upstream main
git rebase upstream/main 
git reset --soft upstream/main
git add .
git commit -m "test: migrate message tests to js fixtures"

projectnoa · 2023-05-18T20:08:41Z

You would probably want to:

git fetch upstream main
git rebase upstream/main 
git reset --soft upstream/main
git add .
git commit -m "test: migrate message tests to js fixtures"

@MoLow I ran the commands and merged the changes. I don't believe the squashing of the commits was achieved tho. I'm struggling to figure this out. I'm holding back this PR with my lack of experience with git.

projectnoa · 2023-05-23T18:43:44Z

@MoLow

Should I discard this PR and create a new one with only the changes in one commit?

MoLow · 2023-05-23T18:50:58Z

either that, or squash this PR into a commit with no merge commits, and no conflicts

test: migrate message tests to js fixtures squashed Add test runners to /parallel dir and fix snapshot Update test/parallel/test-node-output-tick.mjs Co-authored-by: Moshe Atlow <moshe@atlow.co.il> Update test/parallel/test-node-output-promise.mjs Co-authored-by: Moshe Atlow <moshe@atlow.co.il> Update test/parallel/test-node-output-assertion.mjs Co-authored-by: Moshe Atlow <moshe@atlow.co.il> Update test/parallel/test-node-output-map.mjs Co-authored-by: Moshe Atlow <moshe@atlow.co.il> fixed snapshot discrepancy with output-errors.mjs Corrected test runner name prefix Removing snapshots that show as 'new' on git diff Restored deleted files Deleted files restored

projectnoa · 2023-05-23T19:27:57Z

either that, or squash this PR into a commit with no merge commits, and no conflicts

Figured out how to properly squash the PR commits. Apologies for the hassle.

test/fixtures/map/source_map_enclosing_function.js

test/fixtures/console/core_line_numbers.js

projectnoa · 2023-05-24T01:19:23Z

@MoLow let me know of any other observations.

Thanks!

MoLow · 2023-05-31T15:30:26Z

test/fixtures/console/core_line_numbers.snapshot

@@ -0,0 +1,10 @@
+node:punycode:*


@projectnoa there are still many files that do not appear as moved

projectnoa · 2023-06-07T20:03:47Z

Hi @MoLow,

Could you please provide some guidance?

I really appreciate your help.

Thanks!

MoLow · 2023-06-07T20:11:43Z

Could you please provide some guidance?

Sure, you should go over the diff of this PR and undo any changes to files that are not marked as moved

projectnoa · 2023-06-07T20:25:45Z

Could you please provide some guidance?

Sure, you should go over the diff of this PR and undo any changes to files that are not marked as moved

@MoLow
Won't this break the tests? Some files will not have the changes that are needed for the new tests to work.

MoLow · 2023-06-07T20:34:33Z

if it brakes tests, lets revert that as well.
I think this PR should have a minimal diff

projectnoa · 2023-06-13T19:54:21Z

if it brakes tests, lets revert that as well.
I think this PR should have a minimal diff

So, if I understand correctly.

All .out files that were moved into fixtures as a .snapshot file must have the original content (I must not regenerate or modify these outputs).
Any .js files that cannot validate this original output must be reverted and left on the messages directory.
All .mjs files that run the tests don't apply to the previous rules since they are new.

Is this correct @MoLow ?

MoLow · 2023-06-15T06:32:37Z

All .out files that were moved into fixtures as a .snapshot file must have the original content (I must not regenerate or modify these outputs).

I don't think having the original content is a must. I only asked the diff to be small enough for git to detect it as a file rename

Any .js files that cannot validate this original output must be reverted and left on the messages directory.

for this phase, yes - then we can iterate in smaller PR's that can be reviewed more carfully

All .mjs files that run the tests don't apply to the previous rules since they are new.

correct. they also do not have to be mjs files, that was just done for convenience

projectnoa · 2023-07-12T04:44:03Z

Hello @MoLow, @NiharPhansalkar,

I did my best to restore the .snapshots to their original state so they show as Edited and not New. Additionally, I restored the original Python tests in the message directory. I was unable to make the following test pass (stdin_messages.js, eval_messages.js, core_line_numbers.js, util-inspect-error-cause.js) since the contents of some of the .out files is just too different to the output produced by the tests. In the end I was just bending over backwards to twist the code to validate the test, which defeats the purpose of tests.

If anybody is smart enough to figure where I went wrong and want to fix this PR, please do so. Otherwise, this is where I end my journey with this contribution, for my sanity's sake.

I apologize for not being able to figure this out and make a proper contribution.

Feel free to discard this PR if the effort of fixing it is not worth it.

Thanks

MoLow · 2023-07-12T05:37:35Z

I don't think we should make the tests reduce the diff. just ship a first PR where all diffs are minimal, excluding tests that are more complicated. then in follow-up PRS - we can handle those tests with a bigger diff

GeoffreyBooth · 2023-07-12T15:15:28Z

test/fixtures/map/source_map_throw_set_immediate.js

-require('../../common');
-require('../source-map/uglify-throw');
+require('../common');
+require('../fixtures/source-map/uglify-throw');


I think in general we want all files to end with a newline.

GeoffreyBooth · 2023-07-12T15:16:04Z

test/fixtures/errors/throw_error_with_getter_throw.snapshot


-Node.js *
+Node.js *


Why are all these files not ending with newlines? Aren’t they autogenerated?

GeoffreyBooth · 2023-07-12T15:17:04Z

test/fixtures/console/eval_messages.snapshot

+    at new Script (node:vm:*:*)
+    at createScript (node:vm:*:*)
+    at Object.runInThisContext (node:vm:*:*)
+    at node:internal/process/execution:*:*
+    at [eval]-wrapper:*:*
+    at runScript (node:internal/process/execution:*:*)
+    at evalScript (node:internal/process/execution:*:*)
+    at node:internal/main/eval_string:*:*


This stack trace is a step backward; this test will break if this call path ever changes, which isn’t something that should be breaking a test. Can we eliminate this from the snapshot, perhaps by updating the snapshot generation script if necessary?

nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem. labels May 5, 2023

projectnoa mentioned this pull request May 5, 2023

message tests can be migrated from python to JS #47707

Open

GeoffreyBooth added the test Issues and PRs related to the tests. label May 5, 2023

MoLow reviewed May 5, 2023

View reviewed changes

deps/zlib/zconf.h.included Outdated Show resolved Hide resolved

MoLow requested changes May 5, 2023

View reviewed changes

projectnoa changed the title ~~#47707 migrated test/message to JS fixture tests~~ test: migrated test/message to JS fixture tests May 5, 2023

projectnoa commented May 6, 2023

View reviewed changes

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

projectnoa requested a review from MoLow May 6, 2023 00:34