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

test: removed literal from assert #16805

Closed
wants to merge 3 commits into from
Closed

test: removed literal from assert #16805

wants to merge 3 commits into from

Conversation

awegrzyn
Copy link
Contributor

@awegrzyn awegrzyn commented Nov 6, 2017

Removed literal from assert to display default message.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@awegrzyn awegrzyn mentioned this pull request Nov 6, 2017
Closed
3 tasks
@apapirovski apapirovski added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 6, 2017

I'm not sure that removing this allows for a more helpful error message.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Nov 6, 2017
@Trott
Copy link
Member

Trott commented Nov 6, 2017

I'm not sure that removing this allows for a more helpful error message.

@mscdex How about we restore the error message but also include c.value in the message? (I probably suggested they simply remove the message. Whoops, I guess.)

@awegrzyn Can you change it to something like this?:

assert.strictEqual(
  c.value,
  42,
  `require(".") should honor NODE_PATH; expected 42, found ${c.value}`
);

gireeshpunathil
gireeshpunathil reviewed Nov 7, 2017
View reviewed changes
test/parallel/test-require-dot.js Outdated
@@ -15,4 +15,4 @@ m._initPaths();

const c = require('.');

assert.strictEqual(c.value, 42, 'require(".") should honor NODE_PATH');
assert.strictEqual(c.value, 42, `require(".") should honor NODE_PATH; expected 42, found ${c.value}`);
Copy link
Member

Choose a reason for hiding this comment

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

this line will be well beyond 80 chars. Can you please format it ? hint: follow @Trott 's suggestion

mhdawson
mhdawson approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Nov 8, 2017

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

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2017
@awegrzyn @Trott
test: improve assertion in test-require-dot
d520059 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: nodejs#16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 8, 2017

Landed in d520059.
Thanks for the contribution! 🎉

@Trott Trott closed this Nov 8, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
@awegrzyn @evanlucas
test: improve assertion in test-require-dot
271c89e 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: #16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
@awegrzyn @MylesBorins
test: improve assertion in test-require-dot
7c9aee3 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: #16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
@awegrzyn @MylesBorins
test: improve assertion in test-require-dot
6bc0b3e 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: #16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@awegrzyn @MylesBorins
test: improve assertion in test-require-dot
ba25b8d 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: #16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@awegrzyn @MylesBorins
test: improve assertion in test-require-dot
714eb0b 
Include the value that differed from the expected value in an assertion
message in test-require-dot.

PR-URL: #16805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. module Issues and PRs related to the module subsystem. 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
@awegrzyn @mscdex @Trott @jasnell @cjihrig @gireeshpunathil @mhdawson @apapirovski @MylesBorins @nodejs-github-bot