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: parallelize long-running test #3287

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
6 participants
@Trott
Copy link
Member

commented Oct 8, 2015

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

@evanlucas

View changes

test/parallel/test-stringbytes-external-at-max.js Outdated
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

try {
new Buffer(kStringMaxLength * 3);

This comment has been minimized.

Copy link
@evanlucas

evanlucas Oct 8, 2015

Member

do we need to allocate this much?

This comment has been minimized.

Copy link
@evanlucas

evanlucas Oct 8, 2015

Member

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

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 8, 2015

Contributor

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.

This comment has been minimized.

Copy link
@evanlucas

evanlucas Oct 8, 2015

Member

ah that's right

try {
new Buffer(kStringMaxLength * 3);
} catch(e) {
assert.equal(e.message, 'Invalid array buffer length');

This comment has been minimized.

Copy link
@evanlucas

evanlucas Oct 8, 2015

Member

when this call throws for me, I get a RangeError with a different message?

RangeError: Invalid typed array length
    at new Uint8Array (native)
    at allocate (buffer.js:98:17)
    at new Buffer (buffer.js:49:12)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 8, 2015

Contributor

Yup. This has changed since we now allocate Buffer's directly from JS. Thought I fixed all these error string instances.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 9, 2015

Contributor

@Trott Can you change this? The error message has changed recently due to a difference in Buffer allocation.

This comment has been minimized.

Copy link
@Trott

@mscdex mscdex added the test label Oct 8, 2015

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2015

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!

@evanlucas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

Nice!!!!

@trevnorris

View changes

test/parallel/test-stringbytes-external-at-max.js Outdated
const buf = new Buffer(kStringMaxLength);

var maxString = buf.toString();
assert.equal(maxString.length, kStringMaxLength);

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 8, 2015

Contributor

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.

This comment has been minimized.

Copy link
@Trott

Trott Oct 9, 2015

Author Member

@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.)

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 9, 2015

Contributor

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.

This comment has been minimized.

Copy link
@Trott

Trott Oct 9, 2015

Author Member

@trevnorris Fixed in 55a1d94. PTAL.

@trevnorris

View changes

test/parallel/test-stringbytes-external-at-max.js Outdated

maxString = buf.toString('binary');
assert.equal(maxString.length, kStringMaxLength);
maxString = undefined;

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 9, 2015

Contributor

This is a bit annoying, but also don't know if it's worth worrying about. Here's the basic test:

assert.equal(Buffer(kStringMaxLength * 2).toString('ucs2').length, kStringMaxLength);

Which means an even larger allocation would be necessary. Honestly, I'm not sure if doing this test gains us anything.


assert.throws(function() {
buf1.toString('utf8');
}, /toString failed/);

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 9, 2015

Contributor

Interestingly, this will still throw as expected even if there are multibyte characters in the string. It must have something to do with v8 internals.

On the side, I don't think that utf8, hex and base64 are necessary. utf8 for reasons similar to the above. hex and base64 will never be shorter than binary. It may be useful to add ucs2, but not sure if doing the larger allocation will break the test on pi.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Oct 9, 2015

Contributor

Nm about the ucs2 stuff. didn't see the other test files below.

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2015

Did another review. Thanks for working this out. The test has been a serious thorn.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2015

Ref: #3303

@Trott Trott force-pushed the Trott:borked-test branch to c3672b0 Oct 9, 2015

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2015

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.

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

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2015

Looks like the updating of the error message assertion broke the Pi 2 tests:

not ok 100 test-stringbytes-external-at-max.js
#
#assert.js:89
#  throw new assert.AssertionError({
#  ^
#AssertionError: 'Invalid array buffer length' == 'Invalid typed array length'
#    at Object.<anonymous> (/home/iojs/build/workspace/node-test-binary-arm/RUN_SUBSET/0/nodes/pi2-raspbian-wheezy/test/parallel/test-stringbytes-external-at-max.js:13:10)
#    at Module._compile (module.js:425:26)
#    at Object.Module._extensions..js (module.js:432:10)
#    at Module.load (module.js:356:32)
#    at Function.Module._load (module.js:311:12)
#    at Function.Module.runMain (module.js:457:10)
#    at startup (node.js:134:18)
#    at node.js:961:3
  ---
  duration_ms: 1.429

I'm inclined to roll back 3b9e2be.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2015

Reverted the error message check, trying again on CI: https://ci.nodejs.org/job/node-test-pull-request/474/

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2015

CI looks good now. arm-fanned passing! If this is sufficiently better than the current situation to merge, I think test-child-process-emfile.js on 32-bit FreeBSD is the next target on Failing Test Whack-A-Mole.

@targos

This comment has been minimized.

Copy link
Member

commented Oct 10, 2015

We could backport v8/v8@60f8317 to have a better error message.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Oct 10, 2015

LGTM

1 similar comment
@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2015

LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 11, 2015

LGTM... still not sure if this fixes the AIX issue /cc @mhdawson
will get this landed in master tho

Trott added a commit that referenced this pull request Oct 11, 2015

test: parallelize long-running test
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
@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 11, 2015

Landed in 31c971d

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 11, 2015

@mhdawson ... as soon as you get the chance, please let me know if this fixes the AIX problem

@jasnell jasnell closed this Oct 11, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2015

It looks like at least test-stringbytes-external-exceed-max-by-1.js is timing out occasionally on pi1-raspbian-wheezy.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2015

@mscdex test-stringbytes-external-exceed-max.js can take over a minute on pi1-raspbian-wheezy and it only has one call to .toString(). test-stringbytes-external-exceed-max-by-1.js has eight .toString() calls. The solution might be just to split test-stringbytes-external-exceed-max-by-1.js into multiple test files. (Splitting a monolithic test file into multiple test files is what got this from always-failing on pi1-raspbian-wheezy to only-failing-sometimes.)

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2015

@Trott Yeah I'm thinking it should be split up more (one for each .toString()?).

@Trott

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2015

@mscdex Let's try this: #3323

Trott added a commit to Trott/io.js that referenced this pull request Oct 12, 2015

test: parallelize long-running test
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

Trott added a commit that referenced this pull request Oct 28, 2015

test: parallelize long-running test
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
@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 28, 2015

Landed in v4.x-staging in c76ef6c

@jasnell jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 28, 2015

Trott added a commit that referenced this pull request Oct 29, 2015

test: parallelize long-running test
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

@jasnell jasnell removed the land-on-v4.x label Nov 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.