test: fix flaky inspector-stop-profile-after-done #18126

Trott · 2018-01-12T20:36:30Z

In test/sequential/test-inspector-stop-profile-after-done.js, start the
profiler before running the code to avoid a race condition. If
Profile.start is received after the debugger is waiting to disconnect,
then the process will not terminate.

Fixes: #16772

Checklist

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

Affected core subsystem(s)

test inspector

Trott · 2018-01-12T23:29:06Z

Stress test showing failure on master:
https://ci.nodejs.org/job/node-stress-single-test/1682/nodes=pi1-raspbian-wheezy/console

Stress test for this pull request:
https://ci.nodejs.org/job/node-stress-single-test/1685/nodes=pi1-raspbian-wheezy/console

UPDATE: Failures still happening. Bummer.

apapirovski · 2018-01-12T23:32:32Z

~~Doesn't this get rid of the one line that's actually being tested?~~ Edit by @apapirovski: This can now be ignored.

[EDIT by @Trott: This comment is not applicable any more as the change has completely morphed into something different. Just don't want people to be confused to think that it applies to the fix that is currently there now or the message explaining it above that has been completely changed since the original posting.]

Trott · 2018-01-23T22:25:34Z

So, this is kind of interesting. The same exact branch on two different Pi 1 devices takes radically different amounts of time to run the test.

Here's test-requireio_securogroup-debian7-arm_pi1p-1 taking about 13 seconds to run the test and (so far, at least) never timing out: https://ci.nodejs.org/job/node-stress-single-test/1752/nodes=pi1-raspbian-wheezy/console

Here's the exact same branch running on test-requireio_mhdawson-debian7-arm_pi1p-1 and taking about 90 seconds to run the test and timing out here and there: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/31/label=pi1-raspbian-wheezy/console

@nodejs/build (particularly @rvagg) any idea what the cause of the disparity might be?

EDIT: Further testing seems to reveal that it gets way slower after the first failure. So the test will take 13 seconds or so an a Pi 1 until there's a timeout. Then it will take more like 90 seconds. But there doesn't seem to be a stale process, or at least not one I noticed. Maybe there is but my ps/grep is not finding it? Anyway, that's at least slightly less of a mystery, perhaps.

rvagg · 2018-01-23T23:00:38Z

Wow, big difference. It could be that test-requireio_mhdawson-debian7-arm_pi1p-1 was having trouble at the time, maybe some background work that wasn't killed from a previous run. It could also be that test-requireio_mhdawson-debian7-arm_pi1p-1 genuinely has problems, it's one of the Pi's that I have repeated problems with—although not enough to pull it entirely.

rvagg · 2018-01-23T23:02:10Z

Actually, the links you've provided indicate that they're now working at roughly the same level, the former in the 60-70 range and the latter in the 70-80 range. Maybe it's some kind of warm-up thing?

Trott · 2018-01-23T23:14:41Z

Actually, the links you've provided indicate that they're now working at roughly the same level, the former in the 60-70 range and the latter in the 70-80 range. Maybe it's some kind of warm-up thing?

Yipes! Interesting... Also, they're both showing failures too.

BridgeAR · 2018-02-01T17:46:51Z

Ping @Trott

Trott · 2018-02-02T03:27:09Z

Ping @Trott

I'm stumped for now.

Trott · 2018-02-02T04:28:59Z

Oh, I just had a blast-from-the-past memory about a similar problem and I now think I know what might be going on...

Trott · 2018-02-02T04:35:12Z

Stress test: https://ci.nodejs.org/job/node-stress-single-test/1765/nodes=pi1-raspbian-wheezy/

Trott · 2018-02-02T19:52:47Z

Hmmm, build failure on the stress test. Let's try the fanned stress test instead: https://ci.nodejs.org/view/All/job/node-stress-single-test-pi1-fanned/33/

And for comparison, here's master:
https://ci.nodejs.org/view/All/job/node-stress-single-test-pi1-fanned/34/

Trott · 2018-02-07T21:42:37Z

Trying something else: https://ci.nodejs.org/view/All/job/node-stress-single-test/1777/

Trott · 2018-02-08T17:56:13Z

So far, this latest change looks like it fixes it.

Stress test: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/40/label=pi1-raspbian-wheezy/console

…lts or not

Trott · 2018-03-02T07:05:05Z

Stress test is green! https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/83/label=pi1-raspbian-wheezy/

Removing in-progress flag.

Trott · 2018-03-02T07:05:55Z

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

apapirovski

Awesome. Thanks for continuing to work on this and finally resolving it, @Trott!

Use common.platformTimeout() to give longer durations to Raspberry Pi devices to make test more reliable. PR-URL: nodejs#18126 Fixes: nodejs#16772 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2018-03-02T20:12:42Z

Landed in 81d73fe

Use common.platformTimeout() to give longer durations to Raspberry Pi devices to make test more reliable. PR-URL: nodejs#18126 Fixes: nodejs#16772 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use common.platformTimeout() to give longer durations to Raspberry Pi devices to make test more reliable. PR-URL: #18126 Fixes: #16772 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added wip Issues and PRs that are still a work in progress. test Issues and PRs related to the tests. inspector Issues and PRs related to the V8 inspector protocol flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jan 12, 2018

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 12, 2018

Trott force-pushed the fix-flaky-inspector branch 3 times, most recently from 45d9aaa to 3e42d89 Compare January 23, 2018 16:56

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

Trott force-pushed the fix-flaky-inspector branch from cc09e8d to 1ef282b Compare February 1, 2018 06:30

Trott force-pushed the fix-flaky-inspector branch 2 times, most recently from a9b3589 to 507e811 Compare February 7, 2018 20:56

Trott force-pushed the fix-flaky-inspector branch from 507e811 to 2458454 Compare February 8, 2018 16:40

Trott force-pushed the fix-flaky-inspector branch 2 times, most recently from bc2d911 to cbea82d Compare February 8, 2018 18:14

Trott removed the wip Issues and PRs that are still a work in progress. label Feb 8, 2018

Trott added 14 commits March 1, 2018 17:55

wip: 3 sends

02fcc81

wip: is it better if we await the whole debugger-attach thing?

4c6c788

all four sends

4d3f4c8

wip: 3 sends but still waiting for debugger disconnect message

c57df84

try moving Profiler.start

be6a7f1

see if moving Profiler.start after program exits still has flaky resu…

053d19e

…lts or not

add an await

63ad338

no reason to think this will go any better but why not?

aff6d01

try writing to the process to let it know when to end

37748aa

remove profiler to see if that fixes it

9b9abc6

try longer times

7143464

doubling all the times: makes it worse or better or neither?

818e16f

increase times some more as it seems to enhance stability

ecd18cf

increase timeouts

e259d26

Trott force-pushed the fix-flaky-inspector branch from 46335e5 to e259d26 Compare March 2, 2018 02:01

Trott removed the wip Issues and PRs that are still a work in progress. label Mar 2, 2018

apapirovski approved these changes Mar 2, 2018

View reviewed changes

richardlau approved these changes Mar 2, 2018

View reviewed changes

Trott closed this Mar 2, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

Trott deleted the fix-flaky-inspector branch January 13, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix flaky inspector-stop-profile-after-done #18126

test: fix flaky inspector-stop-profile-after-done #18126

Trott commented Jan 12, 2018 •

edited

Loading

Trott commented Jan 12, 2018 •

edited

Loading

apapirovski commented Jan 12, 2018 •

edited

Loading

Trott commented Jan 23, 2018 •

edited

Loading

rvagg commented Jan 23, 2018

rvagg commented Jan 23, 2018

Trott commented Jan 23, 2018

BridgeAR commented Feb 1, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 7, 2018

Trott commented Feb 8, 2018

Trott commented Mar 2, 2018

Trott commented Mar 2, 2018

apapirovski left a comment

Trott commented Mar 2, 2018

test: fix flaky inspector-stop-profile-after-done #18126

test: fix flaky inspector-stop-profile-after-done #18126

Conversation

Trott commented Jan 12, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Jan 12, 2018 • edited Loading

apapirovski commented Jan 12, 2018 • edited Loading

Trott commented Jan 23, 2018 • edited Loading

rvagg commented Jan 23, 2018

rvagg commented Jan 23, 2018

Trott commented Jan 23, 2018

BridgeAR commented Feb 1, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 2, 2018

Trott commented Feb 7, 2018

Trott commented Feb 8, 2018

Trott commented Mar 2, 2018

Trott commented Mar 2, 2018

apapirovski left a comment

Choose a reason for hiding this comment

Trott commented Mar 2, 2018

Trott commented Jan 12, 2018 •

edited

Loading

Trott commented Jan 12, 2018 •

edited

Loading

apapirovski commented Jan 12, 2018 •

edited

Loading

Trott commented Jan 23, 2018 •

edited

Loading