Skip to content

test: mark tests as flaky as intermediate step - #20835

Closed
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:mark-tests-flaky
Closed

test: mark tests as flaky as intermediate step#20835
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:mark-tests-flaky

Conversation

@BridgeAR

@BridgeAR BridgeAR commented May 19, 2018

Copy link
Copy Markdown
Member

We have a few http2 issues that are currently resolved. Until they
are just mark these two tests as flaky.

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 Node.js core tests and test infrastructure. label May 19, 2018
@BridgeAR
BridgeAR requested review from addaleax and apapirovski May 19, 2018 12:01
@BridgeAR BridgeAR added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label May 19, 2018
@BridgeAR

Copy link
Copy Markdown
Member Author

This is a alternative to #20834 and #20832

We have a few http2 issues that are currently resolved. Until they
are, just mark these two tests as flaky.
@BridgeAR
BridgeAR force-pushed the mark-tests-flaky branch from 509b793 to 236ffc0 Compare May 19, 2018 12:02

@apapirovski apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SGTM.

@BridgeAR

BridgeAR commented May 19, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 19, 2018
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 19, 2018
@addaleax

Copy link
Copy Markdown
Member

#20750 also mentions parallel/test-http2-client-upload.

I’d still prefer the partial revert from #20834 over marking that test as flaky, tbh.

@BridgeAR

Copy link
Copy Markdown
Member Author

@addaleax this way the test itself does not have to be touched though and the code can be fixed. I personally believe that the status is somewhat meant for cases like the current one where in case we do not want a full revert.

@apapirovski

Copy link
Copy Markdown
Contributor

I prefer this since the tests will still fail instead of just being green but won't make the CI red.

@gireeshpunathil

Copy link
Copy Markdown
Member

frequent CI failures due to flaky tests have been an issue off late - causes confusion to the PR at hand, needing to re-run leading to additional time and effort. Marking flaky until they are fixed is a good idea.

@BridgeAR

Copy link
Copy Markdown
Member Author

@addaleax are you OK with this instead of #20834?

@addaleax

Copy link
Copy Markdown
Member

@BridgeAR Feel free to land it, yes

@BridgeAR

Copy link
Copy Markdown
Member Author

Landed in 8254e38

@BridgeAR BridgeAR closed this May 19, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 19, 2018
We have a few http2 issues that are currently resolved. Until they
are, just mark these two tests as flaky.

PR-URL: nodejs#20835
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
We have a few http2 issues that are currently resolved. Until they
are, just mark these two tests as flaky.

PR-URL: #20835
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax addaleax mentioned this pull request May 22, 2018
@BridgeAR
BridgeAR deleted the mark-tests-flaky branch January 20, 2020 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-track PRs proposed for a shorter-than-standard waiting period before landing. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants