-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Conversation
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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
node/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-at-max.js
Lines 11 to 15 in b10ee9d
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Only CI failure is known FreeBSD flaky test. (How is everyone not LGTM-ing the crud out of #7555 to get that fixed?) |
Raspberry Pis in CI seem to skip correctly:
That output format matches other skipped tests, such as:
Meanwhile, the test is not skipped on (for example) OS X:
I think this is good to go. |
lgtm, seems like the sensible way to go |
lgtm |
LGTM |
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>
Landed in b9b49ee |
@Trott lts? |
@thealphanerd Yes if it lands cleanly and CI passes. |
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>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected 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.