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 third argument(string literal) from strictEqual() #20920

Conversation

AbhimanyuVashisht
Copy link
Contributor

@AbhimanyuVashisht AbhimanyuVashisht commented May 24, 2018

In test/parallel/test-stream-pip-await-drain-push-while-write.js, There are two
calls in it to assert.strictEqual(). Both calls are provided with three
arguments. The third argument in each case is a string literal. Unfortunately,
that will suppress the presentation of the values of the first two arguments in
the case of an AssertionError

This is fixed by removing the third argument from the call. The third argument
in the call is basically a string literal.

This particular issue is provided by @Trott, Thank you so much for getting me started.

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-stream-pip-await-drain-push-while-write.js, There are two
calls in it to assert.strictEqual(). Both calls are provided with three
arguments. The third argument in each case is a string literal. Unfortunately,
that will suppress the presentation of the values of the first two arguments in
the case of an AssertionError

This is fixed by removing the third argument from the call. The third argument
in the call is basically a string literal.

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 24, 2018
Copy link
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

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

LGTM, with a minor nit. 🎉

@@ -7,17 +7,15 @@ const writable = new stream.Writable({
write: common.mustCall(function(chunk, encoding, cb) {
assert.strictEqual(
readable._readableState.awaitDrain,
0,
'State variable awaitDrain is not correct.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a good idea to preserve this message in a comment above the call to assert.strictEqual() — something like this // State variable awaitDrain should be 0?

cc: @Trott

Copy link
Member

Choose a reason for hiding this comment

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

A static value is always bad. Someone will look at the line anyway, so the context will be clear.

Copy link
Member

Choose a reason for hiding this comment

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

@sagirk I don't think it's needed in this case. A comment that says "awaitDrain should be 0" is superfluous above await.strictEqual(awaitDrain, 0).

Copy link
Contributor

Choose a reason for hiding this comment

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

@BridgeAR @Trott Makes sense! Thanks! 👍

@BridgeAR
Copy link
Member

@AbhimanyuVashisht
Copy link
Contributor Author

@BridgeAR why these following errors are coming up?

@BridgeAR
Copy link
Member

@AbhimanyuVashisht our test suite has flaky tests. Just ignore that.

@BridgeAR BridgeAR added 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. labels May 25, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 25, 2018
Remove obsolete string literals from `assert.strictEqual()` calls in
test/parallel/test-stream-pip-await-drain-push-while-write.js.

PR-URL: nodejs#20920
Refs: https://www.nodetodo.org/getting-started
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR
Copy link
Member

Landed in 4970cd6 🎉

I fixed the commit message while landing to adhere to the 72 character rule as described in our guidelines and minimized the text somewhat.

@AbhimanyuVashisht 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
Remove obsolete string literals from `assert.strictEqual()` calls in
test/parallel/test-stream-pip-await-drain-push-while-write.js.

PR-URL: #20920
Refs: https://www.nodetodo.org/getting-started
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
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.

8 participants