Skip to content

Conversation

@robinetmiller
Copy link
Contributor

@robinetmiller robinetmiller commented Sep 11, 2025

While #852 adds code coverage, it misses coverage of 5 files:

lib/json.rb
lib/json/common.rb
lib/json/version.rb
lib/json/ext.rb
lib/json/ext/generator/state.rb

This occurs because the Coverage module (and therefore SimpleCov) need to be started before any require statement to notice a file. SimpleCov itself requires JSON, which causes the above files to be skipped in coverage metrics.

From what I can tell, there are three workaround options:

Option 1
Don't use SimpleCov for the measurement. Just use Coverage module directly and use SimpleCov to only format the output afterward.

The downside to Option 1 is that it loses the ancillary features that SimpleCov provides, That said, the main advantage is that it's the least invasive option.

Option 2
Patch Kernel.require to skip require 'json' statements coming from SimpleCov, but otherwise behave normally.

This option has the obvious drawback of (lightly) messing with Kernel, but it cleans up the use of SimpleCov to its standard API. This might be better for future maintainers to control this tool's behaviour.

Option 3
Mess around with defined constants, $LOADED_FEATURES, and/or $LOAD_PATH to force Ruby to actually run the require twice.

This is what Docile does to get around the same require-sequencing problem.

Some of my earlier experiments were to try to get SimpleCov to load the Ruby built-in JSON while the test runner used the head copy (using $LOAD_PATH shenanigans and require_relative), but that ran into namespace collisions, overrides, and complaints from the C bindings about pointing at the wrong thing. This spooked me off of Option 3's approach; it might be able to be done, but I personally do not have enough experience with Ruby C bindings to have a good feel for how this could break. Docile doesn't use C bindings at all, so they don't have this concern; plus, it gets used in the setup phase, so Option 2 is unavailable to them anyway.

I've implemented both Option 1 and Option 2 in 18150e0 and 11dde98, respectively, so that the core JSON team has a choice about which approach is preferred. Both of them will produce coverage on all files, solving the original problem.

Option 3 is not provided for the reasons cited above, but could be if someone more familiar with the C bindings can assure me that it wouldn't be fragile.

@robinetmiller robinetmiller changed the title Add coverage Add coverage on all files Sep 11, 2025
@robinetmiller
Copy link
Contributor Author

Not sure why the 3.0 series are failing, but I'm out of time this week to look into it. (Is it worth checking against less than 3.2 though?)

I can come back later if needed.

byroot added a commit to byroot/json that referenced this pull request Sep 11, 2025
Fix: ruby#853

Simplecov end up requiring json so we need to start collecting
coverage before.
@byroot
Copy link
Member

byroot commented Sep 11, 2025

Simpler solution: #855

@byroot byroot closed this in #855 Sep 11, 2025
matzbot pushed a commit to ruby/ruby that referenced this pull request Sep 12, 2025
Fix: ruby/json#853

Simplecov end up requiring json so we need to start collecting
coverage before.

ruby/json@ca72019fd3
byroot added a commit to RubyElders/json that referenced this pull request Oct 27, 2025
Fix: ruby#853

Simplecov end up requiring json so we need to start collecting
coverage before.
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.

2 participants