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

Code coverage with sourcemaps support #178

Closed
Marsup opened this issue Sep 3, 2014 · 14 comments
Labels
Milestone

Comments

@Marsup
Copy link
Member

@Marsup Marsup commented Sep 3, 2014

Hi,

I'm mostly working on projects with ES6 transpilation nowadays.
Errors produced in the tests are finely reported thanks to source-map-support, the only thing I'm missing is the equivalent for code coverage lines.
So I'd like to discuss the idea before looking into it myself in case you wouldn't want it.
Thoughts ?

@arb arb added the discussion label Sep 3, 2014
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 3, 2014

What do you need for lab to start doing? Are you using the es6-transpiler?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 3, 2014

I'm using traceur, but that's more related to how sourcemaps are produced actually.

I think most of the code needed is already in source-map-support, since it does line matching on exceptions. Lab should have a different behavior when grabbing uncovered lines if there is a sourcemap associated to the file (inside as a comment or as a separate file).

@geek geek added request and removed discussion labels Sep 3, 2014
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 5, 2014

Can you provide an example of generated output for the sourcemaps you are looking to use. When looking at https://www.npmjs.org/package/source-map the consumer needs the sources, names, and mappings. In other words, how is the source-map referenced, as a separate file? How do you know what sourcemap to load for a given script?

How do you want the coverage reporter to change, just add the original line numbers next to any lines that aren't covered?

Marsup added a commit to Marsup/lab that referenced this issue Sep 6, 2014
Marsup added a commit to Marsup/lab that referenced this issue Sep 6, 2014
Marsup added a commit to Marsup/lab that referenced this issue Sep 6, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 6, 2014

I have done the patch that provides the functionality, it's pretty straightforward but it introduces a < 1.x dependency and I know we have a policy of not doing it, so are we making an exception ?

So far it only adds the original file name and line number to the line details, that's left to the reporters to decide what to do with it.

See the full diff here. I have included both kinds of sourcemaps, as external file and inline, they are just a transpilation from while.js. For some reason inline doesn't give the same original file name, I have opened an issue on their project about that.

I think the reporter should change to only show original lines, not the transpiled ones, maybe letting the user know it did a sourcemaps conversion.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 9, 2014

I am not opposed to this feature, maybe we can make it an opt-in feature... this way for the common case source-map isn't trying to analyze each test script. Thoughts?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 9, 2014

That could be done, but is lab really in anyone's critical path ? After all that's just a readFile and regexp lookup on the common case, that should be negligible.

I'm OK with either ways but I'm not sure it should be user-controllable, it's transparent and doesn't affect the results if there's no need.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 10, 2014

I am more concerned about it introducing another layer to debug when tracking down bugs in lab. Plus, for most people it won't be used so it doesn't hurt to avoid the extra execution time.

Maybe in the near future we can enable it by default, but for now for a minor release lets keep it off by default.

@geek geek added this to the 4.4.0 milestone Sep 10, 2014
@geek geek added the feature label Sep 10, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 10, 2014

OK, I'll try to finish the patch asap with that in mind.

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 10, 2014

@geek Most of the letters are taken for cli, any preference ? 😀

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Sep 10, 2014

hmm... could do -M for map, but I don't have a preference on the flag

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 10, 2014

I think I did most of the code but I have a dilemma concerning the tests. Coverage can't be properly tested with the CLI given that the tests are in test directory, and from what I gathered it is not even tested. Should I create test files in another directory ?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Sep 11, 2014

Look at some of the other coverage tests. We mess with the rules.

@geek geek removed this from the 4.4.0 milestone Sep 12, 2014
Marsup added a commit to Marsup/lab that referenced this issue Sep 13, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Sep 13, 2014

This should be OK with this last commit. I have taken care of console and html reporters, I don't think it can fit in lcov format, and others don't seem to have this kind of information.
Shall I go into pull request mode for review ?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Sep 13, 2014

You can always do a PR. They are free!

@geek geek modified the milestones: 4.5.0, 4.4.4 Sep 15, 2014
@geek geek closed this in 47c532c Sep 22, 2014
geek added a commit that referenced this issue Sep 22, 2014
Fix #178: Add sourcemaps support
@Marsup Marsup removed the request label Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.