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.

Sign up for GitHub

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: fixed the arguments order in assert.strictEqual #24607

Closed
wants to merge 1 commit into from
Closed

test: fixed the arguments order in assert.strictEqual #24607

wants to merge 1 commit into from

Conversation

sota1235
Copy link
Contributor

@sota1235 sota1235 commented Nov 24, 2018 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 24, 2018
@ronkorving ronkorving added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 24, 2018
@gireeshpunathil
Copy link
Member

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

@gireeshpunathil
Copy link
Member

fast-track? pls 👍

@gireeshpunathil
Copy link
Member

only failure in windows-fanned test_threadsafe_function is known - #23621

@gireeshpunathil
Copy link
Member

landed as f351c5d , thanks!

gireeshpunathil pushed a commit that referenced this pull request Nov 28, 2018
@sota1235 @gireeshpunathil
test: fix arguments order in assert.strictEqual
f351c5d 
PR-URL: #24607
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 28, 2018

only failure in windows-fanned test_threadsafe_function is known - #23621

Per the Collaborators Guide, we may not land without a green or yellow CI. Please don't disregard failures, including known failures. (If they are to be disregarded, they need to be marked in the status file, which will result in a yellow CI when they fail.) Thanks.

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Nov 28, 2018 •
edited
Loading

@Trott - thanks for the correction! I did not know that we should not land even when the failing test has an open issue on it. I will follow that route.

the relevant section in the guide, however is not explicit on it and assumes that the failing job is flaky and may recover from a resume build option? In this case no point of using resume build as we know it to fail?

Do not land any Pull Requests without passing (green or yellow) CI runs. If you believe any failed (red or grey) CI sub-tasks are unrelated to the change in the Pull Request, use "Resume Build" in the left navigation of the relevant node-test-pull-request job. It will create a new node-test-pull-request run that preserves all the green results from the current job but re-runs everything else.

targos pushed a commit that referenced this pull request Nov 28, 2018
@sota1235 @targos
test: fix arguments order in assert.strictEqual
992a904 
PR-URL: #24607
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 29, 2018

I believe Resume Build now works in all cases including the fanned jobs. (If it doesn't, definitely notify Build WG. Or just use Rebuild instead of Resume Build. Or both.)

@gireeshpunathil gireeshpunathil mentioned this pull request Nov 29, 2018
Merged
4 tasks
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@sota1235 @refack
test: fix arguments order in assert.strictEqual
3e37563 
PR-URL: nodejs#24607
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@sota1235 @BethGriggs
test: fix arguments order in assert.strictEqual
1c609bf 
PR-URL: #24607
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sota1235 @rvagg
test: fix arguments order in assert.strictEqual
e7b8067 
PR-URL: #24607
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@shisama shisama shisama approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sota1235 @gireeshpunathil @Trott @addaleax @cjihrig @shisama @ronkorving @BethGriggs @nodejs-github-bot