test: make test-tick-processor.js non-flaky by indutny · Pull Request #8542 · nodejs/node

indutny · 2016-09-14T23:28:52Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Wait for a sought-for symbol to appear instead of just hard-killing
subprocesses at 2s timeout.

Fix: #4427

Wait for a sought-for symbol to appear instead of just hard-killing subprocesses at 2s timeout. Fix: nodejs#4427

indutny · 2016-09-14T23:28:57Z

cc @Trott

indutny · 2016-09-14T23:29:05Z

cc @nodejs/testing

indutny · 2016-09-14T23:29:50Z

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

indutny · 2016-09-14T23:30:16Z

If CI is green - I'd suggest stress-run for that particular test in this PR.

Trott · 2016-09-14T23:52:59Z

 common.isFreeBSD) {
 common.skip('C++ symbols are not mapped for this os.');
- return;
+} else {


Did you mean to include the rest of the tests inside this else block? Either that or the return keyword has to be restored, I would think...

Yikes! Fixed now.

Trott · 2016-09-14T23:57:06Z

Also, if this PR ends up fixing the test's flakiness across all platforms, I'm totally naming my next goldfish Fedor in your honor. Well, probably not because that would be creepy. But I'm thinking about it anyway.

indutny · 2016-09-14T23:58:35Z

/me googled "creepy goldfish":

indutny · 2016-09-15T00:12:04Z

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

Trott · 2016-09-15T00:13:17Z

A test failure now will be a timeout as the test retries and retries and retries and never finds what it's looking for? (I am more than OK with that. Just making sure I understand the test and the intention.)

indutny · 2016-09-15T00:13:58Z

@Trott your analysis is correct.

Trott · 2016-09-15T03:25:03Z

Still some timeouts/failures. I wonder if splitting it into three test files would help so that:

each test can get the full 60 seconds (or whatever, depending on the platform) to succeed
when a test case fails, we know which one is failing

indutny · 2016-09-15T07:15:32Z

It won't help, looks like you can't open a locked file on Windows. I wonder if we can use pipes for this.

indutny · 2016-09-15T07:45:30Z

The newest version splits files into three and collects stdout output of --prof child, let's run CI one more time.

indutny · 2016-09-15T07:45:59Z

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

indutny · 2016-09-15T11:06:00Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/4055/

indutny · 2016-09-15T14:21:26Z

I think it should be generally ok, if we will increase allowed timeout values for new tick-processor tests. @Trott what do you think?

Trott · 2016-09-15T18:54:35Z

I think it should be generally ok, if we will increase allowed timeout values for new tick-processor tests. @Trott what do you think?

I'm not sure what you mean exactly. You mean the retry timeout? And for the Raspberry Pi devices only (because they look like the only ones failing right now, thankfully)? If so, we can probably set it to common.platformTimeout(750) instead of 750.

Would need run some stress tests but even as is, if this means the test are only flaky on the Raspberry Pi devices and not anywhere else, that's still a huge improvement!

indutny · 2016-09-15T19:23:54Z

@Trott I meant the test timeout, it looks like the test is killed too early on pi's.

Trott · 2016-09-15T20:19:02Z

@Trott I meant the test timeout, it looks like the test is killed too early on pi's.

Ah, yeah, that would be up to the @nodejs/build group I suppose. I think the timeout for Pi devices is already double or more that of most everything else, though.

An alternative might be to skip some or all (depending on whether some or all are failing) of these three tests on Pi devices by checking common.enoughTestMem and doing the common.skip() + return thing if it is false. (Does memory affect this test? Or is it more about CPU speed or something like that? If it's mostly a memory, then at least checking common.enoughTestMem makes sense and we're not using it as a proxy for "is Raspberry Pi".)

indutny · 2016-09-15T20:21:07Z

@Trott this seems to be CPU bound to me.

Trott · 2016-09-15T20:26:37Z

@Trott this seems to be CPU bound to me.

In that case, I guess long term, I'd like to see if @nodejs/build and @nodejs/testing would be OK with bumping the test timeout on the Raspberry Pi devices up. That might allow us to re-enable a handful of the other tests that we turned off for those devices.

In the meantime, just to get this fixed until something like that can happen, would it be too much of a code smell (and would it even work) to check the speed property of the objects in the array returned by os.cpus() and decide whether or not to skip based on that?

indutny · 2016-09-15T20:27:33Z

@Trott I think it should be safer to just mark these tests as FLAKY on ARM.

Trott · 2016-09-15T20:51:37Z

@Trott I think it should be safer to just mark these tests as FLAKY on ARM.

Sure, that works. The downside there is that it will also mark it flaky on the four ARM test platforms that aren't Raspberry Pi. But that seems acceptable to me.

indutny · 2016-09-15T20:59:09Z

@Trott we don't have ARM-specific code for these features, so it should be safe.

indutny · 2016-09-15T21:23:32Z

@Trott pushed an update that should supposedly mark these tests as FLAKY on arm only.

Trott · 2016-09-15T21:25:05Z

LGTM

indutny · 2016-09-15T21:27:45Z

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

indutny · 2016-09-15T21:29:27Z

@Trott I decided to re-run it to test FLAKY specifiers for the arch (we don't seem to use them anywhere else)

Trott · 2016-09-15T21:30:38Z

@Trott I decided to re-run it to test FLAKY specifiers for the arch (we don't seem to use them anywhere else)

Oh, yeah, the FLAKY test designation was ignored on Raspberry Pi for a while, IIRC. Might still be for all I know... Guess we're about to find out...

indutny · 2016-09-15T22:22:57Z

@Trott it looks like it didn't quite work. Should we land this as it is and defer the rest to the @nodejs/testing ?

Trott · 2016-09-16T00:04:11Z

@Trott it looks like it didn't quite work. Should we land this as it is and defer the rest to the @nodejs/testing ?

I think that would be OK.

Trott · 2016-09-16T00:13:39Z

If you want to be bold and bump up the Raspberry Pi devices from 3 minute timeout to 5 minute timeout, I think you just need to change the 3 on line 819 of test.py to 5. If that's all it takes, I would support that. :-D

And if you really don't want to spend more time tweaking things just to accommodate Raspberry Pi, I can also understand that. :-|

indutny · 2016-09-16T12:18:02Z

Landed in 5a17139, thank you!

Wait for a sought-for symbol to appear instead of just hard-killing subprocesses at 2s timeout. Fix: #4427 PR-URL: #8542 Reviewed-By: Rich Trott <rtrott@gmail.com>

test: make test-tick-processor.js non-flaky
a4b9b17
Wait for a sought-for symbol to appear instead of just hard-killing subprocesses at 2s timeout. Fix: nodejs#4427

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 14, 2016

mscdex added the process Issues and PRs related to the process subsystem. label Sep 14, 2016

Trott reviewed Sep 14, 2016
View reviewed changes

fix
feee308

lint
3ccc39c

split
6142cc4

lint
22cff8c

flaky
2e785fd

indutny closed this Sep 16, 2016

indutny deleted the fix/gh-4427 branch September 16, 2016 12:18

indutny added a commit that referenced this pull request Sep 16, 2016
test: make test-tick-processor.js non-flaky
5a17139
Wait for a sought-for symbol to appear instead of just hard-killing subprocesses at 2s timeout. Fix: #4427 PR-URL: #8542 Reviewed-By: Rich Trott <rtrott@gmail.com>

matthewloring mentioned this pull request Sep 23, 2016
test: skip cpu-intensive tests on slow hosts #8652
Closed
2 tasks

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
test: make test-tick-processor.js non-flaky
82b7894
Wait for a sought-for symbol to appear instead of just hard-killing subprocesses at 2s timeout. Fix: #4427 PR-URL: #8542 Reviewed-By: Rich Trott <rtrott@gmail.com>

Uh oh!

Uh oh!

Conversation

indutny commented Sep 14, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Sep 14, 2016

Uh oh!

indutny commented Sep 14, 2016

Uh oh!

indutny commented Sep 14, 2016

Uh oh!

indutny commented Sep 14, 2016

Uh oh!

TrottSep 14, 2016

Choose a reason for hiding this comment

Uh oh!

indutnySep 14, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 14, 2016

Uh oh!

indutny commented Sep 14, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Sep 15, 2016

Uh oh!

Trott commented Sep 16, 2016

Uh oh!

Trott commented Sep 16, 2016

Uh oh!

indutny commented Sep 16, 2016

Uh oh!

Trott commented Sep 15, 2016•
edited
Loading

Trott commented Sep 15, 2016•
edited
Loading