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: mark test-worker-eventlooputil flaky #35886

Closed
wants to merge 1 commit into from
Closed

test: mark test-worker-eventlooputil flaky #35886

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 30, 2020
@MylesBorins MylesBorins added flaky-test Issues and PRs related to the tests with unstable failures on the CI. fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. and removed test Issues and PRs related to the tests. labels Oct 30, 2020
@MylesBorins
Copy link
Contributor Author

Can we please fast-track to fix CI?

@MylesBorins MylesBorins added the test Issues and PRs related to the tests. label Oct 30, 2020
@MylesBorins MylesBorins mentioned this pull request Oct 30, 2020
Closed
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@MylesBorins
test: mark test-worker-eventlooputil flaky
5b7b0b1 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: nodejs#35844
@MylesBorins MylesBorins force-pushed the mark-test-worker-eventlooputil-flaky branch from 6f2bb4a to 5b7b0b1 Compare October 30, 2020 18:15
@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2020
@nodejs-github-bot
Copy link
Collaborator

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

Trott
Trott reviewed Oct 30, 2020
View reviewed changes
test/parallel/parallel.status
Comment on lines 8 to +9
test-http2-respond-file-error-pipe-offset: PASS,FLAKY
test-worker-eventlooputil: PASS,FLAKY
Copy link
Member

Choose a reason for hiding this comment

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

Please include the GitHub issue for that test in a comment above. Same for the entry above that, as it was missed in the fast-track PR.

Suggested change
test-http2-respond-file-error-pipe-offset: PASS,FLAKY
test-worker-eventlooputil: PASS,FLAKY
# https://github.com/nodejs/node/issues/35881
test-http2-respond-file-error-pipe-offset: PASS,FLAKY
# https://github.com/nodejs/node/issues/35844
test-worker-eventlooputil: PASS,FLAKY

Trott
Trott approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with comments added.

@Trott
Copy link
Member

Trott commented Oct 30, 2020

I know I already approved this, but are we sure we want to do this? This is a recently added test for a recently added feature that has not gone out in any release. The failures may indicate that the feature is not ready for release. It may be the case that a revert is in order. But if we mark it as flaky, no one will notice and no one will care.

@Flarna Flarna mentioned this pull request Oct 31, 2020
Closed
@Qard
Copy link
Member

Qard commented Oct 31, 2020

I'm all for a revert or a fix as soon as we can manage it, but a temporary flaky mark would be nice to unblock the CI as currently almost every run is failing.

@gireeshpunathil
Copy link
Member

+1 for temporary flake status.

  • valuable diagnostic feature
  • lot of effort went into it
  • I have pinged its author
  • I will also investigate

if it does not work out well, I will update and we can revisit the decision?

@Trott
Copy link
Member

Trott commented Oct 31, 2020

Since only freebsd appears to be affected, the flaky designation should be moved there rather than the "all" section?

@Trott
Copy link
Member

Trott commented Oct 31, 2020

I'm not blocking since no one else seems to agree strongly with the concerns I'm raising. I'm conflicted myself. I can see it both ways. I mean, I did approve this, after all.

Either way, I would encourage folks to look at #35891 if they haven't already. A fix! (Possibly.)

@gireeshpunathil gireeshpunathil added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2020
@github-actions
Copy link
Contributor

Landed in c0af8bd...915a94c

@github-actions github-actions bot closed this Oct 31, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 31, 2020
@MylesBorins @nodejs-github-bot
test: mark test-worker-eventlooputil flaky
915a94c 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

PR-URL: #35886
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
@MylesBorins @targos
test: mark test-worker-eventlooputil flaky
830b789 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

PR-URL: #35886
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@targos targos mentioned this pull request Nov 3, 2020
Merged
nodejs-github-bot pushed a commit that referenced this pull request Nov 4, 2020
@Flarna @nodejs-github-bot
test: correct test-worker-eventlooputil
9dbde1d 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: #35891
Fixes: #35844
Refs: #35886
Refs: #35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Nov 4, 2020
@Flarna @targos
test: correct test-worker-eventlooputil
231af88 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: #35891
Fixes: #35844
Refs: #35886
Refs: #35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
@MylesBorins @BethGriggs
test: mark test-worker-eventlooputil flaky
1ec3b9d 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

PR-URL: #35886
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@MylesBorins @BethGriggs
test: mark test-worker-eventlooputil flaky
69924c4 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

PR-URL: #35886
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@MylesBorins @BethGriggs
test: mark test-worker-eventlooputil flaky
cab65fb 
This is consistently failing in CI right now. Lets mark it flaky
while we figure out what is going on.

Refs: #35844

PR-URL: #35886
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Feb 1, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
344f7ff 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
juanarbol pushed a commit to juanarbol/node that referenced this pull request Feb 1, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
40314bd 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37163
juanarbol pushed a commit to juanarbol/node that referenced this pull request Feb 10, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
e132479 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
richardlau pushed a commit that referenced this pull request Mar 2, 2021
@Flarna @richardlau
test: correct test-worker-eventlooputil
78c42ac 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: #35891
Fixes: #35844
Refs: #35886
Refs: #35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: #37163
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
77ef942 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 2, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
2e3251e 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 8, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
e663156 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 8, 2021
@Flarna @juanarbol
test: correct test-worker-eventlooputil
280d3c2 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
richardlau pushed a commit to juanarbol/node that referenced this pull request Mar 16, 2021
@Flarna @richardlau
test: correct test-worker-eventlooputil
d7a4ccd 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: nodejs#35891
Fixes: nodejs#35844
Refs: nodejs#35886
Refs: nodejs#35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: nodejs#37165
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
@Flarna @MylesBorins
test: correct test-worker-eventlooputil
01ba1c9 
The active worker check compared the time from sending message till
response arrived from worker with the complete time the worker was
running till it responses to the spin request.

If sending back the message is slow for some reason the test fails.

Adapt the test to compare the time seen inside the worker with the
time read from main thread.

PR-URL: #35891
Fixes: #35844
Refs: #35886
Refs: #35664
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: #37163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickyes rickyes rickyes approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@Trott Trott Trott approved these changes

@Qard Qard Qard approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MylesBorins @nodejs-github-bot @Trott @Qard @gireeshpunathil @rickyes