Skip to content

test: parallelize long-running test #3287

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.

Sign up for GitHub

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

Closed
Trott wants to merge 4 commits into from
Closed

test: parallelize long-running test #3287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c3672b0
test: parallelize long-running test
Trott Oct 8, 2015
3b9e2be
fixup: update error message
Trott Oct 9, 2015
55a1d94
fixup
Trott Oct 9, 2015
21e054b
fixup: revert error message check
Trott Oct 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
test: parallelize long-running test
  • Loading branch information
@Trott
Trott committed Oct 9, 2015
commit c3672b09eb6c74e6d3acb85302a6f496b5daa99f
30 changes: 30 additions & 0 deletions test/parallel/test-stringbytes-external-at-max.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

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

// v8 fails silently if string length > v8::String::kMaxLength
(function() {
// v8::String::kMaxLength defined in v8.h
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

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

@evanlucas evanlucas Oct 8, 2015

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

@evanlucas evanlucas Oct 8, 2015

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

@trevnorris trevnorris Oct 8, 2015

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

@evanlucas evanlucas Oct 8, 2015

Choose a reason for hiding this comment

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

ah that's right

} catch(e) {
assert.equal(e.message, 'Invalid array buffer length');
console.log(
'1..0 # Skipped: intensive toString tests due to memory confinements');
return;
}

const buf = new Buffer(kStringMaxLength);

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

@trevnorris trevnorris Oct 8, 2015

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

@Trott Trott Oct 9, 2015

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

@trevnorris trevnorris Oct 9, 2015

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

@Trott Trott Oct 9, 2015

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.

// Free the memory early instead of at the end of the next assignment
maxString = undefined;

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

@trevnorris trevnorris Oct 9, 2015

Choose a reason for hiding this comment

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

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.

})();
52 changes: 52 additions & 0 deletions test/parallel/test-stringbytes-external-exceed-max-by-1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';

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

// v8 fails silently if string length > v8::String::kMaxLength
// v8::String::kMaxLength defined in v8.h
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

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

@evanlucas evanlucas Oct 8, 2015

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@trevnorris trevnorris Oct 8, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@trevnorris trevnorris Oct 9, 2015

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Trott Trott Oct 9, 2015

Choose a reason for hiding this comment

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

@trevnorris @evanlucas Updated in 3b9e2be

console.log(
'1..0 # Skipped: intensive toString tests due to memory confinements');
return;
}

const buf1 = new Buffer(kStringMaxLength + 1);

assert.throws(function() {
buf1.toString();
}, /toString failed|Invalid array buffer length/);

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

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

@trevnorris trevnorris Oct 9, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@trevnorris trevnorris Oct 9, 2015

Choose a reason for hiding this comment

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

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


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

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

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

var maxString = buf1.toString('binary', 1);
assert.equal(maxString.length, kStringMaxLength);
maxString = undefined;

maxString = buf1.toString('binary', 0, kStringMaxLength);
assert.equal(maxString.length, kStringMaxLength);
// Free the memory early instead of at the end of the next assignment
maxString = undefined;
22 changes: 22 additions & 0 deletions test/parallel/test-stringbytes-external-exceed-max-by-2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

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

// v8 fails silently if string length > v8::String::kMaxLength
// v8::String::kMaxLength defined in v8.h
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

try {
new Buffer(kStringMaxLength * 3);
} catch(e) {
assert.equal(e.message, 'Invalid array buffer length');
console.log(
'1..0 # Skipped: intensive toString tests due to memory confinements');
return;
}

const buf2 = new Buffer(kStringMaxLength + 2);

const maxString = buf2.toString('utf16le');
assert.equal(maxString.length, (kStringMaxLength + 2) / 2);
23 changes: 23 additions & 0 deletions test/parallel/test-stringbytes-external-exceed-max.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

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

// v8 fails silently if string length > v8::String::kMaxLength
// v8::String::kMaxLength defined in v8.h
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

try {
new Buffer(kStringMaxLength * 3);
} catch(e) {
assert.equal(e.message, 'Invalid array buffer length');
console.log(
'1..0 # Skipped: intensive toString tests due to memory confinements');
return;
}

const buf0 = new Buffer(kStringMaxLength * 2 + 2);

assert.throws(function() {
buf0.toString('utf16le');
}, /toString failed/);
69 changes: 0 additions & 69 deletions test/parallel/test-stringbytes-external.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,72 +107,3 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS;
assert.equal(a, b);
assert.equal(b, c);
})();

// v8 fails silently if string length > v8::String::kMaxLength
(function() {
// v8::String::kMaxLength defined in v8.h
const kStringMaxLength = process.binding('buffer').kStringMaxLength;

try {
new Buffer(kStringMaxLength * 3);
} catch(e) {
assert.equal(e.message, 'Invalid array buffer length');
console.log(
'1..0 # Skipped: intensive toString tests due to memory confinements');
return;
}

const buf0 = new Buffer(kStringMaxLength * 2 + 2);
const buf1 = buf0.slice(0, kStringMaxLength + 1);
const buf2 = buf0.slice(0, kStringMaxLength);
const buf3 = buf0.slice(0, kStringMaxLength + 2);

assert.throws(function() {
buf1.toString();
}, /toString failed|Invalid array buffer length/);

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

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

assert.throws(function() {
buf0.toString('utf16le');
}, /toString failed/);

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

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

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

var maxString = buf2.toString();
assert.equal(maxString.length, kStringMaxLength);
// Free the memory early instead of at the end of the next assignment
maxString = undefined;

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

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

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

maxString = buf3.toString('utf16le');
assert.equal(maxString.length, (kStringMaxLength + 2) / 2);
maxString = undefined;
})();