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 test failing on AIX as flaky #8065

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Aug 11, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point. This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 11, 2016
@mhdawson mhdawson self-assigned this Aug 11, 2016
@mhdawson
Copy link
Member Author

@joaocgreis @Trott

I'm heading out on vacation so do you think once I get your ok I can land or do we need to wait the normal 48 hours.

@addaleax
Copy link
Member

Could you use a test: prefix for the commit? Otherwise LGTM and fast-tracking this seems okay.

We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under nodejs#7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under nodejs#7973
test-stdio-closed

- covered by nodejs#3796
test-debug-signal-cluster
@mhdawson
Copy link
Member Author

don't know why I was thinking "doc:" instead of "test:" fixed

@Trott
Copy link
Member

Trott commented Aug 11, 2016

LGTM. I think changes to .status files should be exempt from the 48-hour rule, but that's just my opinion. So I certainly won't complain if you land it sooner. Should definitely get a CI run first, though, as I'm not sure what types of mishaps can happen if there is a typo in the .status file. So...uh...CI: https://ci.nodejs.org/job/node-test-pull-request/3624/

@Trott Trott changed the title doc: mark test failing on AIX as flaky test: mark test failing on AIX as flaky Aug 11, 2016
@mhdawson
Copy link
Member Author

mhdawson commented Aug 11, 2016

Thanks, agree on the CI run was going to do that, thanks for launching
Will land once CI run completes if green and then add AIX to the regression runs

@joaocgreis
Copy link
Member

LGTM

I agree with @Trott , marking tests as flaky should be able to land immediately.

I started an AIX job with the exact same parameters that would have been used if the job had been launched from node-test-commit: https://ci.nodejs.org/view/All/job/node-test-commit-aix/331/

@mhdawson
Copy link
Member Author

@joaocgreis thanks, I had run my report on onnode-test-commit-aix but good to see it as will will be landed.

mhdawson added a commit that referenced this pull request Aug 11, 2016
We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under #7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under #7973
test-stdio-closed

- covered by #3796
test-debug-signal-cluster

PR-URL: #8065
Reviewed-By: joaocgreis - João Reis <reis@janeasystems.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mhdawson
Copy link
Member Author

Landed as a0b3bc5

@mhdawson
Copy link
Member Author

mhdawson commented Aug 11, 2016

Added AIX back to node-test-commit

Build on master to validate all is still green:https://ci.nodejs.org/job/node-test-commit/4519/ (had wrong link)

@Trott
Copy link
Member

Trott commented Aug 11, 2016

This is strange. I thought test-aix was supposed to be yellow, not green. (Wrong setup in the status file maybe?)

And even though it's green, it's showing up as red in the status posted to this PR. Huh?

@mhdawson
Copy link
Member Author

@joaocgreis I'm pretty sure I added the entries as suggested to make it yellow but do see its green instead.

@joaocgreis
Copy link
Member

The status file seems good. I haven't seen anything yellow in a while in CI, I'll investigate this.

The bits to show the status in PR's are quite new, and according to @jbergstroem still not final. I see a difference between what is in test-aix and test-linux, one runs if it succeeds and the other if it fails. But I'm no expert in this.

cjihrig pushed a commit that referenced this pull request Aug 11, 2016
We now have adequate AIX hardware to add AIX to
the regular regression runs.

However, there are a couple of failing tests even
though AIX was green at one point.  This PR
marks those tests as flaky so that we can add AIX
so that we can spot any new regressions without making
the builds RED

The tests are being worked under the following PRs

- being worked under #7564
test-async-wrap-post-did-throw
test-async-wrap-throw-from-callback
test-crypto-random

- being worked under #7973
test-stdio-closed

- covered by #3796
test-debug-signal-cluster

PR-URL: #8065
Reviewed-By: joaocgreis - João Reis <reis@janeasystems.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@Trott
Copy link
Member

Trott commented Aug 11, 2016

So strange that the post-status-to-PR widget is showing test-aix failing even though it succeeds....

@Fishrock123
Copy link
Contributor

cc @jbergstroem ^ any ideas?

@mhdawson
Copy link
Member Author

Going to close this since PR landed. Open this issue to track nodejs/build#467 issue with how flaky test failures are reported.

@mhdawson mhdawson closed this Aug 12, 2016
@jbergstroem
Copy link
Member

@Trott, @Fishrock123: yeah, the PR bot/jenkins stuff is not quite done hence my suggestion of delaying making it default. Fixed a few cases post this issue but not quite there yet!

@MylesBorins
Copy link
Contributor

adding to lts watch but it is not landing cleanly. I'm going to wait until we run ci on staging to see if we have AIX failures and revisit

@mhdawson mhdawson deleted the aixflaky branch March 15, 2017 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants