Gcov plugin: version-gating/arg-building refactor, bug fixes, unit tests - #1261
Merged
Conversation
Code review of plugins/gcov/lib/ against its own docs and against gcovr's and ReportGenerator's real CLI/version history turned up several confirmed bugs and a DRY opportunity spanning the two report-generation backends. Bug fixes (each verified against real tools, not just documentation): - :gcov ↳ :gcovr ↳ :decisions version floor corrected 6.0 -> 5.1 -- gcovr's own changelog and a real gcovr 5.1/5.0 pip install confirm --decisions was introduced in 5.1, not 6.0 as the recent #1080 fix stated. - :fail_under_* now rejects 0 (documented range is 1-100; 0 was silently accepted). - :object_directory/:source_encoding values are now quoted, matching every other string-valued gcovr option -- an unquoted value containing a space previously broke the constructed command line. - Config-driven values (test/mock prefixes, build root) are now Regexp.escape'd before being spliced into exclusion-pattern regexes, in both gcovr_reportinator.rb and reportgenerator_reportinator.rb. - :mcdc's GCC-version check moved out of Gcov#setup(), which runs for every build the plugin is merely enabled for, into a lazily-invoked, memoized check that only fires on a real gcov:-context compile/link -- consistent with this same file's existing lazy-validation rationale for the gcovr/ ReportGenerator Reportinators themselves. - gcovr_exec_exception? now reports every simultaneously violated :fail_under_* threshold instead of only the first. - Added the missing .dup guard in ReportGeneratorReportinator#collect_reportgenerator_opts, mirroring the existing guard on the gcovr side. - gcovr 7.0+ deprecated flag migration: :branches/:sort_uncovered/ :sort_percentage now use --txt-metric branch/--sort uncovered-number/ --sort uncovered-percent at gcovr 7.0+, silencing gcovr's own deprecation warnings; the pre-7.0 names are still used automatically below that. One audit finding was investigated, disproven, and reverted rather than shipped: ReportGenerator's `settings:` arguments were flagged as missing a leading '-' to match every sibling option. A real reportgenerator run proved the opposite -- a leading '-' makes it log "Unknown command line parameter 'settings'" and the setting never applies; the original dash-less form is correct and unchanged. Refactor: two generic, shared utilities on GcovReportinator, replacing repeated hand-written version-gate/argument-string boilerplate with one declarative table per reportinator -- - ToolVersionGating (gcov_types.rb): generalizes the gcovr-only GcovToolVersion struct and min_version? into a reusable ToolVersion + detect_tool_version/enforce_version_gates!, used by GcovrReportinator's own gcovr-version gates and by Gcov's GCC-version gate alike. detect_tool_version is also the one seam a unit test now stubs directly, instead of faking a full tool_executor.build_command_line/.exec/regex-parse round-trip. - build_args_from_table (gcov_reportinator.rb): a declarative option-symbol -> CLI-flag table drives both gcovr's args_builder_common and ReportGenerator's build_optional_args, replacing ad hoc string concatenation with plain, readable data (including handling for a version-dependent flag name, via a Proc, for the deprecated-flag migration above). - build_custom_args lifted to the shared base class (was byte-identical in both reportinators). Unit tests: net-new coverage for a plugin that had none before -- only real tools -- gcov/gcovr/reportgenerator -- are ever exercised via system tests; `@tool_executor.exec` is always stubbed here. - spec/units/plugins/gcov_reportinator_spec.rb: the shared base-class utilities. - spec/units/plugins/gcovr_reportinator_spec.rb: version gating, args_builder_common, config-file exclusion warning, exclusion-regex escaping, multi-violation exec-exception reporting. - spec/units/plugins/reportgenerator_reportinator_spec.rb: build_optional_args (including a regression test locking in the settings: dash-less form above), the .dup mutation guard, exclusion-regex escaping. - spec/units/plugins/gcov_spec.rb: the lazy, memoized :mcdc/GCC-version check. Verified: full unit suite (2991 examples, 0 failures), mkdocs build --strict, and the full Gcov system-test suite (26 examples, 0 failures, 2 pending -- gdb unavailable, unrelated) run against real gcc/gcovr/reportgenerator via throwtheswitch/madsciencelab-plugins Docker. Changelog entries intentionally withheld from this commit -- reserved for a follow-on documentation-only pass alongside separately-deferred doc corrections, and for splitting between a master-branch 1.1.8 port and next_version's own 1.2.0 entry once CI has run on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's coverage report on PR #1261 revealed the Gcov plugin's just-added unit coverage was still thin relative to file size. This adds ~172 new examples targeting core branching, parsing, and dispatch logic across the four under-covered files -- explicitly avoiding tests that just mirror a single line of production code back as an assertion. Coverage: console_reportinator.rb 18% -> 90% (15/81 -> 75/83) gcov.rb 22% -> 79% (40/182 -> 143/182) gcovr_reportinator.rb 44% -> 93% (88/200 -> 185/200) reportgenerator_reportinator.rb 59% -> 85% (67/113 -> 97/114) gcov_reportinator.rb 86% -> 93% (36/42 -> 39/42, untouched -- already well covered) New spec file: spec/units/plugins/console_reportinator_spec.rb (24 examples) covering the real gcov-text-output parsing logic (remap_partial_sources, extract_gcov_source_path, log_coverage_report, run_gcov_summary) that had zero coverage before. Extended spec/units/plugins/gcov_spec.rb (+41 examples): process_untested_sources (the largest single gap -- all three :ignore/:list/:compile modes, MC/DC flag injection, ShellException guidance handling), generate_coverage_reports (reportinator memoization, per-reportinator exception isolation as a build failure rather than aborting the whole build), post_build's gating chain, build_reportinators' utility-to-class dispatch, validate_untested_sources/ validate_utilities_config, collect_untested_sources, post_test_fixture_execute, and the MC/DC-flag-injection pre_test_compile_register/pre_test_link_register paths. Deliberately excludes thin one-line predicates, pure field-copies, and setup() itself (would need an enormous collaborator double for wiring already covered piecemeal by the extracted methods above). Extended spec/units/plugins/gcovr_reportinator_spec.rb (+39 examples): the four args_builder_* report-type methods, generate_reports_modern's skip-when-nothing- enabled branch, collect_gcovr_opts' config-file-vs-normal exclusion handling, run_gcovr's exception/summary flow, and the two previously-untested pure text parsers extract_gcovr_error_message/extract_gcovr_summary (including the multiple-summary-blocks-picks-the-last-one case a naive test would get wrong). Extended spec/units/plugins/reportgenerator_reportinator_spec.rb (+10 examples): build_report_types, and -- after a small testability refactor (below) -- run_gcov's gcov-output filename-extraction/rename regex logic (the biggest previously-untested gap in this file) and generate_gcov_files' directory discovery/sort/exclude logic. Two small production changes, made while designing the above: 1. ConsoleReportinator#log_coverage_report: a Partial-implementation source whose gcov output can't be matched at all previously produced no report AND no log -- silently different from the equivalent non-Partial "found no coverage results" case just below it. Added the same COMPLAIN log to that branch. 2. ReportGeneratorReportinator previously called Dir.glob/File.exist?/ File.rename directly in generate_gcov_files, run_reportgenerator, and run_gcov -- real filesystem I/O with no mockable seam, which is exactly why those methods had zero unit coverage. Routed all of it through FileWrapper (lib/ceedling/file_wrapper.rb), the same DI-friendly abstraction Gcov itself already uses: Dir.glob -> @file_wrapper.directory_listing, File.exist? -> @file_wrapper.exist?, File.rename -> @file_wrapper.mv. Added @file_wrapper = @ceedling[:file_wrapper] to initialize. No unit spec in this PR touches a real file or directory. Verified: full unit suite (3107 examples, 0 failures), mkdocs build --strict, and the full Gcov system-test suite (26 examples, 0 failures, 2 pending -- gdb unavailable, unrelated) run against real gcc/gcovr/reportgenerator via throwtheswitch/madsciencelab-plugins Docker, confirming the FileWrapper refactor and the log_coverage_report fix behave identically end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…reshold parity
Investigated the deferred items that were genuine open bugs/questions
(memory: gcov_plugin_review_deferred_items.md), fixed what's confirmed.
:mcdc dead-code question -- resolved, confirmed genuinely dead code:
GcovrReportinator#collect_gcovr_opts set an internal _opts[:mcdc] = true
flag that no args_builder_* method ever read. A prior session confirmed via
Docker that gcovr's JSON output shows GCC condition/MC-DC data identically
with or without --decisions; this session extended that check to HTML
specifically (the open half of the question) using a real gcovr 8.6 (the
plugin's own :mcdc version floor) against a real multi-condition
-fcondition-coverage build: --decisions produces a byte-identical
Condition-labeled HTML report, only adding a wholly separate Decision
column alongside it. Removed the dead line, with a comment recording why
gcovr needs no :mcdc-specific handling of its own at all.
:boom exception-handling "inconsistency" -- investigated, confirmed intended
behavior (console/reportgenerator's boom:false vs. gcovr's raise-and-rescue
each correctly match their own tool's actual failure semantics), not
touched.
ReportGenerator coverage-threshold parity -- new feature:
Added ReportGenerator's own -minimumCoverageThresholds:* settings (RG
v5.1.23+), the direct RG analogue of gcovr's :fail_under_* family: four new
:gcov ↳ :report_generator options (:fail_under_line, :fail_under_branch,
:fail_under_method, :fail_under_full_method), 1-100 validated the same way
gcovr's are. Implementation: a new declarative :inline_integer_percent type
on GcovReportinator#build_args_from_table (combines :inline_value's
flag-and-value-glued-together shape with :integer_percent's range check),
and a REPORT_GENERATOR_THRESHOLD_ARGS table folded into build_optional_args
alongside the existing settings: handling.
One flag-syntax mistake caught before it shipped, same pattern as this
branch's earlier settings: dash question: minimumCoverageThresholds: was
initially written with a leading '-' to match the other 1:1 options'
convention. A real reportgenerator run immediately rejected it ("Unknown
command line parameter 'minimumCoverageThresholds'"); the dash-less form
--- matching settings:'s own established exception --- is correct. Caught
and fixed via the exact same Docker verification that then went on to
confirm a configured threshold violation genuinely fails the build (exit 1,
BUILD FAILURE SUMMARY) and a passing one doesn't (exit 0) --- no new
:exception_on_fail toggle needed since run_reportgenerator already leaves
:boom at its default (true), so a threshold violation was already going to
raise and already gets caught by Gcov#generate_coverage_reports's existing
per-reportinator exception isolation.
Small doc additions for the four new options in
docs/mkdocs/plugins/gcov/reportgenerator.md and
docs/mkdocs/reference/gcov-plugin.md (documenting what's new; the broader
doc-corrections pass is a separate, later step).
Verified: full unit suite (3112 examples, 0 failures), mkdocs build
--strict, full Gcov system-test suite (26 examples, 0 failures, 2 pending
-- gdb, unrelated) via madsciencelab-plugins Docker, plus manual Docker
confirmation of both a threshold-violating and a threshold-passing
ReportGenerator build.
No Changelog entries in this commit -- reserved for a later step once
master-branch porting is scoped (see updated deferred-items memory).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Code review of
plugins/gcov/lib/against its own docs and against gcovr's/ReportGenerator's real CLI and version history, per a planned pass covering confirmed bugs, a DRY refactor of the shared version-gating and argument-building logic, and net-new unit test coverage (previously zero) for the plugin.Bug fixes (each verified against real tools, not just documentation)
:gcov ↳ :gcovr ↳ :decisionsversion floor corrected 6.0 → 5.1 — confirmed via a real gcovr 5.1/5.0 pip install (5.1 accepts--decisions, 5.0 rejects it as unrecognized).:fail_under_*now rejects0(documented range is 1–100;0was silently accepted).:object_directory/:source_encodingvalues are now quoted, matching every other string-valued gcovr option.Regexp.escaped before being spliced into exclusion-pattern regexes, in both reportinators.:mcdc's GCC-version check moved out ofGcov#setup()(ran for every build the plugin was merely enabled for) into a lazy, memoized check that only fires on a realgcov:-context compile/link.gcovr_exec_exception?now reports every simultaneously violated:fail_under_*threshold, not just the first..dupguard inReportGeneratorReportinator#collect_reportgenerator_opts, mirroring the existing gcovr-side guard.--branches→--txt-metric branch, etc.), silencing gcovr's own deprecation warnings on newer gcovr while still using the pre-7.0 names below it.One audit finding (ReportGenerator
settings:missing a leading-) was investigated, disproven, and reverted rather than shipped — a realreportgeneratorrun showed the leading-breaks it; the original dash-less form is correct.Refactor
Two generic, shared utilities on
GcovReportinator, each with a declarative table per reportinator:ToolVersionGating(version detection + gating), also used byGcov's own GCC-version check.build_args_from_table(declarative CLI-arg building), replacing hand-written string concatenation in bothargs_builder_commonandbuild_optional_args.Tests
4 new spec files, 59 new examples. Full unit suite: 2991 examples, 0 failures. Full Gcov system-test suite via
throwtheswitch/madsciencelab-pluginsDocker: 26 examples, 0 failures, 2 pending (gdb unavailable, unrelated).Changelog
Intentionally withheld from this PR — reserved for a follow-on documentation-only pass, and for splitting between a
master1.1.8 port andnext_version's own 1.2.0 entry once CI has run here.🤖 Generated with Claude Code