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.

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

test: replace third argument in assert.strictEqual #19162

Closed
wants to merge 2 commits into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Mar 6, 2018

Replace the third argument in assert.strictEqual with something much
more informative so that it display relevant information when the
test fails

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Replace the third argument in assert.strictEqual with something much
more informative so that it display relevant information when the
test fails
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 6, 2018
@ryzokuken
Copy link
Contributor Author

@Trott here it is! 😄

@Trott
Copy link
Member

Trott commented Mar 6, 2018

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

This would be better with no message at all which will then show a comparison of the two values.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

But if we do put a message in, it should be similar to the one above it and mention at what index the error occurred and for what hash. At least that's information that's not provided by the default strictEqual message.

@ryzokuken
Copy link
Contributor Author

@apapirovski is it better now? Let me know if anything is amiss.

@apapirovski
Copy link
Member

Lite CI for the change: https://ci.nodejs.org/job/node-test-pull-request-lite/224/

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM but I agree with @apapirovski. Almost all strictEqual assertions are better without the message argument.

@ryzokuken
Copy link
Contributor Author

@lpinca @apapirovski so, should I remove it altogether? As it is, it does give us some extra information (hash and index)

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

In this case it is probably fine to have the extra message. Otherwise it is difficult to see what test case just ran.

@apapirovski
Copy link
Member

I'm fine with it either way. It offers a bit more info since it provides the hash and index, which would make debugging a tiny bit easier. Admittedly that can also be gleaned from the comparison value but it's a bit less convenient.

jasnell pushed a commit that referenced this pull request Mar 6, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 6, 2018

Landed in 78a7536

@jasnell jasnell closed this Mar 6, 2018
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
PR-URL: nodejs#19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Backport-PR-URL: #22380
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants