message tests can be migrated from python to JS

[MoLow]

See #47498 for an example of such a migration
pseudo-tty tests can migrate as well, but that will require some more work using something like https://github.com/microsoft/node-pty or similar

[MoLow]

@RafaelGSS is this a good candidate for Grace Hopper Open Source Day?

[projectnoa]

I would like to work on this if is available.

[MoLow]

@projectnoa no need to ask, this is an issue that many people can work on since there are many tests under tests/message.

[GeoffreyBooth]

@projectnoa no need to ask, this is an issue that many people can work on since there are many tests under tests/message.

Yes. Please follow the example of #47498, and branch off of that PR’s branch so you get the new helpers that it adds. You might also want to add a comment on this thread letting others know which tests you plan to migrate, so that no one else works on the same ones while you do, duplicating effort.

[projectnoa]

Great! I'll dig into the project and mention what I'll tackle tomorrow.

[projectnoa]

Tests I will be working on:
assert_throws_stack.js,
assert_throws_stack.out,
core_line_numbers.js,
core_line_numbers.out,
eval_messages.js,
eval_messages.out,
if-error-has-good-stack.js,
if-error-has-good-stack.out,
internal_assert.js,
internal_assert.out,
internal_assert_fail.js,
internal_assert_fail.out,
max_tick_depth.js,
max_tick_depth.out,
message.status,
nexttick_throw.js,
nexttick_throw.out,
promise_unhandled_warn_with_error.js,
promise_unhandled_warn_with_error.out,
source_map_enclosing_function.js,
source_map_enclosing_function.out,
source_map_reference_error_tabs.js,
source_map_reference_error_tabs.out,
source_map_sourcemapping_url_string.js,
source_map_sourcemapping_url_string.out,
source_map_throw_catch.js,
source_map_throw_catch.out,
source_map_throw_icu.js,
source_map_throw_icu.out,
source_map_throw_set_immediate.js,
source_map_throw_set_immediate.out,
stdin_messages.js,
stdin_messages.out,
test-no-extra-info-on-fatal-exception.js,
test-no-extra-info-on-fatal-exception.out,
testcfg.py

[MoLow]

@projectnoa we try to avoid cookie licking, so if you have a ready PR just open one or multiple PRs

[mertcanaltin]

@MoLow I would love to help too

[MoLow]

@mertcanaltin go for it!

[projectnoa]

Apologies @MoLow, you were right. I didn't do my due diligence and jumped into the task without understanding the challenge ahead. I still want to do the work and contribute but I need to ask some questions.

To my understanding, there's a set of unit tests in Python that need to be migrated to JS. I'm having trouble finding the Python tests. Can someone point me in the right direction?

These tests will be residing in the test/message directory, correct? Are the message tests targeting a specific module or feature?

What are the .out files in this directory?

Also, the tests files I announced that I would work on are not the target of this operation. I should have paid attention a notice that they were tests already migrated. Apologies about that. I will instead focus on understanding the test workflow and see how I can start migrating a few tests.

I was able to compile and run all current tests successfully, so I'm ready to work as soon as I can understand the test better.

Thanks!

[GeoffreyBooth]

@projectnoa Once you’ve checked out the node repo and built, run

tools/test.py -J --mode=release message

To run all the message tests. At the moment, now that #47498 has landed there are only 26 left.

These aren’t tests written in Python; they’re tests that are run by the Python script tools/test.py, but in a special way different from all the other tests in the Node codebase. The Python code runs all the .js files within test/message and compares their outputs against the matching .out files in the same folder, using a complicated regex pattern. Whenever the regex comparison fails, the test fails.

We don’t want to have all this logic in Python, so we’ve been migrating the test/message tests to use a new pattern where the regex comparison is done in JavaScript; the #47498 PR created a new helper to do that comparison, and migrated a few dozen tests to use it. The work that remains is to migrate the rest of the message tests, following the example of #47498.

[projectnoa]

Pull request created. Apologies for the delay.

#47868

I appreciate your input and observations.

[NiharPhansalkar]

Hello, can I try working on this issue if it is available? I am very new to Open Source, so I would like to learn via this good-first-issue.

[NiharPhansalkar]

Hello @MoLow, thank you for responding. I have gone through the messages above and also the issue you have mentioned (#47498), please guide me and let me know if my understanding of the issue is correct?

According to my understanding, you have written the test/common/assertSnapshot.js file (a helper), and now, I have to change files within test/message to use the functions written within the file instead of what they are currently using? (And most of these files in test/message have been changed, now only 26 remain?)

[MoLow]

@NiharPhansalkar, you are missing one detail, but you understood the essence of the change.
previous PRS migrating these tests moved the tests within test/message into test/fixtures and created new tests within test/parallel to run these fixtures using assertSnapshot

[NiharPhansalkar]

@MoLow Took a look at the PRs in #47498. Correct me if I am wrong.

We will write .snapshot files in the test/fixtures and write tests in test/parallel that when fail, will display these .snapshots?

[MoLow]

the snapshots are written by assertSnapshot when NODE_REGENERATE_SNAPSHOTS=1

[NiharPhansalkar]

@MoLow I am sorry if I am asking too many questions. From all the PRs I can see, I can see nothing written by anyone in the test/fixtures directory. Only new snapshots are created.
So, I should write tests within test/parallel (replacing those in test/message) which will utilise the helper and create snapshots inside test/fixtures? (snapshots will be automatically created by the assertSnapshot helper)

[MoLow]

it's ok :)
see #47498 as an example - the fixtures directory contains both snapshots and js files

[NiharPhansalkar]

@MoLow Thank you, will take a look into it!

[NiharPhansalkar]

@MoLow Can I start with the following? (I don't want to start doing something wrong)

I will move the entire .js files from test/message into test/fixtures by creating an aptly named directory. Then make changes in the .js files (eg require changes).

After this step is complete, I will then write a test in test/parallel (eg. a test for assertion type errors).

Am I correct? (Btw, I have not elaborated on the test/parallel part because I still don't quite understand how I have to write a test. I will surely read up the PRs and try to figure that out once I am done with part 1 of moving the files.)

[projectnoa]

Hello @MoLow, @NiharPhansalkar,

I'm still willing and working on completing this contribution. Can we collaborate to close it together?

[NiharPhansalkar]

@projectnoa Sure! I would love to collaborate on it!
Any way we can keep in touch and coordinate?

[projectnoa]

@projectnoa Sure! I would love to collaborate on it!
Any way we can keep in touch and coordinate?

Sure, I'll email you some options to connect.

[jahjahLemonade]

@MoLow
I submitted a PR

util/
	util-inspect-error-cause.js
	util-inspect-error-cause.out
	util_inspect_error.js
	util_inspect_error.out

• test: migrate message util tests from Python to JS #49721 #50333

[jahjahLemonade]

@MoLow @GeoffreyBooth
I'll take on this as well

v8/
	v8_warning.js
	v8_warning.out

[yiyunlei]

I will finish my part recently.

eval/
	eval_messages.js
	eval_messages.out
	stdin_messages.js
	stdin_messages.out

[jahjahLemonade]

I submitted a new PR. I'm still working on correcting my previous PR for util. I should have it done shortly.

v8/
	v8_warning.js
	v8_warning.out

• test: migrate message v8 tests from Python to JS #50421

[DylanTet]

Are there still tests that need to be migrated?

[GeoffreyBooth]

Are there still tests that need to be migrated?

Probably. Look for any .out files still in the repo that don’t have open PRs for them.

[DylanTet]

Awesome thank you @GeoffreyBooth!

[yiyunlei]

I will finish my part recently.

eval/
	eval_messages.js
	eval_messages.out
	stdin_messages.js
	stdin_messages.out

PR

• test: migrate message eval tests from Python to JS #50482