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 from call to assert.strictEqual() #19659

Conversation

@ForrestWeiswolf
Copy link
Contributor

commented Mar 28, 2018

Remove the message argument from call to assert.strictEqual so
that the AssertionError will report the value of er.code, and add
a comment with the message.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Remove the message argument from call to assert.strictEqual so
that the AssertionError will report the value of er.code, and add
a comment with the message.
@Trott
Trott approved these changes Mar 28, 2018
@Trott

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

(Could make the assert.strictEqual() all one line now if you want to.)

@Trott

This comment has been minimized.

@Trott Trott added the author ready label Mar 29, 2018
@ForrestWeiswolf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

It says 'node-test-commit — tests failed'. I'm having trouble reading the details of that; is there something I need to do at this point?

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

@ForrestWeiswolf TL/DR: Nothing to do, it's unrelated, I'll re-run the one host that failed.

Clicking through the results, there's only one failure, and it involves test/parallel/test-inspector-no-crash-ws-after-bindings.js. So it is almost certainly unrelated to this change.

Looking more closely, it is an issue we see a fair amount lately where a test process seems to crash in CI but no stack trace is provided. (@BridgeAR, another one! Is there an open issue I can link these too? I did a quick search but didn't find one.)

https://ci.nodejs.org/job/node-test-commit-plinux/16401/nodes=ppcle-ubuntu1404/console

not ok 1036 parallel/test-inspector-no-crash-ws-after-bindings
  ---
  duration_ms: 1.318
  severity: fail
  stack: |-
    [err] Debugger listening on ws://127.0.0.1:57972/e7574d26-00ef-4ec7-84b5-706de0257bd2
    [err] 
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [err] Can connect now!
    [err] 
    [test] Checking upgrade is not possible
    [test] Testing /json/list
  ...
@Trott

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

Re-running test-commit-plinux as the previous failure is certainly unrelated: https://ci.nodejs.org/job/node-test-commit-plinux/16427/

trivikr added a commit that referenced this pull request Apr 3, 2018
Remove the message argument from call to assert.strictEqual so
that the AssertionError will report the value of er.code, and add
a comment with the message.

PR-URL: #19659
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@trivikr

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Landed in 14310b5

Congratulations @ForrestWeiswolf for your first commit in Node.js core! 🎉🎉🎉

@trivikr trivikr closed this Apr 3, 2018
targos added a commit that referenced this pull request Apr 4, 2018
Remove the message argument from call to assert.strictEqual so
that the AssertionError will report the value of er.code, and add
a comment with the message.

PR-URL: #19659
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos referenced this pull request Apr 4, 2018
BethGriggs added a commit that referenced this pull request Dec 4, 2018
Remove the message argument from call to assert.strictEqual so
that the AssertionError will report the value of er.code, and add
a comment with the message.

PR-URL: #19659
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs referenced this pull request Dec 4, 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.