Fix v8 tests #17383

MylesBorins · 2017-11-29T02:43:33Z

I was digging in to e7f30db to try and figure out why the tests were broken on master. I noticed that the original commit came with tests for Async-Iterator, which afaik is not yet on 6.2

I've reverted the original change and relanded as a backport removing the async iterator tests

I bumped the V8 patch level for each of these commits... please let me know if I you think I should do it differently.

/cc @nodejs/v8

MylesBorins · 2017-11-29T02:45:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11790/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1088/

MylesBorins · 2017-11-29T03:12:19Z

This is failing on some systems due to infra failure, @refack can chime in about that I think. We do have green on the systems that are building though... so I think it is safe to say that this works!!!

I'd like to fast track this change.

refack · 2017-11-29T03:43:47Z

The ppc systems are back to normal
node CI: https://ci.nodejs.org/job/node-test-commit-plinux/13554/
(the V8 CI looks good enough IMO)

CentOS6 is known (fix in #17381)

TimothyGu · 2017-11-29T03:59:26Z

I bumped the V8 patch level for each of these commits... please let me know if I you think I should do it differently.

I'm okay with not bumping patch level at all, since V8 itself isn't changed by these two commits, only its tests. I'd also go with squashing the two commits into one.

MylesBorins · 2017-11-29T04:07:05Z

I personally want us to be very conservative with making changes inside the source tree, even to tests.

…

On Nov 29, 2017 12:59 PM, "Timothy Gu" ***@***.***> wrote: I bumped the V8 patch level for each of these commits... please let me know if I you think I should do it differently. I'm okay with not bumping patch level at all, since V8 itself isn't changed by these two commits, only its tests. I'd also go with squashing the two commits into one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17383 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVzJlh_gyQeYTnZ8T1634EcEt_MRrks5s7NakgaJpZM4QuVlm> .

bnoordhuis

LGTM but the first commit log should contain the reason for the revert.

gibfahn · 2017-11-29T11:59:00Z

+1 on bumping our (node) patch level, I use that as a count of "number of commits landed on top of the V8 patch level, so it's useful even for tests IMO.

refack

IMHO squash and put the PR OP as the commit message (bump v8_embedder_string)

mhdawson

LGTM

hashseed · 2017-11-29T15:00:22Z

Sorry about that. I haven't thought about backporting when I fixed that upstream :p

MylesBorins · 2017-11-29T16:33:59Z

@refack I'm not super comfortable with us floating arbitrary changes against V8. The revert is primarily to be explicit that the original cherry-pick was in correct and should have been a backport

This reverts commit e7f30db. The original commit included tests for AsyncIterator that broke on 6.2 PR-URL: nodejs#17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#49237} Fixes: nodejs#17270 PR-URL: nodejs#17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins · 2017-11-30T00:48:14Z

landed in f2eaf87...58adb3c

This reverts commit e7f30db. The original commit included tests for AsyncIterator that broke on 6.2 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#49237} Fixes: #17270 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This reverts commit e7f30db. The original commit included tests for AsyncIterator that broke on 6.2 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#49237} Fixes: #17270 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This reverts commit e7f30db. The original commit included tests for AsyncIterator that broke on 6.2 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#49237} Fixes: #17270 PR-URL: #17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#49237} Fixes: nodejs#17270 PR-URL: nodejs#17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#49237} Fixes: nodejs#17270 PR-URL: nodejs#17383 Backport-PR-URL: nodejs#16413 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#49237} Fixes: nodejs#17270 PR-URL: nodejs#17383 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Original commit message: Fix map constructor to correctly throw. We need to throw before rethrowing, otherwise the exception does not trigger a debugger event and is not reported if uncaught. [email protected], [email protected] Bug: v8:7047 Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6 Reviewed-on: https://chromium-review.googlesource.com/758372 Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#49237} Fixes: #17270 PR-URL: #17383 Backport-PR-URL: #16413 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins requested a review from fhinkel November 29, 2017 02:43

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Nov 29, 2017

MylesBorins force-pushed the fix-v8-tests branch from 13017ac to c181768 Compare November 29, 2017 02:44

MylesBorins mentioned this pull request Nov 29, 2017

V8 test broken on master #17270

Closed

MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 29, 2017

MylesBorins requested review from hashseed and targos November 29, 2017 03:13

bnoordhuis approved these changes Nov 29, 2017

View reviewed changes

cjihrig approved these changes Nov 29, 2017

View reviewed changes

targos approved these changes Nov 29, 2017

View reviewed changes

refack approved these changes Nov 29, 2017

View reviewed changes

hashseed approved these changes Nov 29, 2017

View reviewed changes

mhdawson approved these changes Nov 29, 2017

View reviewed changes

MylesBorins added 2 commits November 30, 2017 09:47

MylesBorins force-pushed the fix-v8-tests branch from c181768 to 58adb3c Compare November 30, 2017 00:47

MylesBorins closed this Nov 30, 2017

MylesBorins merged commit 58adb3c into nodejs:master Nov 30, 2017

targos added dont-land-on-v4.x labels Dec 6, 2017

targos added the dont-land-on-v8.x label Dec 6, 2017

MylesBorins mentioned this pull request Dec 12, 2017

deps: V8: backport 14ac02c from upstream #17512

Closed

2 tasks

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins deleted the fix-v8-tests branch December 20, 2017 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix v8 tests #17383

Fix v8 tests #17383

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

refack commented Nov 29, 2017

TimothyGu commented Nov 29, 2017

MylesBorins commented Nov 29, 2017 via email

bnoordhuis left a comment

gibfahn commented Nov 29, 2017

refack left a comment

mhdawson left a comment

hashseed commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 30, 2017

Fix v8 tests #17383

Fix v8 tests #17383

Conversation

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

refack commented Nov 29, 2017

TimothyGu commented Nov 29, 2017

MylesBorins commented Nov 29, 2017 via email

bnoordhuis left a comment

Choose a reason for hiding this comment

gibfahn commented Nov 29, 2017

refack left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

hashseed commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins commented Nov 30, 2017