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.

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

Jump to bottom

Fix v8 tests #17383

Merged
merged 2 commits into from
Nov 30, 2017
Merged

Fix v8 tests #17383

merged 2 commits into from
Nov 30, 2017

Conversation

MylesBorins
Copy link
Contributor

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 MylesBorins requested a review from fhinkel November 29, 2017 02:43
@nodejs-github-bot 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
Copy link
Contributor Author

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 MylesBorins mentioned this pull request Nov 29, 2017
Closed
@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 29, 2017
@MylesBorins
Copy link
Contributor Author

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

refack commented Nov 29, 2017

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

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

MylesBorins commented Nov 29, 2017 via email

bnoordhuis
bnoordhuis approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@gibfahn
Copy link
Member

gibfahn commented Nov 29, 2017

+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
refack approved these changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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

mhdawson
mhdawson approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@hashseed
Copy link
Member

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

@MylesBorins
Copy link
Contributor Author

@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

@MylesBorins
Revert "deps: cherry-pick 3c8195d from V8 upstream"
f5ec28d 
This reverts commit e7f30db.

The original commit included tests for AsyncIterator that broke on 6.2

PR-URL: nodejs#17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
deps: backport 3c8195d from V8 upstream
58adb3c 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#17270
PR-URL: nodejs#17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor Author

landed in f2eaf87...58adb3c

@MylesBorins MylesBorins merged commit 58adb3c into nodejs:master Nov 30, 2017
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
Revert "deps: cherry-pick 3c8195d from V8 upstream"
0d21169 
This reverts commit e7f30db.

The original commit included tests for AsyncIterator that broke on 6.2

PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
deps: backport 3c8195d from V8 upstream
713eb84 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49237}

Fixes: #17270
PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Closed
2 tasks
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
Revert "deps: cherry-pick 3c8195d from V8 upstream"
e92b33e 
This reverts commit e7f30db.

The original commit included tests for AsyncIterator that broke on 6.2

PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
deps: backport 3c8195d from V8 upstream
cae6703 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49237}

Fixes: #17270
PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
Revert "deps: cherry-pick 3c8195d from V8 upstream"
465a32a 
This reverts commit e7f30db.

The original commit included tests for AsyncIterator that broke on 6.2

PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Dec 12, 2017
@MylesBorins
deps: backport 3c8195d from V8 upstream
47cd49a 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49237}

Fixes: #17270
PR-URL: #17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@MylesBorins MylesBorins deleted the fix-v8-tests branch December 20, 2017 01:41
targos pushed a commit to targos/node that referenced this pull request Jan 15, 2018
@MylesBorins @targos
deps: backport 3c8195d from V8 upstream
fe7a1f8 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#17270
PR-URL: nodejs#17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 16, 2018
@MylesBorins @gibfahn
deps: backport 3c8195d from V8 upstream
97821e8 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#17270
PR-URL: nodejs#17383
Backport-PR-URL: nodejs#16413
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 22, 2018
@MylesBorins @gibfahn
deps: backport 3c8195d from V8 upstream
5854746 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#17270
PR-URL: nodejs#17383
Backport-PR-URL: nodejs#16413
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit to targos/node that referenced this pull request Feb 7, 2018
@MylesBorins
deps: backport 3c8195d from V8 upstream
259192f 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#17270
PR-URL: nodejs#17383
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
@MylesBorins @gibfahn
deps: backport 3c8195d from V8 upstream
0a064c4 
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.

    R=gsathya@chromium.org, jgruber@chromium.org

    Bug: v8:7047
    Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
    Reviewed-on: https://chromium-review.googlesource.com/758372
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#49237}

Fixes: #17270
PR-URL: #17383
Backport-PR-URL: #16413
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@hashseed hashseed hashseed approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

@fhinkel fhinkel Awaiting requested review from fhinkel

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@MylesBorins @refack @TimothyGu @gibfahn @hashseed @bnoordhuis @targos @cjihrig @mhdawson @nodejs-github-bot