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: change assert message to default #18259

Closed
wants to merge 2 commits into from
Closed

test: change assert message to default #18259

wants to merge 2 commits into from

Conversation

ryanmahan
Copy link
Contributor

assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

Refs: #13296

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@ryanmahan
test: change assert message to default
45a938a 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

Refs: nodejs#13296
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 19, 2018
lpinca
lpinca approved these changes Jan 19, 2018
View reviewed changes
test/known_issues/test-http-path-contains-unicode.js Outdated
assert.strictEqual(
expected,
'/caf\u{e9}\u{1f436}',
'Sanity check that string literal produced the expected string'
'/caf\u{e9}\u{1f436}'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please swap the arguments (actual first expected second) and put everything on the same line now that it doesn't take more than 80 chars? Thank you.

@ryanmahan
Corrections for PR comments
7d6a2f6 
Condensed line 14 assert to one line
@maclover7
Copy link
Contributor

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

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2018
@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 21, 2018
BridgeAR
BridgeAR reviewed Jan 21, 2018
View reviewed changes
test/known_issues/test-http-path-contains-unicode.js
'/caf\u{e9}\u{1f436}',
'Sanity check that string literal produced the expected string'
);
//Sanity check for café🐶 is café🐶
Copy link
Member

Choose a reason for hiding this comment

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

The comment is actually not even necessary. It will be printed in case of a error and the expected value is visible as well.

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 23, 2018
@ryanmahan @Trott
test: change assert message to default
f5d9169 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

PR-URL: nodejs#18259
Refs: nodejs#13296
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Jan 23, 2018

Landed in f5d9169.
Thanks for the contribution! 🎉

@Trott Trott closed this Jan 23, 2018
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@ryanmahan @evanlucas
test: change assert message to default
5855a57 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

PR-URL: #18259
Refs: #13296
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@ryanmahan @MylesBorins
test: change assert message to default
d629be2 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

PR-URL: #18259
Refs: #13296
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@ryanmahan @MylesBorins
test: change assert message to default
bb2d292 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

PR-URL: #18259
Refs: #13296
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Feb 27, 2018
Closed
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ryanmahan @MayaLekova
test: change assert message to default
80c2e33 
assert.strictEqual message argument removed to replace
with default assert message to show the expected vs
actual values

PR-URL: nodejs#18259
Refs: nodejs#13296
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Leko Leko Leko approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. 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
@ryanmahan @maclover7 @Trott @jasnell @Leko @lpinca @cjihrig @BridgeAR @MylesBorins @nodejs-github-bot