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: mark flaky tests as flaky #22330

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

In the last 100 CI runs, these tests have failed too many unrelated PRs

Reason     async-hooks/test-callback-error
Type       JS_TEST_FAILURE
Failed PR  13 (#22266, #22211, #22274, #22289, #22284, #22293, #22300, #22273, #22310, #22249, #22306, #22318, #22269)
Appeared   test-digitalocean-fedora27-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16383/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     parallel/test-cluster-master-error
Type       JS_TEST_FAILURE
Failed PR  10 (#22293, #22307, #22310, #22306, #22249, #22320, #22318, #21611, #22128, #22269)
Appeared   test-joyent-alpine38_container-x64-1, test-softlayer-alpine38_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16434/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     parallel/test-cluster-master-kill
Type       JS_TEST_FAILURE
Failed PR  10 (#22293, #22307, #22310, #22306, #22249, #22320, #22318, #21611, #22128, #22269)
Appeared   test-joyent-alpine38_container-x64-1, test-softlayer-alpine38_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16434/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16450/
--------------------------------------------------------------------------------
Reason     async-hooks/test-statwatcher
Type       JS_TEST_FAILURE
Failed PR  7 (#22274, #22290, #22288, #22293, #22242, #21611, #22302)
Appeared   test-azure_msft-win2016-x64-4, test-joyent-ubuntu1604_sharedlibs_container-x64-1
First CI   https://ci.nodejs.org/job/node-test-pull-request/16387/
Last CI    https://ci.nodejs.org/job/node-test-pull-request/16451/

Mark them as flaky for now so we can land PRs

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Aug 15, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 15, 2018
@joyeecheung
Copy link
Member Author

Waiting on the container rebuilds

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2018
@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

sorry for the CI interruption with containers. restarted @ https://ci.nodejs.org/job/node-test-pull-request/16456/

@@ -9,6 +9,9 @@ prefix parallel
[$system==win32]

[$system==linux]
# https://github.com/nodejs/node/issues/22308
test-cluster-master-kill: PASS,FLAKY
test-cluster-master-error: PASS,FLAKY
Copy link
Member

@Trott Trott Aug 15, 2018

Choose a reason for hiding this comment

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

The host/platform that these two were failing on has been removed from CI, so I think there's no need to mark them as flaky. (For what it's worth, they were failing 100% of the time and I'm not a fan of marking 100% failures as flaky.)

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, I'd like to see Alpine 3.8 back in rotation. We're risking either ignoring Alpine 3.8 completely, or if we mark this as flaky we risk not fixing the underlying problem. Pick your poison.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also against adding this, since it'll create a permanent yellow CI, and that is desensitizing...
We have a tracking issue, and we have eyes on the problem, so the chances of loosing track are lower.

@rvagg
Copy link
Member

rvagg commented Aug 15, 2018

@joyeecheung
Copy link
Member Author

Removed the cluster errors. New CI: https://ci.nodejs.org/job/node-test-pull-request/16461/

@joyeecheung joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 15, 2018

All the failures are git failures, so this should be ready to land

Failures in job https://ci.nodejs.org/job/node-test-pull-request/16461/

ubuntu1604-arm64

See failures on test-packetnet-ubuntu1604-arm64-2:
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	build/

no changes added to commit

ubuntu1604_sharedlibs_debug_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-2:
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

no changes added to commit

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

no changes added to commit

joyeecheung added a commit that referenced this pull request Aug 15, 2018
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
joyeecheung added a commit that referenced this pull request Aug 15, 2018
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in b87cb5d and 59c8abf

@Trott
Copy link
Member

Trott commented Aug 15, 2018

All the failures are git failures, so this should be ready to land

Do note that the COLLABORATOR_GUIDE "Testing and CI" section says that those need to be fixed and re-run rather than disregarded and landed:

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.

@joyeecheung
Copy link
Member Author

@Trott Ah, sorry about that, although in this case resuming the builds does not help because those are build failures. I am going to fix them in nodejs/build#1453

targos pushed a commit that referenced this pull request Aug 19, 2018
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Aug 19, 2018
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
Refs: #21425

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
Refs: #15985

PR-URL: #22330
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants