-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: parallelize long-running test #3287
Conversation
const kStringMaxLength = process.binding('buffer').kStringMaxLength; | ||
|
||
try { | ||
new Buffer(kStringMaxLength * 3); |
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.
do we need to allocate this much?
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 ask mainly because from all of the tests I've run on a Pi 2, this will throw (every time from what I've seen). But it always seemed to get past this part a Pi 1
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.
The reason for the over allocation is to also judge the amount of extra space the heap has available. If we only allocate enough for the test then the toString()
operations may make v8 abort because of unavailable memory.
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.
ah that's right
CI: https://ci.nodejs.org/job/node-test-commit/774/ It's not red! First time in weeks, I think! I'm so happy, I'm going to put an exclamation point after every sentence! |
Nice!!!! |
const buf = new Buffer(kStringMaxLength); | ||
|
||
var maxString = buf.toString(); | ||
assert.equal(maxString.length, kStringMaxLength); |
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.
Not a good test. The memory needs to be filled or else it may contain random multi-byte characters and the string length might vary. Example:
let b = new Buffer(4).fill('a');
b[0] = 0xc8;
b[1] = 0xa2;
b.toString().length === 3;
So as much as I hate to do this, fill the above buffer with 0x61
(numbers are currently optimized better than strings). This way you get reliable results.
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.
@trevnorris This would seem to be a bug that exists in the current test and not something that was introduced in the refactoring I did, right? (I'm still happy to fix it, but I want to make sure I actually understand correctly. If I did cause this, I want to know where I goofed.)
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.
This is actually my fault. When these tests were added I specified to skip the zero fill. Discounting the utf-8 case. Though, in reality, the utf-8 case is not necessary. Since the length of a utf-8 string will always be <= a binary string. By filling it we're essentially doing a binary conversion internally since no multi-byte characters will be detected.
So I'd say we drop this one and skip the zero fill.
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.
@trevnorris Fixed in 55a1d94. PTAL.
Did another review. Thanks for working this out. The test has been a serious thorn. |
Ref: #3303 |
I think this is good to go for our current CI. PTAL @trevnorris @evanlucas I want to get the AIX issue in #3303 fixed too, but I don't want that to hold this up for the CI so I'd prefer to get this merged and then I (or someone else) can tackle #3303. |
Looks like the updating of the error message assertion broke the Pi 2 tests:
I'm inclined to roll back 3b9e2be. |
Reverted the error message check, trying again on CI: https://ci.nodejs.org/job/node-test-pull-request/474/ |
CI looks good now. |
We could backport v8/v8@60f8317 to have a better error message. |
LGTM |
1 similar comment
LGTM |
LGTM... still not sure if this fixes the AIX issue /cc @mhdawson |
Fixes a persistently troublesome failing test by splitting it out into multiple parallel tests. Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed By: Trevor Norris <trev.norris@gmail.com> Reviewed By: James M Snell <jasnell@gmail.com> PR-URL: #3287
Landed in 31c971d |
@mhdawson ... as soon as you get the chance, please let me know if this fixes the AIX problem |
It looks like at least |
@mscdex |
@Trott Yeah I'm thinking it should be split up more (one for each |
Fixes a persistently troublesome failing test by splitting it out into multiple parallel tests. Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed By: Trevor Norris <trev.norris@gmail.com> Reviewed By: James M Snell <jasnell@gmail.com> PR-URL: nodejs#3287
Fixes a persistently troublesome failing test by splitting it out into multiple parallel tests. Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed By: Trevor Norris <trev.norris@gmail.com> Reviewed By: James M Snell <jasnell@gmail.com> PR-URL: #3287
Landed in v4.x-staging in c76ef6c |
Fixes a persistently troublesome failing test by splitting it out into multiple parallel tests. Reviewed By: Evan Lucas <evanlucas@me.com> Reviewed By: Trevor Norris <trev.norris@gmail.com> Reviewed By: James M Snell <jasnell@gmail.com> PR-URL: #3287
I got terribly sad seeing
test-stringbytes-external
fail on Raspberry Pi in CI again and again and again, so I did the easiest laziest thing I could think of to fix it, which was to split the long-running test into multiple files so it wouldn't time out.Fixes (hopefull) #2370