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

Source map remapping has issues on files with no branches or functions #121

Open
bcoe opened this issue Oct 8, 2020 · 16 comments
Open

Source map remapping has issues on files with no branches or functions #121

bcoe opened this issue Oct 8, 2020 · 16 comments
Labels

Comments

@bcoe
Copy link
Member

bcoe commented Oct 8, 2020

I noticed this error cropping up for HTML reports:

Screen Shot 2020-10-08 at 1 10 55 PM

@LarsDenBakker
Copy link

We've been seeing this as well. Any clue on how we could fix this?

@bcoe
Copy link
Member Author

bcoe commented Oct 15, 2020

@LarsDenBakker it basically seems to be that the reporting logic is unhappy if there a file contains no branches or functions, is my theory. Interestingly, this only seems to crop up when we're applying source maps, and trying to convert from JavaScript to TypeScript.

What makes this problem so sticky, is that the AST from V8 doesn't necessarily match the AST that TypeScript used to build its source maps.

@bcoe
Copy link
Member Author

bcoe commented Oct 15, 2020

To address the issue, someone needs to dig into the output that v8-to-istanbul generates, and figure out what missing fields are causing issues for upstream reporters.

@LarsDenBakker
Copy link

I dug into it a while ago, but wasn't able to figure it out at the time :(

@bcoe
Copy link
Member Author

bcoe commented Nov 17, 2020

@LarsDenBakker are you still able to reproduce this, I did put a few fixes in place related to this bug a month ago.

@LarsDenBakker
Copy link

It looks like we're still on v5, so I'll update and ask if users are still seeing this error.

@mario-aleo
Copy link

mario-aleo commented Nov 30, 2020

I've just updated my @web/test-runner to the latest version (0.10.0) and unfortunately, I'm still getting the Cannot read property 'decl' of undefined error :(

image

Later I'll create a repository with this error and share it with you so you can easily reproduce it :)

@Westbrook
Copy link

This definitely intersects with what's going on in here: istanbuljs/istanbuljs#207, as while I get this issue in a handful of places, I can move it around by changing the use of exported fat arrow functions or not in various test contexts...even if those fat arrow methods are leveraged in non-covered files... 🤔

@bcoe
Copy link
Member Author

bcoe commented Feb 8, 2021

@mario-aleo @LarsDenBakker if you could provide a minimal reproduction, that would be super helpful 👍

@stephenwade
Copy link

stephenwade commented Mar 28, 2021

@bcoe @mario-aleo @LarsDenBakker I ran into this issue today with @web/test-runner. I was able to narrow it down to one class file and two test files.

If any of these characteristics are changed, the coverage report works properly:

  • The class function assigns an anonymous function to an instance property.
export class MyClass {
  fn() {
    this._ = function () {};
  }
}
  • There are two test files, both of which import the same class file.
  • Only one of the test files calls the class function.

https://github.com/stephenwade/coverage-issue-repro

To reproduce, run npm test and open coverage/lcov-report/my-class.js.html.

Screen shot of coverage error

@bcoe
Copy link
Member Author

bcoe commented Mar 31, 2021

@stephenwade thank you for the reproduction.

@Stuk
Copy link

Stuk commented Apr 30, 2021

I have a minimal repro repository here: https://github.com/Stuk/wtr-coverage-repro. The details of what I believe causes this issue are listed in the readme.

To summarize, metadata for functions in a file is only collected the first time that file is loaded, however some methods might only be defined when more code executed. Hence the above error when trying to access metadata for a covered function that doesn't always exist.

@bcoe
Copy link
Member Author

bcoe commented May 4, 2021

@Stuk thank you for the reproduction, not sure when I'll have cycles for OSS in the immediate term, but this will help the fix be quick when I can sit down to look.

@Stuk
Copy link

Stuk commented May 4, 2021

@bcoe I have a fix for this that I was able to find in the https://github.com/modernweb-dev/web repo that I'll be sending a PR for tomorrow. Unfortunately as I was skimming the code I wasn't able to work out how to fix it in this repo.

Stuk added a commit to Stuk/web that referenced this issue May 4, 2021
Collect and merge function definitions from all runs in coverage mapping

Fixes modernweb-dev#689

See also istanbuljs/v8-to-istanbul#121
LarsDenBakker pushed a commit to modernweb-dev/web that referenced this issue May 5, 2021
…ts (#1436)

Collect and merge function definitions from all runs in coverage mapping

Fixes #689

See also istanbuljs/v8-to-istanbul#121
@LarsDenBakker
Copy link

The fix from @Stuk fixes our logic of merging coverage of different test runs on different browsers. I'm not sure if that was the same scenario as originally reported in this issue, it might still occur without merging involved.

@Westbrook
Copy link

It is certainly possible, however I can report that the change specifically removed some instances of the following in my project.

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

No branches or pull requests

6 participants