spec: opt-in line coverage for .lic scripts - #7547
Open
simtel12 wants to merge 3 commits into
Open
Conversation
`COVERAGE=1 bundle exec rspec` now produces a real coverage report in coverage/ (already gitignored). Three things were in the way: 1. The specs never require the .lic files -- load_lic_class evals a slice of them -- and Ruby only measures eval'd source when Coverage is set up with eval: true. Without that the report is 0% covered, which is what made it look like coverage simply did not work here. 2. The extractors built the path as "<root>/spec/../foo.lic", whose project-relative form is "spec/../foo.lic" -- which SimpleCov's default test-directory filter discards. Expanding the path via the new lic_path helper makes the default filters correct, so no filter surgery is needed. It also means backtraces point at the file's real path. 3. Ruby allocates a file's eval-coverage line array on the first eval attributed to that filename and never grows it. A multi-class .lic is eval'd once per class, so any class starting past the end of the first-eval'd one was silently dropped -- not counted as missed, just absent. combat-trainer.lic reported 2 of its 11 classes (791 relevant lines); it now reports all 11 (3724). prime_lic_coverage sizes the array once per file, and is a no-op unless Coverage is running. While wiring this up: automap_spec and moonwatch_spec each defined their own top-level load_lic_module, and dependency_spec built its own path. Two identical top-level defs in different spec files is the load-order hazard spec_helper's own header warns about, so load_lic_module moves to spec_helper and the three specs use the shared extractors. That also stopped automap.lic and dependency.lic being dropped from the report and moonwatch.lic losing its five modules. load_lic_constant now passes the constant's real line number to eval instead of defaulting to line 1. Suite is unchanged at 2922 examples with and without COVERAGE set.
Only the class/module bodies a spec extracts are ever eval'd, so the rest of a script -- the before_dying block, the `Klass.new` entry point, a top-level def, a multi-line constant -- had no coverage entry at all. That is not the same as being uncovered: those lines were dropped from the denominator entirely, which flattered every percentage. Backfill them at exit from SimpleCov's own static line classifier, the same one it uses for files that were never loaded. Real measurements always win; this only fills nil holes with 0. Across the 39 specced scripts this moves the denominator 16007 -> 16424 relevant lines with the covered count unchanged at 6336, so the reported figure goes 39.6% -> 38.6%. combat-trainer.lic now shows its before_dying cleanup and ct_safe_eval as missed rather than omitting them.
The static-classifier backfill was inventing missed lines inside code that demonstrably ran. SimpleCov's line classifier calls every element line of a multi-line literal relevant, while Ruby attributes the whole literal to a single line, so a constant like astrology.lic's OBSERVE_SUCCESS_PATTERNS came out with its opening line reported as missed even though the array is built (Ruby records the hit on the following line). Inside a range that was eval'd, Ruby is authoritative and the static classifier is not, so restrict the backfill to lines outside every range an extractor touched. The extractors now record those ranges in LIC_EVAL_RANGES. Drops 23 bogus missed lines: 16424 -> 16401 relevant, covered unchanged at 6336. astrology.lic keeps its before_dying block and `Astrology.new.run` marked missed, which is the point, but no longer claims its constants are.
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds opt-in line/branch coverage for the
.licscripts:Default runs are untouched — without
COVERAGEset, nothing here loads simplecov or does any extra work.Current picture across the 39 specced scripts: 6391 / 16401 lines (39.0%), branch 27.3%. The other 183 scripts have no spec at all.
Why this needed more than
SimpleCov.startI tried the obvious thing first and got a report claiming 0 covered lines. Three separate problems, each of which silently produces a wrong number rather than an error:
1. The specs never
requirethe scripts.load_lic_classevals a line-slice of the.lic, and Ruby only measures eval'd source when Coverage is set up witheval: true. Withoutenable_coverage :evalthe report is empty. Starting simplecov at the top ofspec_helper.rbis early enough —.rspec's--require spec_helperruns before any*_spec.rbloads, so every extractor call still lies ahead of it.2. The extractors built the path as
<root>/spec/../foo.lic. Its project-relative form isspec/../foo.lic, which simplecov's default test-directory filter discards — so the scripts were being thrown out of their own coverage report. The newlic_pathhelper expands the path, after which the default filters are correct and no filter configuration is needed. It also means backtraces from eval'd code point at the file's real path.3. Ruby sizes a file's eval-coverage line array on the first eval attributed to that filename and never grows it. A multi-class
.licis eval'd once per class, so any class starting past the end of the first-eval'd one was dropped — not counted as missed, just absent.combat-trainer.licwas reporting 2 of its 11 classes.prime_lic_coveragesizes the array once per file up front; it is a no-op unlessCoverage.running?.Un-eval'd code is now counted as missed
Only the class/module bodies a spec extracts ever run, so the rest of a script — the
before_dyingblock, theKlass.newentry point, a top-leveldef— had no coverage entry at all. That is not the same as being uncovered: those lines were dropped from the denominator, which flattered every percentage. They are now backfilled from simplecov's static line classifier (the same one it uses for files that were never loaded) at ~390 lines across the suite.The backfill is deliberately restricted to lines outside every range an extractor eval'd, recorded in
LIC_EVAL_RANGES. Inside an eval'd range Ruby is authoritative and the static classifier is not: it calls each element line of a multi-line literal relevant, while Ruby attributes the whole literal to one line. Without the restriction,astrology.lic'sOBSERVE_SUCCESS_PATTERNShad its opening line reported missed while the next line showed a hit — 23 invented missed lines across the suite.Extractor consolidation
automap_specandmoonwatch_speceach defined their own top-levelload_lic_module, anddependency_specbuilt its own path. Two identical top-level defs in different spec files is exactly the load-order hazardspec_helper.rb's own header warns about, soload_lic_modulemoves into spec_helper and the three specs use the shared extractors.That was not only tidiness — it was also why
automap.licanddependency.licwere missing from the report entirely andmoonwatch.licwas losing its five modules.Two smaller fixes while in here:
load_lic_constantpasses the constant's real line number toevalinstead of defaulting to line 1.load_lic_constantusesindexrather thanfind, so the line number is available.Note on the dependency
simplecovis added to the:developmentgroup. It is only loaded whenCOVERAGEis set, but it does become a bundle dependency. If that is unwelcome I am happy to move the whole thing behind an optional group instead — say the word.Test plan
bundle exec rspec— 2977 examples, 0 failures (unchanged from main)COVERAGE=1 bundle exec rspec— same 2977 examples, pluscoverage/index.htmlbundle exec rubocop spec/— cleancoverage/index.html:combat-trainer.licshows all 11 classes, and itsbefore_dyingblock reads as missed rather than being absent