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: remove flaky status for test-npm-install #19216

Closed
wants to merge 1 commit into from
Closed

test: remove flaky status for test-npm-install #19216

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 8, 2018

I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: #13498

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott
test: remove flaky status for test-npm-install
43b1d95 
I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: nodejs#13498
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 8, 2018
@Trott
Copy link
Member Author

Trott commented Mar 8, 2018

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

maclover7
maclover7 approved these changes Mar 8, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1, is it documented anywhere how architectures/systems that have no tests listed are (or should) removed from status files?

@Trott
Copy link
Member Author

Trott commented Mar 8, 2018

is it documented anywhere how architectures/systems that have no tests listed are (or should) removed from status files?

I don't believe it is documented. Personally, I prefer to leave the architectures/systems there with no entries so that no one has to figure out how to add them back. (Is it arm or ARM or something else? That sort of guessing.)

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

Trott commented Mar 10, 2018

Landed in 9757906

@Trott Trott closed this Mar 10, 2018
Trott added a commit that referenced this pull request Mar 10, 2018
@Trott
test: remove flaky status for test-npm-install
9757906 
I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: #13498

PR-URL: #19216
Refs: #13498
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Mar 17, 2018
@Trott @targos
test: remove flaky status for test-npm-install
6c455a7 
I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: #13498

PR-URL: #19216
Refs: #13498
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@Trott @MylesBorins
test: remove flaky status for test-npm-install
411f3e0 
I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: #13498

PR-URL: #19216
Refs: #13498
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@Trott @MayaLekova
test: remove flaky status for test-npm-install
473b6cc 
I haven't seen test-npm-install fail on arm in a long time. There is no
open issue for it being unreliable. Remove flaky designation in
parallel.status file. (If it turns out it is flaky, it's easy enough to
open an issue and restore the flaky designation.)

Ref: nodejs#13498

PR-URL: nodejs#19216
Refs: nodejs#13498
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

should this be backported to 8.x?

@Trott
Copy link
Member Author

Trott commented Aug 18, 2018

should this be backported to 8.x?

It should cherry-pick cleanly to 8.x. It should land on 8.x IMO but if it doesn't, it's find to add a dont-land label. This is not a critical change.

@Trott Trott deleted the status-update branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@danbev danbev danbev 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.
8 participants
@Trott @jasnell @danbev @lpinca @cjihrig @maclover7 @richardlau @nodejs-github-bot