rspec / rspec-core

RSpec runner and formatters
http://rspec.info
MIT License
1.23k stars 765 forks source link

SimpleCov execution in rspec-core builds is sneakily broken #3083

Closed nevinera closed 6 months ago

nevinera commented 6 months ago

The current build appears to be attempting to run simplecov (it goes to some trouble), and enforce a 100% coverage rule against it, but if you look through the build output of a recent build on the main branch, you'll find a line like this:

Coverage report generated for RSpec to /home/runner/work/rspec-core/rspec-core/coverage. 2 / 2 LOC (100.0%) covered.

100% is good! But that appears to only be 100% of the two lines in the exe/rspec file, rather than the entire rspec-core gem, and I'm not sure why. When I run it locally using a more conventional RSpec::Support::Spec.setup_simplecov as we use in rspec-expectations, I see that we're closer to 98%, with the most significant missing coverage on lib/rspec/core/bisect/fork_runner.rb (all of RunDispatcher#run_specs).

Screenshot 2024-05-09 at 6 32 39 PM

Is anyone confident that we actually still need this wrapper script? It was introduced in 2013, and simplecov (and Coverage itself) have come a long way since then. Certainly my experience running simplecov locally against rspec-core doesn't match the experience this script was introduced to solve as described in the introducing commit:

Simplecov was only reporting coverage for a handful of rspec's files, and I realized why

I think the simplest answer here is to just get rid of it, and use the same setup approach the other gems use; we'd get to trim a fair bit of code that way. We'll need to adjust the coverage target downward until I can get more tests in again, but that shouldn't take long; there's less uncovered than rspec-expectations had already (or I can introduce the tests first if you'd rather, and get coverage to 100% before fixing the simplecov setup)

nevinera commented 6 months ago

Certainly my experience running simplecov locally against rspec-core doesn't match the experience this script was introduced to solve

Or maybe it does - I now notice that that simplecov output shows coverage info for 31 files, while git lists 75 files in lib. So never mind about that option at least, I need to work out how to get simplecov working correctly I guess -.-

nevinera commented 6 months ago

Aha! At some point, simplecov must have changed the behavior of their filters. the spec/ filter is also matching everything lib/rspec/, which is like.. everything.

pirj commented 6 months ago

spec/ filter is also matching everything lib/rspec/, which is like.. everything

Nice catch! 😅

If you feel that the wrapping script doesn’t add value, let’s remove it. Please keep in mind that we have a “host” repo rspev-dev, keep some common code/config/docs there, and update this in a centralizzed way. Yet another reason we’re moving to a monorepo. So the wrapping coverage script is coming from there, we’ll have to sync this removal across all repos (main four actually, and potentially rspec-rails).

pirj commented 6 months ago

It doesn’t seem that the simplecov wrapper script was coming from a host repo. Thank you!

nevinera commented 6 months ago

The wrapping script did turn out still to matter - the number of files simplecov reported on without it was substantially lower, it was just still large enough that I couldn't tell until I started counting files.

That took a little more work than I expected, because there are a surprising number of files that actually don't get loaded while running the test suite against cruby, but checking into each of the missing items by hand was reassuring :-)

JonRowe commented 6 months ago

Only rspec-core needs the script because of the chicken and egg problem, so its only in this repo