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
Closed

Conversation

Trott
Copy link
Member

@Trott Trott 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

const kStringMaxLength = process.binding('buffer').kStringMaxLength;

try {
new Buffer(kStringMaxLength * 3);
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that's right

@mscdex mscdex added the test Issues and PRs related to the tests. label Oct 8, 2015
@Trott
Copy link
Member Author

Trott 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
Copy link
Contributor

Nice!!!!

const buf = new Buffer(kStringMaxLength);

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

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.

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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.

@trevnorris
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

Ref: #3303

@Trott
Copy link
Member Author

Trott 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
Copy link
Member Author

Trott 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
Copy link
Member Author

Trott commented Oct 10, 2015

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

@Trott
Copy link
Member Author

Trott 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
Copy link
Member

targos commented Oct 10, 2015

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

@evanlucas
Copy link
Contributor

LGTM

1 similar comment
@trevnorris
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell 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
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
Copy link
Member

jasnell commented Oct 11, 2015

Landed in 31c971d

@jasnell
Copy link
Member

jasnell 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
Copy link
Contributor

mscdex 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
Copy link
Member Author

Trott 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
Copy link
Contributor

mscdex commented Oct 11, 2015

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

@Trott
Copy link
Member Author

Trott 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
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
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
Copy link
Member

jasnell commented Oct 28, 2015

Landed in v4.x-staging in c76ef6c

Trott added a commit that referenced this pull request Oct 29, 2015
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
@Trott Trott deleted the borked-test branch January 13, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants