Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make test match others in repository #15935

Closed
wants to merge 4 commits into from
Closed

Make test match others in repository #15935

wants to merge 4 commits into from

Conversation

mattcan
Copy link
Contributor

@mattcan mattcan commented Oct 6, 2017

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

@nodejs-github-bot 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 Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
BridgeAR
BridgeAR reviewed Oct 7, 2017
View reviewed changes
test/addons-napi/test_dataview/test.js Outdated
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Trott Trott Oct 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jasnell commented Oct 13, 2017

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

joyeecheung
joyeecheung reviewed Oct 14, 2017
View reviewed changes
test/addons-napi/test_dataview/test.js Outdated
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the message to

`Expect ${theDataview} to be a Dataview`

like what @Trott suggested?

@BridgeAR
Copy link
Member

Ping @mattcan

@mattcan
Copy link
Contributor Author

mattcan commented Oct 19, 2017

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

lpinca
lpinca reviewed Oct 25, 2017
View reviewed changes
test/addons-napi/test_dataview/test.js Outdated
@@ -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`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Trott commented Oct 25, 2017

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

@Trott
Copy link
Member

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 make lint-js (or vcbuild lint-js if on Windows) to see if there are any lint errors.

Trott
Trott previously requested changes Oct 25, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint error needs fixing.

@Trott Trott dismissed their stale review October 27, 2017 16:57

lint error fixed

@Trott
Copy link
Member

Trott commented Oct 27, 2017

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

@tniessen
Copy link
Member

PTAL, all approvals reviewed the first commit.

tniessen pushed a commit that referenced this pull request Oct 29, 2017
@mattcan @tniessen
test: include actual value in assertion message
201ecef 
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
Copy link
Member

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

@tniessen tniessen closed this Oct 29, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@mattcan @gibfahn
test: include actual value in assertion message
f7bd833 
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]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@mattcan @gibfahn
test: include actual value in assertion message
34a1e0c 
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]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@mattcan @gibfahn
test: include actual value in assertion message
bac2aff 
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]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@mattcan @Qard
test: include actual value in assertion message
bdc76de 
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]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@mattcan @Qard
test: include actual value in assertion message
cc748f3 
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]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@mattcan @addaleax
test: include actual value in assertion message
2a8bdfa 
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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@mattcan
test: include actual value in assertion message
6674c17 
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mattcan @MylesBorins
test: include actual value in assertion message
087d213 
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]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@lpinca lpinca lpinca approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@mattcan @jasnell @BridgeAR @Trott @tniessen @lpinca @cjihrig @joyeecheung @gireeshpunathil @nodejs-github-bot