test: changed test2 & test3 of test-vm-timeout.js

[aniketshukla]

Changed test2 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 2 of test-vm-timeout.js would match any RangeError,
now it looks specifically for the error message
"RangeError: timeout must be a positive number"

Changed test 3 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 3 of test-vm-timeout.js would match any RangeError,
now it looks specifically for the error message
"RangeError: timeout must be a positive number"

Ref: #13454

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test,vm

[vsemozhetbyt]

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

[refack]

@aniketshukla Welcome, and thank you very much for the contribution 🥇
I've "gently" edited you original comment to make it more conformant with our template (you actually have supplied a test, and the affected subsystems are: test,vm)

[vsemozhetbyt]

Тhank you!

[aniketshukla]

@refack @vsemozhetbyt Thank you !

[tniessen]

Please choose a more specific commit message. The first line should start with test,vm: and say something useful, e.g. test,vm: restrict allowed RangeErrors for timeout. Additionally, the next paragraphs can be shortened significantly.

[addaleax]

The first line should start with test,vm:

That’s really not customary for test-only changes; I think we mostly do that kind of format for changes that are test-heavy but also have changes in lib/ or src/. I think the commit message is fine as it is.

[refack]

test,vm: restrict allowed RangeErrors for timeout

I would suggest test: validate full error messages
INHO you can remove everything else. We'll add the (new) reference to #13454 that supplies the motivation, and the filename is automatically added by git to the full commit metadata.

[tniessen]

@addaleax Okay, my bad, I thought we included all affected subsystems in the commit message. IMHO "changed test2 & test3 of test-vm-timeout.js" is not a helpful commit message.

[refack]

Okay, my bad, I interpreted, I thought we included all affected subsystems in the commit message.

INHO It's good you voice your opinion!
Anyway it's a judgment call, in my POV test,vm would be better when a new test is added that tests vm. IMHO this change is more of an improvement to the test suite 🤔

IMHO "changed test2 & test3 of test-vm-timeout.js" is not a helpful commit message.
👍

[refack]

@tniessen personally I give more "assertive" comment to @nodejs/collaborators members then to other users (IMHO @addaleax does too), it doesn't mean we are always right, and it's usually not meant as criticism (unless explicitly written so), and is never ever personal! You should take it as pointer to (undocumented) more common practices.

[tniessen]

@refack It's okay, no offense taken :) I see that including vm in the commit message is not necessary, it might actually be confusing as no actual changes were made to the vm subsystem, but I still hold up my opinion about the commit message. IMO the first line should at least attempt to explain or summarize what was done :)

If we agree on a different commit message, I can change it while merging this (unless someone else plans to do that). I am fine with @refack's suggestion (test: validate full error messages) considering your explanation.

[refack]

Failure on test/windows-fanned is unrelated. Maybe a new flake.

[refack]

arm: https://ci.nodejs.org/job/node-test-commit-arm-fanned/9199/
windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/9568/

[refack]

Landed in 12e39d6

[refack]

Extra post-land sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6445/