test: skip cpu-intensive tests on slow hosts #8652

Trott · 2016-09-19T01:28:41Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test arm V8

Description of change

The test-tick-processor-* tests are now passing everywhere except for
the single-processor 700MHz Raspberry Pi 1 devices.

The tests are CPU-intensive. Skip the tests if there is only one CPU and
it runs at a speed not more than 700 MHz.

Trott · 2016-09-19T01:46:02Z

Since it's looking good to me (only skipping the test on the Raspberry Pi 1 devices, running fine everywhere else), and since this is causing a loooooong streak of red in CI, and since the change is pretty small/manageable, I'd kind of like to land this more quickly than the usual 48 hours. /cc @matthewloring @indutny @nodejs/testing @nodejs/build

matthewloring · 2016-09-19T03:16:19Z

test/parallel/test-tick-processor-builtin.js

-// the full 64 bits and the result is that it does not process the
-// addresses correctly and runs out of memory
-// Disabling until we get a fix upstreamed into V8
-if (common.isAix) {


Sorry, I think I missed a refactoring CL in here. Why is this check no longer necessary?

It's unnecessary in this test because there's a second common.isAix check that exits as well several lines below.

The check still exists in test-tick-processor-unknown.js which is where it matters.

Trott · 2016-09-19T04:22:19Z

Oh, wait, it was timing out before. I have to get my story straight.

OK, so we're back to just the mystery of "why is the checking-cpus-and-skipping code only working in one of the three tests? how is that even possible?"

targos · 2016-09-19T07:10:29Z

If you look at https://ci.nodejs.org/job/node-test-binary-arm/3841/RUN_SUBSET=2,label=pi1-raspbian-wheezy/tapTestReport/ you will see that the duration of the test was 0.18 sec. How could it fail with a timeout ?

Trott · 2016-09-23T18:15:15Z

CI for the current version of this test looks good: https://ci.nodejs.org/job/node-test-commit/5270/

Test is skipped on Pi 1 devices but is run on Pi 2 and Pi 3. [EDIT: But of course now one of the tests fails on one of the Pi 2 devices...]

It's possible this code change will help (or can help with some modification) out on #8725 as well.

The ultimate question is: Are we comfortable deciding that the V8 tick processor tests (at least as currently formulated) should not be expected to complete in a timely fashion on slow single-processor machines? Or is that test smell? /cc @matthewloring @indutny @geek

Trott · 2016-09-23T18:57:18Z

Tried moving from parallel to sequential. Why not?

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

Just about done and looks green except for the perma-yellow unrelated AIX failure.

Trott · 2016-09-23T19:50:47Z

Move to sequential (probably) had nothing to do with the success because stress test (on SmartOS 14 32-bit) is still failing frequently.

Anyway, that's the least important part of this to discuss, so I'll undo it and we can hopefully focus on "Is this a valid approach or should the test really be workable on Pi 1?" In other words: "Is the test CPU constrained and if so, is it OK to skip it in slow CPU situations?"

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz.

matthewloring · 2016-09-23T21:30:02Z

This test is CPU constrained. Originally, the test ran a piece of code for a fixed duration (2 seconds) and then checked the generated profile. This guaranteed that the test wouldn't time out but made it more likely to fail in CPU constrained situations where fewer iterations of the function were executed. #8542 removed this timeout inside the test causing it to rerun when the desired symbols were not seen in the profile. While reducing the flakiness, this introduces the possibility that the test will hit a global timeout on CPU constrained devices. Both flaky fails and timeouts could be avoided by increasing the timeout on slow machines. If this isn't desired, it should be fine to skip this test on slow CPU machines as long as we still have good platform coverage.

Trott · 2016-09-23T23:01:07Z

@matthewloring Thanks for the detailed, thorough, and clear explanation.

Trott · 2016-09-23T23:03:41Z

Given @matthewloring's explanation above and what @indutny has written elsewhere, I'm inclined to think this is an acceptable approach: Skip these tests on machines with slow CPU and rely on coverage in the rest of the test infrastructure. While not a fix for all flakiness in these tests, it at least gets us past the pretty reliable failures on the armv6 Raspberry Pi devices.

However, it can't land without an LGTM from a Collaborator. (Hint, hint?)

matthewloring · 2016-09-23T23:11:52Z

LGTM.

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz. PR-URL: nodejs#8652 Reviewed-By: Matthew Loring <mattloring@google.com>

Trott · 2016-09-24T22:46:16Z

Landed in d196c5d

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz. PR-URL: #8652 Reviewed-By: Matthew Loring <mattloring@google.com>

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz. PR-URL: nodejs#8652 Reviewed-By: Matthew Loring <mattloring@google.com>

MylesBorins · 2016-10-06T23:37:17Z

@Trott should this be backported?

Trott · 2016-10-07T02:09:18Z

@Trott should this be backported?

If #8542 is backported, then probably yes.

If not, then probably no.

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz. PR-URL: #8652 Reviewed-By: Matthew Loring <mattloring@google.com>

Trott added v8 engine Issues and PRs related to the V8 dependency. test Issues and PRs related to the tests. arm Issues and PRs related to the ARM platform. labels Sep 19, 2016

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

Trott force-pushed the check-enough-cpu branch from dc32267 to 314ee1b Compare September 19, 2016 01:40

Trott force-pushed the check-enough-cpu branch from 314ee1b to dbdb5dc Compare September 19, 2016 02:14

matthewloring reviewed Sep 19, 2016

View reviewed changes

Trott force-pushed the check-enough-cpu branch 3 times, most recently from 0ef3081 to 71eddef Compare September 20, 2016 21:03

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

Trott force-pushed the check-enough-cpu branch 2 times, most recently from 50d646b to ab2e147 Compare September 21, 2016 18:22

Trott added the wip Issues and PRs that are still a work in progress. label Sep 21, 2016

Trott force-pushed the check-enough-cpu branch 2 times, most recently from 9983dc1 to 21635c7 Compare September 23, 2016 17:34

Trott mentioned this pull request Sep 23, 2016

parallel/test-tick-processor-unknown failures #8725

Closed

test: skip cpu-intensive tests on slow hosts

096ba37

The `test-tick-processor-*` tests are now passing everywhere except for the single-processor 700MHz Raspberry Pi 1 devices. The tests are CPU-intensive. Skip the tests if there is only one CPU and it runs at a speed not more than 700 MHz.

Trott force-pushed the check-enough-cpu branch from cbe199c to 096ba37 Compare September 23, 2016 19:51

Trott closed this Sep 24, 2016

MylesBorins added the lts-watch-v4.x label Oct 6, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

snyk-bot mentioned this pull request Jul 17, 2020

[Snyk] Security upgrade eslint from 3.19.0 to 4.0.0 adamlaska/node#38

Open

Trott deleted the check-enough-cpu branch January 13, 2022 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skip cpu-intensive tests on slow hosts #8652

test: skip cpu-intensive tests on slow hosts #8652

Trott commented Sep 19, 2016

Trott commented Sep 19, 2016

matthewloring Sep 19, 2016

Trott Sep 19, 2016

Trott Sep 19, 2016

Trott commented Sep 19, 2016

targos commented Sep 19, 2016

Trott commented Sep 23, 2016 •

edited

Loading

Trott commented Sep 23, 2016

Trott commented Sep 23, 2016

matthewloring commented Sep 23, 2016

Trott commented Sep 23, 2016

Trott commented Sep 23, 2016

matthewloring commented Sep 23, 2016

Trott commented Sep 24, 2016

MylesBorins commented Oct 6, 2016

Trott commented Oct 7, 2016

test: skip cpu-intensive tests on slow hosts #8652

test: skip cpu-intensive tests on slow hosts #8652

Conversation

Trott commented Sep 19, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Sep 19, 2016

matthewloring Sep 19, 2016

Choose a reason for hiding this comment

Trott Sep 19, 2016

Choose a reason for hiding this comment

Trott Sep 19, 2016

Choose a reason for hiding this comment

Trott commented Sep 19, 2016

targos commented Sep 19, 2016

Trott commented Sep 23, 2016 • edited Loading

Trott commented Sep 23, 2016

Trott commented Sep 23, 2016

matthewloring commented Sep 23, 2016

Trott commented Sep 23, 2016

Trott commented Sep 23, 2016

matthewloring commented Sep 23, 2016

Trott commented Sep 24, 2016

MylesBorins commented Oct 6, 2016

Trott commented Oct 7, 2016

Trott commented Sep 23, 2016 •

edited

Loading