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

Jump to bottom

test: remove third argument from call to assert.strictEqual() #19659

Closed
wants to merge 1 commit into from
Closed

test: remove third argument from call to assert.strictEqual() #19659

wants to merge 1 commit into from

Conversation

ForrestWeiswolf
Copy link
Contributor

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

@ForrestWeiswolf
test: remove third argument from call to assert.strictEqual()
33e42e3 
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.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 28, 2018
@Trott
Copy link
Member

Trott commented Mar 28, 2018

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

@Trott
Copy link
Member

Trott commented Mar 28, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/13903/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2018
@ForrestWeiswolf
Copy link
Contributor Author

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
Copy link
Member

Trott 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
Copy link
Member

Trott 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 pushed a commit that referenced this pull request Apr 3, 2018
@ForrestWeiswolf @trivikr
test: remove third argument from call to assert.strictEqual()
14310b5 
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
Copy link
Member

trivikr 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 pushed a commit that referenced this pull request Apr 4, 2018
@ForrestWeiswolf @targos
test: remove third argument from call to assert.strictEqual()
46569d6 
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 mentioned this pull request Apr 4, 2018
Merged
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
@ForrestWeiswolf @BethGriggs
test: remove third argument from call to assert.strictEqual()
086570e 
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 mentioned this pull request Dec 4, 2018
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ForrestWeiswolf @Trott @trivikr @jasnell @TimothyGu @cjihrig @richardlau @nodejs-github-bot