ci: add code coverage

[Aryex]

Summary

This PR add SimpleCov as the line coverage tooling.

Related

closes #288

Follow-up issue #307

[currantw]

A few comments for suggestions. Looks good! ✅

[currantw]

In C#, I implemented a "rachet" mechanism for code coverage (see #384).

Basically, when coverage runs as part of CI, it fails if the measured coverage is less than these thresholds – and it fails if the measured coverage is more than these thresholds. To fix this, the user needs to increase these minimum coverage thresholds (e.g. from 40% to 41%) to match the actual measured coverage. That ensures that the coverage should only ever go up (or, at the very least, that a developer would need to explicitly decrease it when merging a PR, and so would theoretically have to justify why the coverage was going down).

Would it be possible to do something similar here?

This obviously isn't a silver bullet, but it should lead to test coverage gradually ticking up over time, even without dedicated efforts to improve it. Since I merged that "rachet" mechanism back in June, line coverage has increased from 70.6% to 76.5% and branch coverage from 50.9% to 61.7%.

[currantw]

Are you planning to use this? If there isn't a clear rationale for uploading this now, I think I would avoid it?

[xShinnRyuu]

reference_config is never defined. Its assignment is inside the comment on lines 18 to 20, so Ruby parses this as a method call and raises NameError. SimpleCov catches that, prints a warning, and carries on.

Two things follow. Line 26 never runs, so no coverage threshold is ever applied on any leg. And this line is the Rubocop offence that makes CI red.

I checked this by running the suite at this head. The warning is the first line of output on every test job:

Warning: Error occurred while trying to load .simplecov.
Error message: undefined local variable or method 'reference_config' for module SimpleCov

To show the block is unreachable I raised the threshold to line: 100, branch: 100 and ran the cluster group, so project_total was true:

$ COVERAGE=1 COV_GROUP=cluster bundle exec ruby -Itest -Ilib test/unit/route_test.rb
Line coverage: 2575 / 2983 (86.32%)
exit 0

Demanding 100% passes. Adding reference_config = true and changing nothing else makes the same config fail correctly:

Line coverage (86.32%) is below the expected minimum coverage (100.00%).
SimpleCov failed with exit 2 due to a coverage related error
exit 2

So the mechanism works and only the definition is missing. Uncommenting lines 18 to 20 fixes both the dead threshold and the lint.

One warning: bundle exec rubocop -A will clear the lint by folding this into a modifier if. That leaves the NameError in place and makes the bug harder to spot. Please fix the variable rather than autocorrecting the style.

[xShinnRyuu]

Two problems in the on: block, both worth fixing before merge.

The nightly schedule trigger is gone, and that looks unrelated to coverage. It also strands the branch on line 91, which tests EVENT_NAME == "schedule" to select the full matrix and the macOS runners. With no schedule trigger nothing can reach that branch, so the full matrix now only runs on a manual dispatch or a workflow_call. Was this a temporary change so cron runs would not fire while you iterated? If so, please restore it.

.simplecov is not in the pull_request paths filter above. The filter lists Gemfile, Rakefile and valkey.gemspec, but not the new config file, so a PR that only edits .simplecov runs no CI at all. That is exactly the PR shape the ratchet workflow produces: bump the threshold, change nothing else. Please add - .simplecov to the list.

[xShinnRyuu]

Nothing reads the number this produces.

Each matrix leg measures coverage inside its own workspace, prints it, and loses it when the job tears down. There is no artifact upload, no $GITHUB_STEP_SUMMARY line, and no status check. That gives 19 independent coverage runs and no way for a reviewer to see a drop.

Writing the figure from coverage/.last_run.json into the job summary would make it visible for very little work. Uploading coverage/ from a single reference leg would let you compare across runs.

The comment also overstates the reach. The Ruby 3.0 and 3.1 legs measure nothing, because test_helper.rb gates on Ruby 3.2, so "every run" is really most of the matrix.

[xShinnRyuu]

These thresholds sit below where coverage already is, so they cannot catch a realistic regression.

I ran the full suite at this head and got line 88.46% and branch 71.54% (1629 tests, standalone plus cluster plus unit, merged). Against a floor of 80, line coverage can fall more than 8 points and still pass.

This is also the ratchet @currantw asked for above, and SimpleCov ships it. expected_coverage sets the minimum and the maximum to the same value, so coverage that rises fails the build until someone raises the number:

expected_coverage line: 88, branch: 71

That gets the behaviour from the C# repo without a custom mechanism.

[xShinnRyuu]

This comment points at the wrong option. The config uses minimum_coverage, not expected_coverage.

The second sentence is also wrong about cause. Reaching 80% coverage would not let Ruby 3.0 or 3.1 measure anything, because the gem itself refuses to install: simplecov 1.2.0 declares required_ruby_version >= 3.2. The version guard on line 9 is correct, but the reason given for it is not.

Suggested replacement for lines 7 and 8:

# SimpleCov 1.x needs Ruby 3.2, so the 3.0 and 3.1 legs measure nothing.
# Drop this guard when those versions are no longer supported.

Separately, ENV["COVERAGE"] is truthy for any value, so COVERAGE=0 switches coverage on. ENV["COVERAGE"] == "1" would match how DEVELOPER.md describes the flag.

[xShinnRyuu]

A contributor cannot learn from this what their coverage needs to be.

The section explains how to produce a report, which is good, but it leaves out the three things someone about to open a PR actually needs: what number is expected, that nothing currently fails on it, and that the thresholds apply to one CI configuration rather than all of them.

Suggested addition after this paragraph:

CI measures coverage on every test job but does not fail on a drop yet. The
thresholds in `.simplecov` are 80% line and 70% branch, and they apply only to
the reference configuration. Issue #307 tracks turning this into a real gate.

Three smaller corrections in the block below:

• Line 88 says "Full suite (standalone + cluster)", but rake test runs unit, standalone and cluster.
• Line 92 has two trailing spaces after open coverage/index.html, which renders as a line break.
• "MacOS" is spelled "macOS".

[xShinnRyuu]

"cluster runs last" is true for rake test but nothing enforces it, so this is a fragile thing to hang the project total on.

rake test:cluster on its own also sets this true. With merge_timeout 3600 the total then covers whatever is already in .resultset.json from earlier runs, which can be stale or missing the standalone suite entirely. Someone rerunning just the cluster suite to debug a failure would get a project total computed from partial data.

Checking that the merged set actually contains all three command names would be sturdier than relying on task order.

[xShinnRyuu]

These badges are worth having, but none of them is about coverage, which makes them scope creep in a coverage PR.

The gap is also conspicuous: this is the one PR where a coverage badge would belong, and there is no way to add one yet because the figure is never published anywhere (see my note on the COVERAGE env line in ci.yml).

Either add a coverage badge once the number is published, or move these three to their own PR so this one stays focused.

[xShinnRyuu]

The load order here is correct and load bearing, so this is worth keeping as is. I checked the counterfactual: moving these three lines below require "valkey" drops unit coverage from 50.62% to 0.00%.

One file still escapes, though. lib/valkey/version.rb reports 0.00% in the merged report even though every run loads it. Bundler gets there first: Gemfile line 6 is gemspec, and valkey.gemspec line 3 does require_relative "lib/valkey/version", so the file is already in $LOADED_FEATURES before any test code runs.

$ bundle exec ruby -e 'puts $LOADED_FEATURES.grep(/valkey\/version/).empty? ? "no" : "already loaded"'
already loaded

The cover "lib/**/*.rb" glob then finds it on disk and lists it at 0%. No test can move that number, so it reads as a real gap when it is not. Adding skip "lib/valkey/version.rb" to .simplecov would keep the report honest.