test: changed test2 & test3 of test-vm-timeout.js by aniketshukla · Pull Request #13453 · nodejs/node

aniketshukla · 2017-06-04T16:33:59Z

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

test: 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" test: 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"

vsemozhetbyt · 2017-06-04T16:41:00Z

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

refack · 2017-06-04T16:57:05Z

@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 · 2017-06-04T17:04:44Z

@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 · 2017-06-04T17:07:07Z

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 · 2017-06-04T17:11:03Z

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 · 2017-06-04T17:11:17Z

@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 · 2017-06-04T17:14:36Z

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 · 2017-06-04T17:20:17Z

@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 · 2017-06-04T17:33:35Z

@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 · 2017-06-04T17:36:35Z

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

refack · 2017-06-04T18:42:49Z

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

test: 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" test: 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" PR-URL: nodejs#13453 Refs: nodejs#13454 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

refack · 2017-06-07T12:57:03Z

Landed in 12e39d6

refack · 2017-06-07T12:57:52Z

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

test: 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" test: 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" PR-URL: #13453 Refs: #13454 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

…sAddon Backend nodejs#13453 added the RecordReplayMarkNextDlopenAsAddon() export which this branch calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Tell the driver which dlopen() loads an addon The driver only let addons be loaded from paths containing node_modules when recording, and refused others without a dlerror() message, which crashed DLib::Open() while building its error. Call the driver's RecordReplayMarkNextDlopenAsAddon() before loading an addon, so that it is attached wherever it is, and fall back to a generic message when dlerror() has none. Requires a driver with RecordReplayMarkNextDlopenAsAddon(), so REPLAY_BACKEND_REV needs to be bumped once it is released. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pass the addon's name when marking its dlopen() for the driver The driver only applies the mark to a dlopen() of that name, so a mark whose dlopen() wasn't intercepted can't apply to a later library. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pin REPLAY_BACKEND_REV to the driver with RecordReplayMarkNextDlopenAsAddon Backend nodejs#13453 added the RecordReplayMarkNextDlopenAsAddon() export which this branch calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Drop the fallback for a NULL dlerror() in DLib::Open Addon dlopen() calls are marked for the driver, so it doesn't refuse them, and the driver makes dlerror() report refused ones anyway. A failed dlopen() always has an error, as upstream assumes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Jun 4, 2017

vsemozhetbyt added the vm Issues and PRs related to the vm subsystem. label Jun 4, 2017

refack approved these changes Jun 4, 2017

View reviewed changes

vsemozhetbyt approved these changes Jun 4, 2017

View reviewed changes

refack mentioned this pull request Jun 4, 2017

reference issue: assert whole error messages #13454

Closed

1 task done

tniessen reviewed Jun 4, 2017

View reviewed changes

addaleax approved these changes Jun 4, 2017

View reviewed changes

refack self-assigned this Jun 4, 2017

cjihrig approved these changes Jun 4, 2017

View reviewed changes

lpinca approved these changes Jun 5, 2017

View reviewed changes

hiroppy approved these changes Jun 5, 2017

View reviewed changes

refack closed this Jun 7, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

2 of 3 tasks

MylesBorins added the land-on-v6.x label Jul 17, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

refack removed their assignment Oct 20, 2018

Conversation

aniketshukla commented Jun 4, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

aniketshukla commented Jun 4, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

tniessen commented Jun 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

tniessen commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 7, 2017

Uh oh!

refack commented Jun 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

aniketshukla commented Jun 4, 2017 •

edited by refack

Loading

tniessen commented Jun 4, 2017 •

edited

Loading