(PE-45655) Wire up SimpleCov and add DR/XL spec coverage to configure.pp - #689
Open
CharithaDunuwille wants to merge 16 commits into
Open
(PE-45655) Wire up SimpleCov and add DR/XL spec coverage to configure.pp#689CharithaDunuwille wants to merge 16 commits into
CharithaDunuwille wants to merge 16 commits into
Conversation
peadm had no working test-coverage measurement despite the simplecov-console
and codecov gems sitting unused in the Gemfile, and the one active mechanism
(RSpec::Puppet::Coverage) only sees manifests/*.pp classes -- structurally
blind to the plans and functions that make up almost all of this repo.
- Widen puppetlabs_spec_helper's SimpleCov track_files glob (spec_helper_local.rb)
from lib/**/*.rb to also include tasks/*.rb, and drop the Codecov formatter
(no CODECOV_TOKEN configured yet) so `rake spec:simplecov` actually exits 0
instead of hard-failing on upload.
- Add a Coverage job to .github/workflows/spec.yml that runs it on every PR
and uploads the HTML report as a build artifact. Marked continue-on-error,
matching this repo's existing convention for advisory-only jobs (see the
PE-45110 experimental Ruby 4 job) -- report-only for now, no threshold
enforced yet.
- Add documentation/test-coverage.md explaining what each of the three
coverage mechanisms (SimpleCov, RSpec::Puppet::Coverage, and manual
plan/function review) actually measures, since none of them alone answers
"how much of PEADM is tested."
- Add real DR/XL branch coverage to spec/plans/subplans/configure_spec.rb,
the ticket's named highest-priority gap: the existing spec only covered
the no-DR path. New tests assert the actual provision_replica task
parameters (replica certname, legacy flag, token_file) for both standard
and extra-large DR topologies -- verified by mutation testing (flipping
`legacy => true` to `false` locally) that they'd actually fail if that
logic broke, not just that the plan "runs successfully."
- Fix a pre-existing, silently-broken assertion in the same file's no-DR
test: `expect_task('peadm::util::copy_file').not_be_called` bound to the
wrong mock registry (copy_file is a plan, not a task -- expect_task and
expect_plan track separate doubles), so it always passed regardless of
actual behavior. Replaced with `expect_plan(...).be_called_times(5)`,
the real call count confirmed by instrumenting the scenario locally
(configure.pp calls it unconditionally even with no replica configured).
Co-Authored-By: Claude <noreply@anthropic.com>
The format-yaml-files CI job runs yamlfix on any changed workflow YAML and auto-commits the result -- but GitHub blocks that auto-commit from pushing to a workflow file without the `workflows` permission scope, so the check failed outright instead of just reformatting. Matching yamlfix's expected style locally avoids that dead end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seamymckenna
approved these changes
Aug 11, 2026
The new DR/XL tests only asserted on provision_replica's targets and params -- a regression that dropped or emptied the copy_file replica target list (which carries the CA chain and orchestrator/console encryption keys a DR replica needs after failover) would have passed both tests undetected. PlanStub#with_targets doesn't work for this: it compares against the raw `targets` param, which still holds resolved Bolt::Target objects at match time, so the Set comparison never matches a plain string list -- confirmed by instrumenting the call locally. Asserting on params['targets'].map(&:name) inside a return block works instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds Ruby coverage reporting and improves DR/XL configure-plan verification.
Changes:
- Configures SimpleCov for Ruby tasks and functions.
- Adds an advisory CI coverage job and coverage documentation.
- Expands DR/XL plan specs and fixes a broken mock expectation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/spec.yml |
Adds the advisory SimpleCov job. |
documentation/test-coverage.md |
Documents coverage mechanisms and baseline. |
spec/plans/subplans/configure_spec.rb |
Adds DR/XL assertions and corrects plan mocking. |
spec/spec_helper_local.rb |
Configures tracked files and formatters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The two DR tests' copy_file target-list assertion was byte-for-byte identical. This repo already has precedent for factoring shared BoltSpec setup into a plain method on the outer describe block (allow_standard_non_returning_calls in add_replica_spec.rb, upgrade_spec.rb, add_compilers_spec.rb) -- follow the same pattern here instead of keeping two copies in sync by hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A regression that flipped source_host to $replica_target (copying content from the wrong node, or the replica's own stale files) would have passed both DR tests undetected -- only the destination targets were asserted, not the source. Mutation-tested: confirmed both tests fail when source_host is swapped to $replica_target in configure.pp, then reverted (0 net diff there). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The comment implied tasks/*.rb and lib/puppet/functions/peadm/*.rb were both newly covered by this change. Only tasks/*.rb is new -- lib/puppet/functions/peadm/*.rb was already covered by puppetlabs_spec_helper's default lib/**/*.rb glob. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- documentation/test-coverage.md: RSpec::Puppet::Coverage measures individual resource declarations, not resource types as a category (confirmed against rspec-puppet's Coverage#add_from_catalog, which keys on resource.to_s). - spec.yml: soften the Coverage job's redundancy claim -- it doesn't pin PUPPET_GEM_VERSION the way the Spec job's matrix does, so a regression unique to that combination wouldn't strictly be redundant with what Spec catches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
continue-on-error masks a hang the same way it masks a real coverage regression -- without a timeout, a stuck bundle exec rake spec:simplecov would run for GitHub Actions' 360-minute default before being killed, silently burning CI capacity on every matching PR. This repo already uses explicit timeout-minutes elsewhere (test-add-compiler-matrix.yml); apply the same pattern here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
allow_apply/allow_any_task/allow_any_plan/allow_any_command was repeated identically in all three describe blocks; none of them care about the exact calls configure.pp makes, only about the expectations set up afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
be_called_times(5) plus the targets/source_host checks would still pass if the same file were copied 5 times instead of the 5 distinct files configure.pp actually syncs. Collect each call's path and assert the full set matches. Mutation-tested: confirmed both DR tests fail when one of the 4 replica_content_sources entries in configure.pp is duplicated (dropping a different one), then reverted (0 net diff there). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bare constant assignment inside a describe block scopes to Object (RSpec evaluates the block via class_exec), not the example group -- silently leaking SYNCED_REPLICA_FILES into the global namespace. Harmless today since nothing else uses that name, but a method avoids the footgun entirely and matches this file's existing pattern (allow_standard_calls!, expect_copy_file_called_for_replica_only!). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The comment referenced "the 5 distinct files" without naming where the 5th one (hiera.yaml, the common_content_source) comes from -- only the 4 replica_content_sources were listed by name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The "without DR" test only asserted call count (5), unlike the two "with DR" tests which also assert targets/source_host/path. A regression that started populating targets from $primary_target or $compiler_targets in this no-replica scenario would have passed silently. Mutation-tested: confirmed all three tests fail when $primary_target is added into the common copy_file call's target list, then reverted (0 net diff in configure.pp). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- spec/plans/subplans/configure_spec.rb: the XL-with-DR test now passes and asserts token_file, matching the Standard-DR test's rigor. Only the target list and legacy/replica params were previously protected on this path -- a token_file regression specific to the XL branch would have passed silently. Mutation-tested: confirmed both DR tests fail when token_file is dropped in configure.pp's provision_replica call, then reverted (0 net diff there). - documentation/test-coverage.md: the Coverage job doesn't run on every PR -- it's gated by the same path filters and fork-PR setup_matrix check as the Spec job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Now that the XL-with-DR test asserts token_file too, its provision_replica expectation block is byte-for-byte identical to the Standard-DR test's. Same treatment already applied to the copy_file assertion in b0fe4e9 -- one place to update if the PE-42816 workaround or params shape ever changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The comment blamed class_exec for the Object-leak, but the leak happens identically under class_exec, module_exec, or instance_eval -- it's Ruby's lexical constant scoping (based on where the code is textually written), not the receiver the exec mechanism uses at runtime. Verified empirically. The conclusion (use a method, not a constant) was already correct; only the stated mechanism was wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seamymckenna
approved these changes
Aug 13, 2026
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
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
tasks/*.rbandlib/puppet/functions/peadm/*.rb(previously untracked by the defaultpuppetlabs_spec_helperconfig), with a report-onlyCoverageCI job (continue-on-error: true, matching the existing experimental-job convention in this repo).spec/plans/subplans/configure_spec.rb, asserting on the actualprovision_replicaparams (replica,token_file,legacy) and target set for both the standard-with-DR and XL-with-DR architectures — not just "the plan runs successfully."expect_task('peadm::util::copy_file').not_be_calledbound to the wrong BoltSpec mock registry (copy_fileis a plan, not a task), so it always passed regardless of actual behavior. Replaced withexpect_plan('peadm::util::copy_file').be_called_times(5), matching the precedent inspec/plans/add_replica_spec.rb.documentation/test-coverage.md, documenting the three separate (non-overlapping) coverage mechanisms in this repo and the real measured baseline (4.29%, 55/1283 lines, 27 files).This is a deliberately scoped-down first PR for PE-45655: infra + the single highest-priority named gap (DR/XL branch coverage in
configure.pp). The remainder of PE-45655's original acceptance criteria (restore.pp/upgrade.pp/install.pp negative-path coverage, no-spec plans, zero-coverage Ruby functions, RBAC/cert task specs, remaining configure.pp branches, raising both coverage floors) is filed as a follow-on: PE-45737.Every new assertion was mutation-tested locally (temporarily broke the corresponding logic in
configure.pp, confirmed the test fails, then reverted —configure.ppitself has no net diff in this PR).Jira: https://perforce.atlassian.net/browse/PE-45655
Test plan
bundle exec rspec spec/plans/subplans/configure_spec.rb— 4 examples, 0 failuresbundle exec rubocop— cleanbundle exec rake spec:simplecovrun locally to confirm the Coverage job's command works and produces a reportconfigure.pp, confirmed failure, reverted/review-prpasses (7 agents: code, error, test, comment, type, simplifier, security)🤖 Generated with Claude Code