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

fix: source mapping for branch statements #518

Merged
merged 5 commits into from Oct 12, 2021
Merged

fix: source mapping for branch statements #518

merged 5 commits into from Oct 12, 2021

Conversation

ReneSchumacher
Copy link
Contributor

@ReneSchumacher ReneSchumacher commented Jan 17, 2020

This PR fixes an issue with file coverage generation when working with transpiled code and source maps (e.g., TypeScript projects). The current implementation has a bug that uses incorrect location information for switch statements. This leads to a mismatch between the statement and branch maps in the transformed file coverage object.

This fixes #517

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell Is there anything I can do to help speed this up? ;-)

@RoniSegal
Copy link

RoniSegal commented Jan 30, 2020

@ReneSchumacher Does this PR fixes also #291 ?

@ReneSchumacher
Copy link
Contributor Author

I'll write a unit test to check that (hopefully tonight if I can find the time). If it's not fixed, perhaps I can add a fix to this PR as well.

@ReneSchumacher
Copy link
Contributor Author

@RoniSegal As far as I can see this has already been fixed in v15.0.0. I just updated the package reference and the statement you mentioned in the issue showed up as a branch.

@coreyfarrell
Copy link
Member

@ReneSchumacher sorry for the slow response I've been very busy lately. I will review this as soon as I can.

@ReneSchumacher
Copy link
Contributor Author

Hi again,
any updates on this? I'd really like to get this off the table and have a couple people actually waiting for the fix.

@andrebriggs
Copy link

@coreyfarrell update?

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell Any chance this will be approved?

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell This is now open for almost half a year. :( Is there something missing or any other reason why this isn't considered? There are still many people waiting for the fix.

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coreyfarrell This is now open for almost half a year. :( Is there something missing or any other reason why this isn't considered? There are still many people waiting for the fix.

istanbuljs gets some funding through TideLift but it's not nearly enough for me to devote the level of attention that istanbuljs needs. It's an unfortunate situation but I have to prioritize my time towards things that pay the bills.

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell Sorry to bother again, but do you still need anything from me before you can merge this in?

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell Is there probably another maintainer who could look into this?

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell I know you have limited time to support this project but this PR, which fixes an obvious bug, is not ten months old. Is there any chance it will be accepted? There are more and more people hitting the issue and it becomes quite frustrating so see a fix sitting there with no one looking at it.

@ReneSchumacher
Copy link
Contributor Author

@coreyfarrell Any chance we can get this fix merged? It's been over a year now and I'm wondering if there aren't any other maintainers who might be able to help here.

@f2calv
Copy link

f2calv commented Jul 9, 2021

@coreyfarrell any update here?

@bcoe bcoe changed the title Fix source mapping for branch statements fix: source mapping for branch statements Oct 12, 2021
@bcoe
Copy link
Member

bcoe commented Oct 12, 2021

@ReneSchumacher thank you for the contribution, and sorry for the long time to land. As @coreyfarrell indicates, we haven't had a huge amount of time to contribute to this project the past couple years.

I've been making an effort to pitch in a bit more, and am landing a few old PRs.


As a follow up PR, does this need to be integrated with the Cobertura reporter?

@bcoe bcoe merged commit 3833708 into istanbuljs:master Oct 12, 2021
@andrebriggs
Copy link

2 years, wow

@ReneSchumacher
Copy link
Contributor Author

As a follow up PR, does this need to be integrated with the Cobertura reporter?

Hey @bcoe, thx for accepting my PR! Really happy to see this merged. I've tested the change locally and there wasn't any need to change the reporter.

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

Successfully merging this pull request may close these issues.

Missing branch coverage for switch statements in Cobertura report
7 participants