Skip to content

bug: coverage reported incorrectly with source-maps on Node 12 #90

Closed

Description

@bcoe

This is a strange one, for the following repo, which uses TypeScript and source-maps:

https://github.com/googleapis/release-please

coverage is reported correctly on Node 11, but does not seem to be correct on Node 12; it's over reporting.

@shinnn this seems like a high priority issue, as it leads to over reporting of coverage, which can give false confidence to users.

Activity

  1. shinnn commented on May 3, 2019

    @shinnn
    Contributor

    A minimal reproduction helps us to tackle this.

  2. bcoe commented on May 3, 2019

    @bcoe
    OwnerAuthor

    @shinnn if you:

    git clone relese-please,
    nvm use 12
    npm t

    The issue crops up in changelog.ts, I'll try to carve out a more minimal reproduction soon.

  3. shinnn commented on May 3, 2019

    @shinnn
    Contributor

    I hope anyone other than maintainers also can provide a small demo.

  4. bcoe commented on May 4, 2019

    @bcoe
    OwnerAuthor

    @shinnn an update on this one, I can't seem to reproduce this issue on my home computer, even though I'm seeing the behavior on my Linux desktop and laptop at work:

    Screen Shot 2019-05-04 at 11 39 36 AM

  5. bcoe commented on May 4, 2019

    @bcoe
    OwnerAuthor

    Here's logs demonstrating that the issue does sometimes occur though:

    https://source.cloud.google.com/results/invocations/c9c47dfa-73a9-4ae4-9ad9-49c30f85a6a5/log

    Note the 96.18% coverage for conventional-commits.ts which is over-reporting.

  6. bcoe commented on May 4, 2019

    @bcoe
    OwnerAuthor

    Issues on 11.11 (solved)

    solved: Fascinating, the issue happens on Node@11.10.1, but not Node@11.11.0; I think we're on the right track here

    ☝️ 11.11.0 is he version where compileFunction lands, eliminating the need for a wrapper; anything earlier than this should take into account wrapper size.

    Issues on 12.x (need to reproduce)

    We have a reproduction on Fedora, described here: nodejs/node#27566

  7. Saul-Mirone commented on May 13, 2019

    @Saul-Mirone

    I downgrade to 11 and test again, nothing change.

    I meet something strange: When I check coverage of js files generated from ts with source-map, I got something not covered like this:

    @Resolver()
    export default class CategoryResolver {
      @InjectRepository(Category)
      private readonly categoryRepository!: Repository<Category>;
    
      @Query((_) => [Category])
      public async categories() {
        const categories = await this.categoryRepository.find();
    
        return categories;
      }
    } // **this line not covered**

    The end brace is always not covered, is that the same problem as this issue?

  8. bcoe commented on May 13, 2019

    @bcoe
    OwnerAuthor

    @Saul-Mirone this is a fairly specific issue related to missing functions in Node 12 environments, this sounds like an off by one error (which might either be in c8, or in V8 itself).

    Mind opening another issue, with a minimal reproduction if possible?

  9. bcoe commented on Oct 24, 2019

    @bcoe
    OwnerAuthor

    this was ultimately addressed upstream in v8.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions