Skip to content

ci: run operator unit tests - #1838

Merged
gfyrag merged 1 commit into
release/v3.0from
fix/operator-tests-ci
Aug 28, 2026
Merged

ci: run operator unit tests#1838
gfyrag merged 1 commit into
release/v3.0from
fix/operator-tests-ci

Conversation

@gfyrag

@gfyrag gfyrag commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What changed

  • add a distinct Tests-Operator job to the Default workflow
  • run the full default-tag misc/operator module suite through pinned Nix Go
  • make release and opt-in PR image publication wait for operator tests
  • add a narrow fail-closed workflow reachability invariant

Reproduction

Target base: ae5346f

Default-tag test-bearing packages existed in misc/operator, while GitHub Actions only generated, linted, and built that module. No PR job ran its unit tests.

Pinned Go 1.26.5 baseline:

  • go test ./...: PASS
  • cold-cache runtime: 58.17s
  • warm candidate runtime: 8.48s
  • go test -race -count=2 ./...: PASS in 38.24s

The initial local attempts failed before repository compilation because an ambient Homebrew Go 1.27 GOROOT overrode the Nix Go 1.26.5 binary. Clearing GOROOT resolved the environment; this PR explicitly clears it in the job command.

Scope

No integration/envtest tags and no Chainsaw coverage are added or changed.

Required CI integration

PR #1835 is still open and owns the in-flight Required CI aggregate. This PR intentionally does not duplicate or edit that aggregate. If #1835 merges first, this branch must be synchronized and Tests-Operator added to Required-CI.needs under its documented contract.

Validation

  • operator go test ./...
  • operator go test -race -count=2 ./...
  • go test -count=1 ./scripts/checkoperatorci
  • bash scripts/check-repo-invariants
  • trusted just pre-commit to fixpoint
  • bash scripts/agent-check
  • exact target-base diff review: APPROVE, 0 findings

Risk

LOW: CI and repository-invariant tooling only; production and operator runtime code are unchanged.

@shipfox-ai

shipfox-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Arbitration summary — PR #1838 (ci: run operator unit tests)

The change is sound and worth merging. misc/operator is a separate Go module and was genuinely excluded from the root module's go test ./..., so the new Tests-Operator job closes a real coverage hole. The job is unconditional, wired into needs: of both publishing gates (Release-Latest, Build-PR-Image), correctly omitted from Coverage.needs (no coverage artifact), and backed by a focused invariant checker (scripts/checkoperatorci) with acceptance/rejection tests. I verified the diff directly. Both reviewers agree the feature works; the only disagreement is whether the invariant gate and its documentation contain non-blocking defects. I side with the middle position: the feature is clean, but two minor documentation/naming refinements are warranted. Overall recommendation: approve with comments.

Agreed findings

None rise to blocking or clearly-confirmed-defect status. All disputed items are addressed below.

Ruled disputes

Finding 1 — Gate does not verify needs: wiring; "reachability" label overstates the guarantee.

  • Claude (A): The checker never parses any job's needs:, so deleting Tests-Operator from every gating list would still PASS while the docs/error message advertise a "reachability" guarantee — a code/doc gap in the same PR.
  • Codex (B): Rule 4 only claims an unconditional pinned-command job must be retained; it never claims to validate downstream needs or branch protection, and current wiring is correct — so this is a possible future hardening, not a defect.
  • Ruling — Partially upheld (minor, non-blocking). I confirmed the parsed struct (scripts/checkoperatorci/main.go, checkOperatorTestsWorkflow, lines ~39–62) contains only Jobsif/continue-on-error/Steps, with no needs field. Rule 4's prose (docs/.../repository-invariant-gates.md:25–28) is accurate about what it enforces. However, the label "reachability check" plus the checker's own error string "operator unit tests must remain reachable" (main.go:59–61) carry a stronger connotation (failure propagates and blocks the pipeline) than the gate actually implements (it only pins job definition + command). Codex is right that this is not a behavioral bug and current needs: wiring is correct; Claude is right that the wording over-promises. Fix is cheap and doc-only: rename to a "definition/command" gate or add one sentence in repository-invariant-gates.md:48–52 clarifying that true merge-blocking lives in GitHub branch protection (out of repo). No code change required to merge.

Finding 2 — Pinned command duplicates the operator-test recipe and forbids just operator-test (DRY).

  • Claude (A): operatorTestsCommand pins a string that duplicates and already diverges from the justfile operator-test recipe (CI adds unset GOROOT + Nix), and the gate forbids the DRY nix develop --command just operator-test, entrenching duplication against repo conventions.
  • Codex (B): The duplication is deliberate and documented ("The command is kept explicit so the nested Go module cannot silently fall outside root-module ./... tests"); repo guidance says prefer DRY, not mandate routing every command through just.
  • Ruling — Not upheld as a defect (optional note). I confirmed both definitions exist and differ. But the explicit pinning is an intentional, documented tradeoff: routing the invariant through a mutable just recipe would let the contract be silently changed in one place, defeating the gate's purpose. That is a legitimate reason to hold the string literal. Codex's reading is correct. Optional improvement: align the operator-test recipe to also unset GOROOT / run under Nix so the two paths don't behaviorally diverge, but this is not required.

Finding 3 — Syntactic gate cannot detect operator tests hidden behind a source-level build tag.

  • Claude (A): The gate inspects only the workflow string, so tests moved behind a build constraint inside misc/operator would silently skip while the gate still reports PASS — worth a one-line doc caveat (explicitly framed as an inherent limitation, not a defect).
  • Codex (B): Inspecting future source-level build constraints is neither promised nor a reasonable implication of a workflow-command invariant; go test ./... following Go's normal build selection needs no caveat.
  • Ruling — Not a finding against the PR. Both parties agree it is an inherent limitation of any syntactic workflow gate, and Claude does not claim it is a defect. A documentation caveat would be a courtesy, not a fix. No action required.

Disposition

Approve with comments. The two doc-only refinements (clarify the "reachability" naming in Finding 1; optionally align the just recipe for Finding 2) can be follow-ups and do not block this PR. Risk: LOW. No human decision required.

Reviewed by Claude (claude-opus-4.8) and Codex (gpt-5.6-sol) via Shipfox; arbitrated by Claude.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.80%. Comparing base (4d4d40c) to head (6b7bab8).
⚠️ Report is 1 commits behind head on release/v3.0.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/v3.0    #1838      +/-   ##
================================================
+ Coverage         76.72%   76.80%   +0.08%     
================================================
  Files               472      472              
  Lines             50356    50356              
================================================
+ Hits              38634    38678      +44     
+ Misses             8324     8277      -47     
- Partials           3398     3401       +3     
Flag Coverage Δ
e2e 76.80% <ø> (+0.08%) ⬆️
scenario 76.80% <ø> (+0.08%) ⬆️
unit 76.80% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gfyrag
gfyrag requested a review from NumaryBot August 28, 2026 16:35
@gfyrag
gfyrag force-pushed the fix/operator-tests-ci branch from fa65489 to 6b7bab8 Compare August 28, 2026 21:11
@NumaryBot

Copy link
Copy Markdown
Contributor

✅ Approve — automated review

The operator test job is unconditional, uses the pinned Nix environment, and gates both image publication jobs. The added invariant checker and tests align with the intended CI contract.

No findings.

@gfyrag
gfyrag merged commit 7c88752 into release/v3.0 Aug 28, 2026
14 checks passed
@gfyrag
gfyrag deleted the fix/operator-tests-ci branch August 28, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants