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: fix assert.strictEqual() parameter order #23564

Closed
wants to merge 1 commit into from

Conversation

@lotharthesavior
Copy link
Contributor

lotharthesavior commented Oct 12, 2018

…follow the order actual-value->expected-value.

According to the documentation, the strictEqual assertions should follow the order actual-value -> expected-value. This PR is changing the current situation of the test parallel/test-tcp-wrap-connect.js to accomplish it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
…eters follow the order actual-value->expected-value.

Refs: test/parallel/test-tcp-wrap-connect.js
@refack
refack approved these changes Oct 12, 2018
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 12, 2018

Hi @lotharthesavior – just as a heads up, our CI does not deal well with merge commits. Do you think you could rebase this instead? Let us know if you need any help!

@lotharthesavior lotharthesavior force-pushed the lotharthesavior:master branch from 53a5add to 91bd5fb Oct 12, 2018
@lotharthesavior

This comment has been minimized.

Copy link
Contributor Author

lotharthesavior commented Oct 12, 2018

Hi, @addaleax , thanks for the review, I removed the merge commit. Let me know if there is something else.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 12, 2018

@BridgeAR BridgeAR changed the title Reordered parameters to the test-tcp-wrap-connect test: fix assert.strictEqual() parameter order Oct 12, 2018

const shutdownReq = new ShutdownWrap();
const err = client.shutdown(shutdownReq);
assert.strictEqual(err, 0);

shutdownReq.oncomplete = function(status, client_, error) {
assert.strictEqual(0, status);
assert.strictEqual(client, client_);
assert.strictEqual(error, undefined);

This comment has been minimized.

Copy link
@trivikr

trivikr Oct 12, 2018

Member

Should this order be preserved?

This comment has been minimized.

Copy link
@lotharthesavior

lotharthesavior Oct 12, 2018

Author Contributor

Hi, @trivikr , would you explain further you comment? I believe that the order here is already changed, unless you meant something else. I didn't understand before, but now I got what you said. I have a question, though: why would it be preserved?

This comment has been minimized.

Copy link
@trivikr

trivikr Oct 13, 2018

Member

My bad, the comment was for line 34.
The strictEqual assertions should follow the order actual-value -> expected-value, where error is actual value, while undefined is expected value.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Oct 15, 2018

Member

Fixed while landing.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
PR-URL: nodejs#23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
PR-URL: nodejs#23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 15, 2018

Landed in aee771c 🎉

@lotharthesavior congratulations on your commit to Node.js!

@BridgeAR BridgeAR closed this Oct 15, 2018
@lotharthesavior

This comment has been minimized.

Copy link
Contributor Author

lotharthesavior commented Oct 15, 2018

Thanks, @BridgeAR ! I'm looking forward to contributing more!

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018
PR-URL: nodejs#23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
jasnell added a commit that referenced this pull request Oct 17, 2018
PR-URL: #23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Oct 20, 2018
PR-URL: #23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Oct 30, 2018
PR-URL: #23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
PR-URL: #23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
PR-URL: #23564
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.