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

deps: v8 backport 2d08967 #26413

Closed
wants to merge 1 commit into from
Closed

deps: v8 backport 2d08967 #26413

wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Mar 3, 2019 •
edited
Loading

Backports v8/v8@2d08967, which addresses issues related to terminal throw statements. When collecting coverage currently, the line after a throw statement will not be tracked:

image

with this patch, the line below the throw statement starts being tracked:

screen shot 2019-03-03 at 10 08 52 am

this is one of the issues reported in #25937

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@bcoe bcoe added v8 engine Issues and PRs related to the V8 dependency. test Issues and PRs related to the tests. backport-requested-v11.x labels Mar 3, 2019
@bcoe bcoe requested review from hashseed and BridgeAR March 3, 2019 18:13
@nodejs-github-bot
Copy link
Collaborator

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2749/pipeline

@bcoe
deps: v8, backport 2d08967
6f986e2 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967
@bcoe
Copy link
Contributor Author

bcoe commented Mar 3, 2019 •
edited
Loading

ci: https://ci.nodejs.org/job/node-test-pull-request/21169/

@bcoe
Copy link
Contributor Author

bcoe commented Mar 4, 2019 •
edited
Loading

v8-build: https://ci.nodejs.org/job/node-test-commit-v8-linux/2133

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed backport-requested-v11.x labels Mar 4, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019

I removed the backport requested label as it has the opposite effect of what it probably was intended for: if set, this PR will not be automatically pulled in by our tools when backporting PRs (if possible) and instead we'll wait for a manual backport.

@bcoe
Copy link
Contributor Author

bcoe commented Mar 5, 2019 •
edited
Loading

@nodejs/build I think I'm ready to land this, but I'm noticing that nodes=benchmark-ubuntu1604-intel-64 is failing for v8 tests; this seems to be happening across the board for builds recently.

@bcoe
Copy link
Contributor Author

bcoe commented Mar 5, 2019

both normal and v8 tests are now passing, I intend to land this tomorrow if no one objects 👍

bcoe added a commit to bcoe/node-1 that referenced this pull request Mar 5, 2019
@bcoe
deps: v8, backport 2d08967
c78788a 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967

PR-URL: nodejs#26413
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bcoe
Copy link
Contributor Author

bcoe commented Mar 5, 2019

Landed in c78788a

@bcoe bcoe closed this Mar 5, 2019
@bcoe bcoe deleted the patch-throw branch March 5, 2019 17:51
@Trott
Copy link
Member

Trott commented Mar 5, 2019

Re-running daily coverage job to see the difference! https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/64/

cjihrig pushed a commit to cjihrig/node that referenced this pull request Mar 12, 2019
@bcoe @cjihrig
deps: v8, backport 2d08967
7ddb8fd 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967

PR-URL: nodejs#26413
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 12, 2019
@bcoe @targos
deps: v8, backport 2d08967
83bfa30 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967

PR-URL: nodejs#26413
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@bcoe @BridgeAR
deps: v8, backport 2d08967
01e69f9 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967

PR-URL: nodejs#26413
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
targos pushed a commit that referenced this pull request Mar 14, 2019
@bcoe @targos
deps: v8, backport 2d08967
1a6ed98 
Original commit message:

  [coverage] Extend SourceRangeAstVisitor for throw statements

  The SourceRangeAstVisitor has custom logic for blocks ending with a
  statement that has a continuation range. In these cases, the trailing
  continuation is removed which makes the reported coverage ranges a bit
  nicer.

  throw Error('foo') consists of an ExpressionStatement, with a
  Throw expression stored within the statement. The source range itself
  is stored with the Throw, not the statement.

  We now properly extract the correct AST node for trailing throw
  statements.

  R=jgruber@chromium.org, neis@chromium.org, yangguo@chromium.org

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <neis@chromium.org>
  Reviewed-by: Georg Neis <neis@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#59936}

Refs: v8/v8@2d08967

PR-URL: #26413
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@devsnek devsnek devsnek approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@hashseed hashseed Awaiting requested review from hashseed

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. 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.
7 participants
@bcoe @nodejs-github-bot @BridgeAR @Trott @jasnell @cjihrig @devsnek