Skip to content

ci: add code coverage #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Aryex wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

ci: add code coverage #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
af07b43
ci: add SimpleCov for code coverage
Aryex Aug 27, 2026
7c45da7
fix: updated minimum coverage
Aryex Sep 3, 2026
ea184f9
fix: add an option to upload coverage reports
Aryex Sep 4, 2026
2a975fc
fix: used expected_coverage instead
Aryex Sep 4, 2026
3fde325
fix: removed upload_coverage
Aryex Sep 4, 2026
dd5590c
fix: only run coverage on ruby 3.2+
Aryex Sep 4, 2026
72af14b
fix: enforce specific coverage on specific versions
Aryex Sep 8, 2026
cc30faf
updated coverage
Aryex Sep 8, 2026
6154b34
fixed: code cov
Aryex Sep 8, 2026
451b779
updated expected coverage
Aryex Sep 8, 2026
b279a60
test
Aryex Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ on:
- main
- release-*

schedule:

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- cron: "0 6 * * *"

workflow_dispatch:
inputs:
full-matrix:
Expand Down Expand Up @@ -252,6 +249,7 @@ jobs:
run: bundle exec rake test
env:
SKIP_TLS_TESTS: ${{ matrix.host.OS == 'macos' && 'true' || '' }}
COVERAGE: "1" # Coverage is cheap so we compute it every run

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Must run in its own process: the fork guard's precondition is that no
# command has been issued yet, which is false inside the shared suite.
Expand Down
28 changes: 28 additions & 0 deletions .simplecov
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

# SimpleCov configuration
SimpleCov.configure do
suite = ENV["COV_GROUP"] || "all"

enable_coverage :branch
primary_coverage :line
command_name "test-#{suite}"
merging true
merge_timeout 3600

skip %r{^/test/}
skip %r{^/valkey-glide/} # vendored upstream submodule, not our code
cover "lib/**/*.rb" # includes unloaded lib files and restricts the report to them

# tracked in https://github.com/valkey-io/valkey-glide-ruby/issues/307
# reference_config = RUBY_PLATFORM.start_with?("x86_64-linux") &&
# RUBY_VERSION.start_with?("3.4") &&
# ENV["ENGINE_VERSION"] == "9.0"

# cluster runs last, so its report is the merge of all three suites.
project_total = suite == "cluster"

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.


if reference_config && project_total

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

minimum_coverage line: 80, branch: 70

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
15 changes: 15 additions & 0 deletions DEVELOPER.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,21 @@ bundle exec rake test:standalone
python3 valkey-glide/utils/cluster_manager.py --tls stop --prefix tls-standalone
```

## Test Coverage

We measure both **line coverage** and **branch coverage** with [SimpleCov](https://github.com/simplecov-ruby/simplecov). Coverage is opt-in via the `COVERAGE` environment variable; when set, the test suite writes an HTML report to `coverage/index.html` and a machine-readable summary to `coverage/.last_run.json`. The `coverage/` directory is gitignored.

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


```bash
# Standalone only
COVERAGE=1 bundle exec rake test:standalone

# Full suite (standalone + cluster)
COVERAGE=1 bundle exec rake test

# Coverage report (MacOS)
open coverage/index.html
```

## RuboCop

```bash
Expand Down
10 changes: 6 additions & 4 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ gem "irb" if RUBY_VERSION >= "4.0"

gem "rake", "~> 13.0"

gem "minitest", "~> 5.16"

gem "minitest-reporters", "~> 1.4"

gem "rubocop", "~> 1.21"

group :test do
gem "minitest", "~> 5.16"
gem "minitest-reporters", "~> 1.4"
gem "simplecov", "~> 1.1" if RUBY_VERSION >= "3.2"
end
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@

# Valkey GLIDE for Ruby

[![CI](https://github.com/valkey-io/valkey-glide-ruby/actions/workflows/ci.yml/badge.svg?branch=main)](https://github.com/valkey-io/valkey-glide-ruby/actions/workflows/ci.yml?query=branch%3Amain)

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[![Gem Version](https://img.shields.io/gem/v/valkey-glide-rb.svg)](https://rubygems.org/gems/valkey-glide-rb)
[![License](https://img.shields.io/badge/license-Apache--2.0-blue.svg)](https://github.com/valkey-io/valkey-glide-ruby/blob/main/LICENSE)

Valkey General Language Independent Driver for the Enterprise (GLIDE) is the official open-source Valkey client library, part of the [Valkey](https://valkey.io) organization. The Ruby gem (`valkey-glide-rb`) wraps [Valkey GLIDE Core](https://github.com/valkey-io/valkey-glide), giving Ruby applications the performance and reliability of the GLIDE core.

## Features
Expand Down
8 changes: 8 additions & 0 deletions Rakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,21 @@ namespace :test do
cluster: "integration/cluster"
}
groups.each do |group, dir|
# Set the COV_GROUP environment variable for the test group task, so that
# SimpleCov can use it to determine the coverage group.
task "cov_group_#{group}" do
ENV["COV_GROUP"] = group.to_s
end

Rake::TestTask.new(group) do |t|
t.libs << "test"
# Only add local lib to load path when not testing installed gem
t.libs << "lib" unless ENV["TEST_INSTALLED_GEM"]
t.test_files = FileList["test/#{dir}/**/*_test.rb"]
t.options = '-v' if ENV['CI'] || ENV['VERBOSE']
end

Rake::Task["test:#{group}"].enhance(["test:cov_group_#{group}"])
end

# Exclude module directories (integration/valkey/, lint/) from lost_tests check
Expand Down
7 changes: 7 additions & 0 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@
# This is useful for CD testing to verify the published gem works correctly
$LOAD_PATH.unshift File.expand_path("../lib", __dir__) unless ENV["TEST_INSTALLED_GEM"]

# We use SimpleCov expected_coverage option, which requires SimpleCov 1.0 which needs

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Ruby 3.2. Enable for Ruby 3.0 and 3.1 once coverage reaches 80%.
if ENV["COVERAGE"] && RUBY_VERSION >= "3.2"
require "simplecov"
SimpleCov.start

@xShinnRyuu xShinnRyuu Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

require "valkey"
require_relative "support/test_cluster"

Expand Down
Loading