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: bailout test-vm-timeout-escape-nexttick #24712

Closed
wants to merge 1 commit into from

Conversation

gireeshpunathil
Copy link
Member

This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

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

Ref: #24620
Ref: #24120
/cc @Trott

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 29, 2018

[$arch==arm]
# https://github.com/nodejs/node/issues/24120
known_issues/test-vm-timeout-escape-nexttick: 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.

Is the entry respected when it includes the directory name like this? (I honestly don't know. We don't use it elsewhere, so regardless, for consistency, perhaps we shouldn't use it here?)

Suggested change
known_issues/test-vm-timeout-escape-nexttick: PASS,FLAKY
test-vm-timeout-escape-nexttick: PASS,FLAKY

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott - thanks, fixed.

@Trott
Copy link
Member

Trott commented Nov 29, 2018

@Trott
Copy link
Member

Trott commented Nov 29, 2018

Collaborators, 👍 here to fast-track.

@Trott Trott added 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. labels Nov 29, 2018
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

nit: Prefer if the commit message was something like test: mark test-vm-timeout-escape-nexttick as flaky to be more consistent with other similar commits.

This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.
@gireeshpunathil
Copy link
Member Author

@richardlau - thanks, done. unfortunately the commit header often runs on a thin line between being 50 chars or less and being a meaningful phrase; often the length of the test case name limits every other creativity possible with the words. :) In this case, the standard was not possible as it steps few chars out of the limit; so I coined something to fit the meaning and the length.

@Trott
Copy link
Member

Trott commented Nov 29, 2018

@gireeshpunathil We've relaxed the "must be 50 chars or less" requirement to be more of a "try to make it 50 chars or less" guideline. The commit message linting only warns now, rather than throws an error, if the first line is more than 50 chars.

From https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines:

contain a short description of the change (preferably 50 characters or less, and no more than 72 characters)

And yeah, keeping it to 50 chars when you are including a long test name (or even when you're not!) can be quite a challenge sometimes.

On a meta issue: One problem we have is that we have too many rules, often with subtle variations, scattered across too many files. So it's really not surprising that nobody (very much including me) can keep all the rules straight. Automation helps. Additionally, I personally hope we can consolidate things and try to get rid of rule variations as much as possible. For example, we should take anything useful out of the onboarding-extras.md doc and put it in some other existing document, then get rid of onboarding-extras.md. Then do the same for probably about 8 other files. :-D

@Trott
Copy link
Member

Trott commented Nov 29, 2018

CI again (only because force-push--it was good the first time): https://ci.nodejs.org/job/node-test-pull-request/19027/

@Trott
Copy link
Member

Trott commented Nov 29, 2018

Collaborators: Can we fast track this? 👍 here to approve.

This test is a real problem for CI reliability as it is failing rather frequently on Pi 1 devices and takes a long time for the Raspberry Pi devices to rerun the test suite. I would love to see this fast-tracked so we could get CI to yellow at least.

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 29, 2018
This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

PR-URL: nodejs#24712
Refs: nodejs#24620
Refs: nodejs#24120
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 29, 2018

Landed in 9e33e86. Thanks @gireeshpunathil!

@Trott Trott closed this Nov 29, 2018
targos pushed a commit that referenced this pull request Nov 29, 2018
This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

PR-URL: #24712
Refs: #24620
Refs: #24120
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

PR-URL: nodejs#24712
Refs: nodejs#24620
Refs: nodejs#24120
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

PR-URL: #24712
Refs: #24620
Refs: #24120
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
This is a known failure so mark it such, so that
CI is green / amber while the issue is being progressed.

PR-URL: #24712
Refs: #24620
Refs: #24120
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants