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: remove message argument from strictEqual() #20912

Closed
wants to merge 1 commit into from

Conversation

sagirk
Copy link
Contributor

@sagirk sagirk commented May 23, 2018

In test/parallel/test-require-process.js, the last thing in the test is
a call to assert.strictEqual(). It has a string literal as its third
argument. Unfortunately, that means that the diff between the two values
being compared will be suppressed if there is an AssertionError. That's
not helpful for debugging.

This is fixed by removing the third argument from the call. It is,
however, preserved in a comment above the call to assert.strictEqual().

This issue was provided by @Trott of nodetodo.org. Gratitude! 🙏

Fixes: #20911
Refs: https://www.nodetodo.org/getting-started

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

In test/parallel/test-require-process.js, the last thing in the test is
a call to assert.strictEqual(). It has a string literal as its third
argument. Unfortunately, that means that the diff between the two values
being compared will be suppressed if there is an AssertionError. That's
not helpful for debugging.

This is fixed by removing the third argument from the call. It is,
however, preserved in a comment above the call to assert.strictEqual().

Fixes: nodejs#20911
Refs: https://www.nodetodo.org/getting-started
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 23, 2018
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thank you for doing this and your contribution and welcome to Node.js

Changes look good to me.

@benjamingr
Copy link
Member

Collaborators please +1 this comment to support fast-tracking this pull request.

@benjamingr
Copy link
Member

@Trott Trott added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 23, 2018
@Trott
Copy link
Member

Trott commented May 23, 2018

Three CI failures, but probably unrelated. Let's get some re-runs to see.

First up, OS X: https://ci.nodejs.org/job/node-test-commit-osx/18802/

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for your contribution! 🎉

@Trott
Copy link
Member

Trott commented May 23, 2018

OS X was green.

Windows and Linux CI are currently disasters so let's wait until that gets all straightened out before re-running those...

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 25, 2018
In test/parallel/test-require-process.js, the last thing in the test is
a call to assert.strictEqual(). It has a string literal as its third
argument. Unfortunately, that means that the diff between the two values
being compared will be suppressed if there is an AssertionError. That's
not helpful for debugging.

This is fixed by removing the third argument from the call. It is,
however, preserved in a comment above the call to assert.strictEqual().

PR-URL: nodejs#20912
Fixes: nodejs#20911
Refs: https://www.nodetodo.org/getting-started
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in 8495452 🎉

@sagirk congratulations on your first commit to Node.js! 🎉

@BridgeAR BridgeAR closed this May 25, 2018
targos pushed a commit that referenced this pull request May 25, 2018
In test/parallel/test-require-process.js, the last thing in the test is
a call to assert.strictEqual(). It has a string literal as its third
argument. Unfortunately, that means that the diff between the two values
being compared will be suppressed if there is an AssertionError. That's
not helpful for debugging.

This is fixed by removing the third argument from the call. It is,
however, preserved in a comment above the call to assert.strictEqual().

PR-URL: #20912
Fixes: #20911
Refs: https://www.nodetodo.org/getting-started
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
@sagirk sagirk deleted the fix/sk/test-require-process branch April 5, 2019 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Remove message parameter from strictEqual()
9 participants