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

Aggregate coverage from calls to subprocess #68

Closed
mojavelinux opened this issue Apr 14, 2020 · 19 comments
Closed

Aggregate coverage from calls to subprocess #68

mojavelinux opened this issue Apr 14, 2020 · 19 comments

Comments

@mojavelinux
Copy link
Contributor

I have several CLI tests in my test suite. The lines covered by these tests are not being reflected in the coverage report. How do I get the coverage data collected from these calls to be incorporated into the coverage report? Is there an additional setting or environment variable that I need to set?

Here's the test suite: https://github.com/asciidoctor/asciidoctor-pdf

@marcandre
Copy link
Member

marcandre commented Apr 23, 2020

Sorry for the delay. I can have a look.

IINM I need to run rake coverage test, right?

The first 3 uncovered spots I checked aren't indeed run, so which lines are you referring to?

@mojavelinux
Copy link
Contributor Author

You need to run:

COVERAGE=deep rspec

Or you can just focus on the CLI tests:

COVERAGE=deep rspec spec/cli_spec.rb

(no need to use Rake)

@mojavelinux
Copy link
Contributor Author

I'll provide a specific example for what I know is hit, but isn't covered. Stay tuned.

@mojavelinux
Copy link
Contributor Author

Btw, I also frequently get core dumps when using deep cover, which is maybe associated with using a child process or maybe not related.

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Apr 23, 2020

Oh yeah, and I just remembered you can use the focused test because that leads to:

ree/util.rb:19:in `block in populate_from_map'
	 2: from /home/dallen/projects/asciidoctor/pdf/.bundle/gems/ruby/2.7.0/gems/deep-cover-core-0.7.10/lib/deep_cover/reporter/base.rb:39:in `block in populate_stats'
	 1: from /home/dallen/projects/asciidoctor/pdf/.bundle/gems/ruby/2.7.0/gems/deep-cover-core-0.7.10/lib/deep_cover/reporter/html/index.rb:14:in `block in stats_to_data'
/home/dallen/projects/asciidoctor/pdf/.bundle/gems/ruby/2.7.0/gems/deep-cover-core-0.7.10/lib/deep_cover/reporter/html/index.rb:52:in `transform_data': undefined method `transform_values' for "value_executed_loc_keys":String (NoMethodError)

^ It seems like deep cover fails in almost all cases when I try to filter the tests. I get all kinds of weird errors in the transform data routine.

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Apr 23, 2020

So here's a case. If I run the following command:

COVERAGE=deep bundle exec rspec -t cli

Then it says the truthy branch on line 222 (if use_title_page) is not covered. But if I write a debug statement to a file inside that condition, the file is written. So the code is clearly going there, but deep cover doesn't think so.

It appears some hit lines are recorded, but not all. It seems like there are logic errors in the merging of the coverage data.

@marcandre
Copy link
Member

The reporting takes forever 😮
I have to check what's taking so long...

@mojavelinux
Copy link
Contributor Author

The reporting takes forever

Tell me about it. But given how vital the information is, I've learned to be patient ;) If it could go faster, I would be 🙏.

@marcandre
Copy link
Member

Ah, I think I found the issue; should be in the HTML reporter (and only the HTML reporter), and it makes sense too. I'm not sure yet of what approach I can take, I'll keep you updated.

@mojavelinux
Copy link
Contributor Author

That's fantastic and encouraging to hear! Thanks for the update.

@marcandre
Copy link
Member

I'm both quite ashamed and happy to report that, testing with some of your code (~500 loc), I get the reporting down from 56 seconds down to 0.8 seconds. Doubling the LOC quadruples the original time to a ridiculous ~4 minutes, while the optimized time is ~15% slower than twice the time at 1.9 seconds.

This is obtained by optimizing Parser::Source::TreeRewriter (no change to DeepCover per say). I need to tweak the code for older rubies (I used bsearch_index) and submit a PR to the Parser crew, but they are usually amazingly responsive.

Now I have to figure out what's going on with the actual coverage, but at least I don't have to wait minutes for the report...

@marcandre
Copy link
Member

Created whitequark/parser#683 ...

@marcandre
Copy link
Member

Updating parser should speed up the process a lot. I hope to look at the actual issue very soon...

marcandre added a commit that referenced this issue May 7, 2020
@marcandre
Copy link
Member

So I can confirm that the issue is that the code running via run_command isn't loading DeepCover. I put a raise 'hey, gotta load DeepCover' unless defined?(DeepCover) in converterd.rb and it fired when running the CLI tests. Our instructions weren't explicit about this, sorry, this so I've tried making it more precise.

In your case, since the code is loaded by asciidoctor, I'd recommend adding require 'deep-cover' if ENV['DEEP_COVER'] at the beginning of your main entry point lib/asciidoctor/pdf.rb and things should work fine and counters from both processes should be merged automatically. (you'll notice that you will have two .dct files in deep_cover instead of just one like currently).

Let us know if it doesn't work as it should.

@mojavelinux
Copy link
Contributor Author

@marcandre Aha! That makes a lot of sense.

I'd recommend adding require 'deep-cover' if ENV['DEEP_COVER'] at the beginning of your main entry point lib/asciidoctor/pdf.rb

Eek! Testing code in production code. Not that. But I see what you mean. I'll use a supplemental script to load it.

Thanks for the pointers! I can't wait to see what the real numbers are.

@mojavelinux
Copy link
Contributor Author

FYI, somehow nyc is able to do this in the Node world without the explicit require. I suspect they are doing some command manipulation. That's why I had just assumed deep cover would be loaded inside the CLI invocation.

@marcandre
Copy link
Member

Interesting. I don't know how nyc does it. I believe that in the Ruby world it can't be done; simplecov for example needs something done (and not just a require).

@mojavelinux
Copy link
Contributor Author

I'm pretty sure nyc intercepts the subprocess communication (though I'm fuzzy on the details at the moment).

@mojavelinux
Copy link
Contributor Author

mojavelinux commented May 8, 2020

It seems to use NODE_OPTIONS to force itself to be required when the subprocess is called (assuming the subprocess is Node). This is similar to how Bundler bootstraps itself in a Ruby subprocess.

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

No branches or pull requests

2 participants