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.

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

bug: finally block in async function indicates incorrect missing block coverage #229

Open
bcoe opened this issue Jun 19, 2020 · 10 comments
Open

Comments

@bcoe
Copy link
Owner

bcoe commented Jun 19, 2020

Observed Behavior

The following code:

async function abc() {
  try {
    return 'abc';
  } finally {
    console.log('in finally');
  } // this is a covered line but uncovered branch
}

abc()

Ends up having the following coverage output:

{
          "functionName": "abc",
          "ranges": [
            {
              "startOffset": 0,
              "endOffset": 146,
              "count": 1
            },
            {
              "startOffset": 97,
              "endOffset": 145,
              "count": 0
            }
          ],
          "isBlockCoverage": true
        }
}

This indicates that the following characters were tracked as an uncovered block:

// this is a covered line but uncovered branch\n

Expected behavior

Coverage should be 100% in the above example.

@bcoe
Copy link
Owner Author

bcoe commented Jun 19, 2020

I have created a tracking issue on v8 here:

https://bugs.chromium.org/p/v8/issues/detail?id=10628

bcoe added a commit to nodejs/node that referenced this issue Jul 9, 2020
Original commit message:

    [coverage] remove the last continuation range before synthetic return

    Rather than only removing the continuation range for the last return
    statement prior to a synthetic return statement, remove the
    continuation tracking for whatever statement occurs prior to the
    synthetic return.

    Bug: v8:10628
    Change-Id: Ieb8e393479c9811cf1b9756840bbfdbe7f44a1b8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2280585
    Commit-Queue: Benjamin Coe <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Sigurd Schneider <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68719}

Refs: v8/v8@2d5017a

PR-URL: #34272
Refs: bcoe/c8#229
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit to targos/node that referenced this issue Jul 13, 2020
Original commit message:

    [coverage] remove the last continuation range before synthetic return

    Rather than only removing the continuation range for the last return
    statement prior to a synthetic return statement, remove the
    continuation tracking for whatever statement occurs prior to the
    synthetic return.

    Bug: v8:10628
    Change-Id: Ieb8e393479c9811cf1b9756840bbfdbe7f44a1b8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2280585
    Commit-Queue: Benjamin Coe <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Sigurd Schneider <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68719}

Refs: v8/v8@2d5017a

PR-URL: nodejs#34272
Refs: bcoe/c8#229
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Jul 13, 2020
Original commit message:

    [coverage] remove the last continuation range before synthetic return

    Rather than only removing the continuation range for the last return
    statement prior to a synthetic return statement, remove the
    continuation tracking for whatever statement occurs prior to the
    synthetic return.

    Bug: v8:10628
    Change-Id: Ieb8e393479c9811cf1b9756840bbfdbe7f44a1b8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2280585
    Commit-Queue: Benjamin Coe <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Sigurd Schneider <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68719}

Refs: v8/v8@2d5017a

PR-URL: #34272
Refs: bcoe/c8#229
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 14, 2020
Original commit message:

    [coverage] remove the last continuation range before synthetic return

    Rather than only removing the continuation range for the last return
    statement prior to a synthetic return statement, remove the
    continuation tracking for whatever statement occurs prior to the
    synthetic return.

    Bug: v8:10628
    Change-Id: Ieb8e393479c9811cf1b9756840bbfdbe7f44a1b8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2280585
    Commit-Queue: Benjamin Coe <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Sigurd Schneider <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68719}

Refs: v8/v8@2d5017a

PR-URL: #34272
Refs: bcoe/c8#229
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 16, 2020
Original commit message:

    [coverage] remove the last continuation range before synthetic return

    Rather than only removing the continuation range for the last return
    statement prior to a synthetic return statement, remove the
    continuation tracking for whatever statement occurs prior to the
    synthetic return.

    Bug: v8:10628
    Change-Id: Ieb8e393479c9811cf1b9756840bbfdbe7f44a1b8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2280585
    Commit-Queue: Benjamin Coe <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Sigurd Schneider <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68719}

Refs: v8/v8@2d5017a

PR-URL: #34272
Refs: bcoe/c8#229
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
evocateur added a commit to dendrochronologist/cli that referenced this issue Feb 3, 2021
@evocateur
Copy link

I'm not sure if it's related, but I think I hit this? I happened to be on c8 v7.4.0 when I encountered the failure in CI, but it reproduces in v7.5.0 when run on node v12.20.1.

The tests pass, it's just the mysterious coverage gap on an async function that fails in node v12. v14 is fine.

dendrochronologist/cli@edd0aaf "fixes" it by ignoring the missed line. Not ideal, long-term.

However, if I avoid using the async keyword, it passes again:

diff --git a/src/api.ts b/src/api.ts
index 99472c5..69c22ef 100644
--- a/src/api.ts
+++ b/src/api.ts
@@ -9,7 +9,7 @@ interface RunOptions extends Partial<ParsedConfig> {
   logger?: MinimalLogger;
 }
 
-export async function run({
+export function run({
   logger = processLogger(),
   ...options
 }: RunOptions): Promise<Node> {
@@ -22,8 +22,5 @@ export async function run({
   const arb = new Arborist({
     path: cwd,
   });
-  const tree = await arb.loadVirtual().catch(() => arb.loadActual());
-
-  return tree;
-  /* c8 ignore next */
+  return arb.loadVirtual().catch(() => arb.loadActual());
 }

I validated this locally with

volta run --node 12 npm test

@evocateur
Copy link

To be clear, the node v14 I was passing with is v14.15.4. Which tracks, given the release of the linked back-porting. I have literally no idea if it could (or should?) be back-ported to v12, though it seems like a good candidate if someone has bandwidth?

@bcoe
Copy link
Owner Author

bcoe commented Feb 5, 2021

@evocateur there are some v8 bug fixes that have landed in v14/v15, that I don't think got backported to v12 unfortunately -- I'm pretty sure that v12 might be closed to extension at this point unfortunately.

@evocateur
Copy link

@bcoe Perfectly fair! Especially considering it's going into maintenance mode in like, 2 months? So I'll probably drop v12 before I even finish this wee babby of a tool, lol.

@simlu
Copy link

simlu commented Mar 31, 2022

Just ran into this one as well. Rough one to debug. Any update regarding a fix?

@milanholemans
Copy link

milanholemans commented Sep 15, 2022

I'm encountering something similar. My finally block has a coverage of 26, while the keyword finally has no coverage.

image

I'm using 5 finally statements in the entire solution, 4 of them are working as expected, only this one has no coverage. I have no idea how this is possible.
I can send you a link to the code so you can see the problem yourself.

Update

Noticed that if you remove the throw statement in the catch block, that the finally is hit. However the amount of hits seems wrong.

image

@xolott
Copy link

xolott commented Oct 13, 2022

I'm also getting the same issue with a finally block. But this is not an async function.

image

Everything is 100% covered but the branch reports a 96.15% coverage

I just added an ignore comment for now
image

@bcoe
Copy link
Owner Author

bcoe commented Nov 3, 2022

@xolott would you be able to provide a minimal breaking example, this would help in investigating the underlying cause.

@archiehharris
Copy link

archiehharris commented Jun 14, 2024

@bcoe I have run into a similar issue as reported by @xolott, where the generated output is showing the finally block as uncovered. If you are still interested in troubleshooting, here is a minimum example repro

EDIT: Just noticed that if you change the handler to finish without returning then the finally block is marked as covered:
await orchestrator.activate()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants