Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

src: replace SetAccessor w/ SetAccessorProperty #38

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 14, 2018

Cherry-picks nodejs/node@efffcc2 into our electron-node-v8.9.3 branch for use in a future Electron 2.0.x release.

Requested by @jasonrudolph as a possible fix for electron/electron#13061 when running tests in headless mode in Atom (atom/atom#17409).

PR-URL: nodejs/node#17665
Fixes: nodejs/node#17636
Refs: nodejs/node#16482
Refs: nodejs/node#16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasonrudolph
Copy link

Thank you, @ckerr! ⚡🙇

@alexeykuzmin
Copy link
Contributor

Cherry-picks nodejs/node@efffcc2

@ckerr Is it a clean cherry-pick or you made some changes?

@ckerr ckerr changed the title src: replace SetAccessor w/ SetAccessorProperty [WIP] src: replace SetAccessor w/ SetAccessorProperty Jun 14, 2018
Remove reliance on V8-specific error messages in
test/parallel/test-tls-external-accessor.js.

Check that the error is a `TypeError`.

The test should now be successful without modification using ChakraCore.

Backport-PR-URL: nodejs/node#20456
PR-URL: nodejs/node#16272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@ckerr
Copy link
Member Author

ckerr commented Jun 15, 2018

@alexeykuzmin: @jasonrudolph found the upstream patch that fixes the tests too, nodejs/node@7eba62e

I've confirmed that Node's tests pass when I check out v8.9.3 and then cherry-pick nodejs/node@efffcc2 and nodejs/node@7eba62e together

@ckerr ckerr changed the title [WIP] src: replace SetAccessor w/ SetAccessorProperty src: replace SetAccessor w/ SetAccessorProperty Jun 15, 2018
@jasonrudolph
Copy link

@alexeykuzmin: @jasonrudolph found the upstream patch that fixes the tests too, nodejs/node@7eba62e

Including the failing test output here for reference:

=== release test-tls-external-accessor ===                                     
Path: parallel/test-tls-external-accessor
assert.js:649
      throw actual;
      ^
TypeError: Illegal invocation
    at assert.throws (/tmp/node/test/parallel/test-tls-external-accessor.js:14:28)
    at tryBlock (assert.js:619:5)
    at innerThrows (assert.js:638:18)
    at Function.throws (assert.js:662:3)
    at Object.<anonymous> (/tmp/node/test/parallel/test-tls-external-accessor.js:14:10)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
Command: out/Release/node /tmp/node/test/parallel/test-tls-external-accessor.js
[03:19|% 100|+ 2038|-   1]: Done                                               
Makefile:213: recipe for target 'test' failed
make: *** [test] Error 1

That failure appears to be resolved by nodejs/node#16272, which was backported to 8.x in nodejs/node#20478 via nodejs/node@7eba62e.

I've cherry-picked that commit onto this branch, and that fixes the failing test.

@ckerr ckerr merged commit f2bcc6b into electron-node-v8.9.3 Jun 15, 2018
@ckerr ckerr deleted the backport-efffcc26 branch June 15, 2018 20:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants