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

Consider embedded input source map in file coverage #87

Merged

Conversation

MichaReiser
Copy link
Contributor

When locating the source map, use the source map included in the file coverage whenever possible. Closes #2 but requires istanbuljs-archived-repos/istanbul-lib-instrument#23

@codecov-io
Copy link

codecov-io commented Sep 6, 2016

Current coverage is 97.44% (diff: 75.00%)

Merging #87 into master will decrease coverage by 0.22%

@@             master        #87   diff @@
==========================================
  Files            13         13          
  Lines           428        430     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            418        419     +1   
- Misses           10         11     +1   
  Partials          0          0          

Powered by Codecov. Last update a990033...8558307

Copy link
Contributor

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Overall this looks reasonable, though there's a mix of spaces and tabs. Please update withe use of tabs for consistency and then we'll test and land.

@MichaReiser
Copy link
Contributor Author

I will apply the requested changes as soon as istanbuljs-archived-repos/istanbul-lib-instrument#23 is landed

@MichaReiser MichaReiser force-pushed the consider-sourcemap-from-file-coverage branch from f50c1f0 to 96911f6 Compare November 7, 2016 10:15
@MichaReiser
Copy link
Contributor Author

Rebased and spaces converted to tab... Feature is going to land in istanbul this week!

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Nov 10, 2016

This is now an officialy supported feature of istanbul....

@dpogue
Copy link

dpogue commented Nov 28, 2016

@dylans Any chance of re-reviewing and merging this? This looks to be the final piece in the long saga of getting working code coverage for transpiled code.

@MichaReiser
Copy link
Contributor Author

First another rebase is needed. However, I'm not going to rebase again if there is no chance that the change is going to be merged any time soon. Because I don't want to spend meaningless hours rewriting / rebasing the code without any chance that the change is accepted.... even if the feature is urgently needed by all using a transpiled language

@jdonaghue
Copy link
Contributor

@MichaReiser if you do have anytime to rebase this, I will land it right after you do so. Thanks for your work!

@MichaReiser MichaReiser force-pushed the consider-sourcemap-from-file-coverage branch from 96911f6 to a990033 Compare November 28, 2016 21:40
When locating the source map, use the source map included in the file
coverage whenever possible. Closes SitePen#2
@MichaReiser MichaReiser reopened this Nov 28, 2016
@@ -91,7 +93,7 @@ class CoverageTransformer {
}
}

if (!match || !rawSourceMap) {
if (!rawSourceMap) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure if this is right this way... But i think this check is sufficient since we only need to have a source map (we dont care about a match... just throw an error if we could not find the source map)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right we don't need that check

@MichaReiser
Copy link
Contributor Author

@jdonaghue The change is rebased.... Not sure how to add tests though...

@jdonaghue
Copy link
Contributor

This looks good @MichaReiser thanks for your work! Landing this now.

@jdonaghue jdonaghue merged commit 5bf2181 into SitePen:master Nov 29, 2016
@MichaReiser
Copy link
Contributor Author

MichaReiser commented Nov 29, 2016

Yeah finally... A long story finally ends :)

When can we expect a release to npm?

@leduyminh48
Copy link

leduyminh48 commented Dec 1, 2016

Uraaaaaaaaaaaaaa, I had the exact problem when sourceMappingUrl was not generated but inputSourceMap was included in coverage instead, fixed the remap.js locally. So happy that this is already done by someone else.
Waiting for a release

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.

6 participants