Make test match others in repository #15935

mattcan · 2017-10-06T17:34:45Z

Working on our first commit in Node. The task was to review assert.ok and determine if the string literal made sense or not. After reviewing other tests in nearby directories, we determined that removing the message would better match the repository.

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)

dataview

BridgeAR · 2017-10-07T21:50:37Z

test/addons-napi/test_dataview/test.js

@@ -10,5 +10,4 @@ const buffer = new ArrayBuffer(128);
 const template = Reflect.construct(DataView, [buffer]);

 const theDataview = test_dataview.CreateDataView(template);
-assert.ok(theDataview instanceof DataView,
-          'The new variable should be of type Dataview');
+assert.ok(theDataview instanceof DataView);


I think it would be best to include the actual instanceof value in the error message here instead of just removing the message.

The value of theDataView instanceof DataView will reliably be true or false and thus false if this fails. What might be useful is displaying the value of theDataView so it can be obvious why that failed. If theDataView is undefined rather than some kind of complex object, that's useful info.

jasnell · 2017-10-13T19:40:02Z

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

joyeecheung · 2017-10-14T13:59:15Z

test/addons-napi/test_dataview/test.js

@@ -10,5 +10,4 @@ const buffer = new ArrayBuffer(128);
 const template = Reflect.construct(DataView, [buffer]);

 const theDataview = test_dataview.CreateDataView(template);
-assert.ok(theDataview instanceof DataView,
-          'The new variable should be of type Dataview');
+assert.ok(theDataview instanceof DataView);


Can you change the message to

`Expect ${theDataview} to be a Dataview`

like what @Trott suggested?

BridgeAR · 2017-10-18T19:54:48Z

Ping @mattcan

mattcan · 2017-10-19T18:19:47Z

My apologies, I'll wrap this up over the weekend.

lpinca · 2017-10-25T06:40:16Z

test/addons-napi/test_dataview/test.js

@@ -10,5 +10,4 @@ const buffer = new ArrayBuffer(128);
 const template = Reflect.construct(DataView, [buffer]);

 const theDataview = test_dataview.CreateDataView(template);
-assert.ok(theDataview instanceof DataView,
-          'The new variable should be of type Dataview');
+assert.ok(`Expect ${theDataview} to be a DataView`);


This is no longer correct as it is always truthy. Can you please add back the first argument? theDataview instanceof DataView.

Yep, sorry. Shouldn't do things while asleep.

No problem. Now I think the line is too long and our linter isn't happy with it. Can you please move the second argument to the next as it was originally?

Thanks!

Trott · 2017-10-25T20:44:34Z

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

Trott · 2017-10-25T21:24:23Z

ESLint is reporting that the changed line exceeds 80 characters. Can you split it into two lines like it was before (but leave your change, of course)? Run make lint-js (or vcbuild lint-js if on Windows) to see if there are any lint errors.

Trott

Lint error needs fixing.

lint error fixed

Trott · 2017-10-27T16:58:32Z

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

tniessen · 2017-10-28T20:19:11Z

PTAL, all approvals reviewed the first commit.

PR-URL: #15935 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

tniessen · 2017-10-29T09:08:45Z

Landed in 201ecef, thank you for your contribution! 🎉

PR-URL: #15935 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs/node#15935 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs#15935 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Backport-PR-URL: #19447 PR-URL: #15935 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

first-pr: make test match other tests

6eacabc

nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

cjihrig approved these changes Oct 7, 2017

View reviewed changes

BridgeAR reviewed Oct 7, 2017

View reviewed changes

jasnell approved these changes Oct 10, 2017

View reviewed changes

lpinca approved these changes Oct 13, 2017

View reviewed changes

joyeecheung reviewed Oct 14, 2017

View reviewed changes

Updated to clearer message

8313444

lpinca reviewed Oct 25, 2017

View reviewed changes

Actually test something

00c8b9b

Trott previously requested changes Oct 25, 2017

View reviewed changes

Fix linting error in test

a01a962

joyeecheung approved these changes Oct 29, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 29, 2017

View reviewed changes

tniessen closed this Oct 29, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make test match others in repository #15935

Make test match others in repository #15935

mattcan commented Oct 6, 2017

BridgeAR Oct 7, 2017

Trott Oct 12, 2017 •

edited

jasnell commented Oct 13, 2017

joyeecheung Oct 14, 2017

BridgeAR commented Oct 18, 2017

mattcan commented Oct 19, 2017

lpinca Oct 25, 2017

mattcan Oct 25, 2017

lpinca Oct 25, 2017

Trott commented Oct 25, 2017

Trott commented Oct 25, 2017

Trott left a comment

Trott commented Oct 27, 2017

tniessen commented Oct 28, 2017

tniessen commented Oct 29, 2017

Make test match others in repository #15935

Make test match others in repository #15935

Conversation

mattcan commented Oct 6, 2017

Checklist

Affected core subsystem(s)

BridgeAR Oct 7, 2017

Choose a reason for hiding this comment

Trott Oct 12, 2017 • edited

Choose a reason for hiding this comment

jasnell commented Oct 13, 2017

joyeecheung Oct 14, 2017

Choose a reason for hiding this comment

BridgeAR commented Oct 18, 2017

mattcan commented Oct 19, 2017

lpinca Oct 25, 2017

Choose a reason for hiding this comment

mattcan Oct 25, 2017

Choose a reason for hiding this comment

lpinca Oct 25, 2017

Choose a reason for hiding this comment

Trott commented Oct 25, 2017

Trott commented Oct 25, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott commented Oct 27, 2017

tniessen commented Oct 28, 2017

tniessen commented Oct 29, 2017

Trott Oct 12, 2017 •

edited