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: fix flaky test/fs-read-buffer-tostring-fail #7575

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 7, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

Description of change

The test is memory intensive and times out occasionally on Raspberry Pi
devices in CI. Successful test runs take about 90 seconds, but the
devices time out after 120 seconds. That's not a lot of headroom. So
let's skip the test on devices that have only modest amounts of memory.

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. arm Issues and PRs related to the ARM platform. labels Jul 7, 2016
The test is memory intensive and times out occasionally on Raspberry Pi
devices in CI. Successful test runs take about 90 seconds, but the
devices time out after 120 seconds. That's not a lot of headroom. So
let's skip the test on devices that have only modest amounts of memory.

Fixes: nodejs#7042
@@ -1,6 +1,13 @@
'use strict';

const common = require('../common');

const skipMessage = 'intensive toString tests due to memory confinements';
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't there need to be a header here or something, like the other tests that have a similar skip message?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. Here's another example:

const skipMessage = 'intensive toString tests due to memory confinements';
if (!common.enoughTestMem) {
common.skip(skipMessage);
return;
}

And here's what this test pass when it skips:

1..0 # Skipped: intensive toString tests due to memory confinements

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'll run CI and see what happens on the Raspberry Pi devices...

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I didn't realize .skip() was adding the necessary header.

@Trott
Copy link
Member Author

Trott commented Jul 7, 2016

@Trott
Copy link
Member Author

Trott commented Jul 7, 2016

Only CI failure is known FreeBSD flaky test. (How is everyone not LGTM-ing the crud out of #7555 to get that fixed?)

@Trott
Copy link
Member Author

Trott commented Jul 7, 2016

Raspberry Pis in CI seem to skip correctly:

ok 45 parallel/test-fs-read-buffer-tostring-fail # skip intensive toString tests due to memory confinements
  ---
  duration_ms: 2.53

That output format matches other skipped tests, such as:

ok 64 parallel/test-http-full-response # skip problem spawning `ab`.
  ---
  duration_ms: 2.855

Meanwhile, the test is not skipped on (for example) OS X:

ok 322 parallel/test-fs-read-buffer-tostring-fail
  ---
  duration_ms: 2.336

I think this is good to go.

@rvagg
Copy link
Member

rvagg commented Jul 7, 2016

lgtm, seems like the sensible way to go

@evanlucas
Copy link
Contributor

lgtm

@santigimeno
Copy link
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Jul 9, 2016
The test is memory intensive and times out occasionally on Raspberry Pi
devices in CI. Successful test runs take about 90 seconds, but the
devices time out after 120 seconds. That's not a lot of headroom. So
let's skip the test on devices that have only modest amounts of memory.

Fixes: nodejs#7042
PR-URL: nodejs#7575
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 9, 2016

Landed in b9b49ee

@MylesBorins
Copy link
Contributor

@Trott lts?

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

@thealphanerd Yes if it lands cleanly and CI passes.

evanlucas pushed a commit that referenced this pull request Jul 13, 2016
The test is memory intensive and times out occasionally on Raspberry Pi
devices in CI. Successful test runs take about 90 seconds, but the
devices time out after 120 seconds. That's not a lot of headroom. So
let's skip the test on devices that have only modest amounts of memory.

Fixes: #7042
PR-URL: #7575
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott Trott deleted the 90 branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants