Skip to content

Commit

Permalink
test: make test-v8-serdes work without stdin
Browse files Browse the repository at this point in the history
If `stdin` was closed or referred to a file, this didn't work,
because it was accessed via file descriptor.

Instead, use another generic native object.

cherry-picked from ayojs/ayo#63

Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
addaleax authored and danbev committed Oct 25, 2017
1 parent 57a716f commit 70670b0
Showing 1 changed file with 10 additions and 8 deletions.
18 changes: 10 additions & 8 deletions test/parallel/test-v8-serdes.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const objects = [
circular
];

const hostObject = new (process.binding('js_stream').JSStream)();

const serializerTypeError =
/^TypeError: Class constructor Serializer cannot be invoked without 'new'$/;
const deserializerTypeError =
Expand Down Expand Up @@ -63,8 +65,8 @@ const deserializerTypeError =
{
const ser = new v8.DefaultSerializer();
ser._writeHostObject = common.mustCall((object) => {
assert.strictEqual(object, process.stdin._handle);
const buf = Buffer.from('stdin');
assert.strictEqual(object, hostObject);
const buf = Buffer.from('hostObjectTag');

ser.writeUint32(buf.length);
ser.writeRawBytes(buf);
Expand All @@ -74,23 +76,23 @@ const deserializerTypeError =
});

ser.writeHeader();
ser.writeValue({ val: process.stdin._handle });
ser.writeValue({ val: hostObject });

const des = new v8.DefaultDeserializer(ser.releaseBuffer());
des._readHostObject = common.mustCall(() => {
const length = des.readUint32();
const buf = des.readRawBytes(length);

assert.strictEqual(buf.toString(), 'stdin');
assert.strictEqual(buf.toString(), 'hostObjectTag');

assert.deepStrictEqual(des.readUint64(), [1, 2]);
assert.strictEqual(des.readDouble(), -0.25);
return process.stdin._handle;
return hostObject;
});

des.readHeader();

assert.strictEqual(des.readValue().val, process.stdin._handle);
assert.strictEqual(des.readValue().val, hostObject);
}

{
Expand All @@ -101,12 +103,12 @@ const deserializerTypeError =

ser.writeHeader();
assert.throws(() => {
ser.writeValue({ val: process.stdin._handle });
ser.writeValue({ val: hostObject });
}, /foobar/);
}

{
assert.throws(() => v8.serialize(process.stdin._handle),
assert.throws(() => v8.serialize(hostObject),
/^Error: Unknown host object type: \[object .*\]$/);
}

Expand Down

6 comments on commit 70670b0

@Trott
Copy link
Member

@Trott Trott commented on 70670b0 Oct 25, 2017

Choose a reason for hiding this comment

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

This commit landed without a PR-URL.

Help us @nodejs/automation! :-D

@danbev
Copy link
Contributor

@danbev danbev commented on 70670b0 Oct 25, 2017

Choose a reason for hiding this comment

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

This commit landed without a PR-URL.

Sorry, can't believe I missed adding it :( Should I revert and update?

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

Was there a PR for this? I'm all for cherry-picking commits from ayo but I want to make sure those are going through the normal PR process :-)

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

Revert is not necessary, just drop the PR URL here in this comment thread if you would

@apapirovski
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's this one #16382

@joyeecheung
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder what we could do for this post-mortem with automation...a bot that checks every commit landed and tries to guess PR-URL?

Please sign in to comment.