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: coverage reported incorrectly with source-maps on Node 12 #90

Closed
bcoe opened this issue May 3, 2019 · 9 comments
Closed

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

bcoe opened this issue May 3, 2019 · 9 comments
Labels
bug help wanted Issue is well defined but needs assignment high priority v8

Comments

@bcoe
Copy link
Owner

bcoe commented May 3, 2019

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.

@bcoe bcoe added bug help wanted Issue is well defined but needs assignment high priority labels May 3, 2019
@shinnn
Copy link
Contributor

shinnn commented May 3, 2019

A minimal reproduction helps us to tackle this.

@bcoe
Copy link
Owner Author

bcoe commented May 3, 2019

@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.

@shinnn
Copy link
Contributor

shinnn commented May 3, 2019

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

@bcoe
Copy link
Owner Author

bcoe commented May 4, 2019

@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

@bcoe
Copy link
Owner Author

bcoe commented May 4, 2019

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.

@bcoe
Copy link
Owner Author

bcoe commented May 4, 2019

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

@Saul-Mirone
Copy link

Saul-Mirone commented May 13, 2019

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?

@bcoe
Copy link
Owner Author

bcoe commented May 13, 2019

@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?

@bcoe
Copy link
Owner Author

bcoe commented Oct 24, 2019

this was ultimately addressed upstream in v8.

@bcoe bcoe closed this as completed Oct 24, 2019
@bcoe bcoe added the v8 label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted Issue is well defined but needs assignment high priority v8
Projects
None yet
Development

No branches or pull requests

3 participants