Make test match others in repository#15935
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added dont-land-on-v4.x node-api 
Trott added the code-and-learn 
| consttheDataview=test_dataview.CreateDataView(template); | ||
| assert.ok(theDataviewinstanceofDataView, | ||
| 'The new variable should be of type Dataview'); | ||
| assert.ok(theDataviewinstanceofDataView); |
There was a problem hiding this comment.
I think it would be best to include the actual instanceof value in the error message here instead of just removing the message.
There was a problem hiding this comment.
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 commented Oct 13, 2017
| consttheDataview=test_dataview.CreateDataView(template); | ||
| assert.ok(theDataviewinstanceofDataView, | ||
| 'The new variable should be of type Dataview'); | ||
| assert.ok(theDataviewinstanceofDataView); |
There was a problem hiding this comment.
BridgeAR commented Oct 18, 2017
Ping @mattcan |
mattcan commented Oct 19, 2017
My apologies, I'll wrap this up over the weekend. |
| consttheDataview=test_dataview.CreateDataView(template); | ||
| assert.ok(theDataviewinstanceofDataView, | ||
| 'The new variable should be of type Dataview'); | ||
| assert.ok(`Expect ${theDataview} to be a DataView`); |
There was a problem hiding this comment.
This is no longer correct as it is always truthy. Can you please add back the first argument? theDataview instanceof DataView.
There was a problem hiding this comment.
Yep, sorry. Shouldn't do things while asleep.
There was a problem hiding this comment.
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 commented Oct 25, 2017
Trott commented Oct 25, 2017
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 |
Trott commented Oct 27, 2017
tniessen commented Oct 28, 2017
PTAL, all approvals reviewed the first commit. |
PR-URL: #15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
tniessen commented Oct 29, 2017
Landed in 201ecef, thank you for your contribution! 🎉 |
PR-URL: #15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Backport-PR-URL: #19447 PR-URL: #15935 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
10 participants
Working on our first commit in Node. The task was to review
assert.okand 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), orvcbuild test(Windows) passesAffected core subsystem(s)
dataview