Migrate tests to testify by mariusvniekerk · Pull Request #487 · roborev-dev/roborev

mariusvniekerk · 2026-03-09T15:39:32Z

Summary

migrate test assertions to testify / patterns across the suite
fix migration regressions by restoring test semantics where assertions were over-rewritten
keep matcher strictness pragmatic ( where numeric type differences are expected)

roborev-ci · 2026-03-09T15:53:46Z

roborev: Combined Review (`f9e90f2`)

Verdict: The PR requires attention due to
a missing truncated diff preventing full analysis and a model-selection bug during compact enqueue.

High

Missing Diff Content
- Location: Diff
- Problem: The diff was truncated (Diff too large to include) and is not accessible for review, preventing analysis of the actual code
  changes.
- Suggested Fix: Provide the complete diff in the payload or ensure the relevant PR branch is checked out locally so the files can be read directly.

Medium

Model Selection Bypass in Compact Enqueue
- Location: [compact.go](/home/roborev
  /repos/roborev/cmd/roborev/compact.go):233, compact.go:261, [server.go](/home/roborev/repos
  /roborev/internal/daemon/server.go):746
- Problem: roborev compact pre-resolves and sends Model in the enqueue request, which makes req.Model non-empty on the daemon side and bypasses backup-model selection logic
  (backup_model is only used when req.Model == ""). If the primary agent is unavailable and the backup agent is chosen, compact jobs can run with the wrong model.
- Suggested Fix: For compact enqueue, pass only explicit CLI --model (or empty), and let the daemon
  perform final model resolution for the selected agent (including backup model).

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T16:17:50Z

roborev: Combined Review (`c1bc23b`)

Verdict: The PR introduces a net loss of targeted regression tests around config resolution, daemon sessions, and storage, which need to be restored.

Medium

Major test coverage regression in config resolution paths
File:
internal/config/config_test.go
Many unit tests for active config behavior were removed (ResolveJobTimeout, ResolveModelForWorkflow, ResolvePostCommitReview, ValidateReviewTypes, ResolveReuseReviewSession*, etc.), while the implementation is still active in internal/config/config. go:742.
Suggested fix: Restore these tests (or move them into focused files) so precedence/normalization logic stays covered.

Reuse-session behavior lost direct daemon test coverage
File: internal/daemon/server_jobs_test.go
Tests for findReusable SessionID and enqueue session reuse were removed, but logic remains in internal/daemon/server.go:965.
Suggested fix: Reintroduce branch-ancestry/lookback/session-validity test cases for reusable sessions.

Session ID persistence/migration regression tests removed

Files: internal/storage/db_job_test.go, internal/storage/db_migration_test.go
Coverage for stale-worker SaveJobSessionID behavior and session_id/migration edge cases was removed, while code paths remain in internal/storage/jobs .go:549 and internal/storage/db.go:266.
Suggested fix: Restore these targeted DB tests to protect concurrency/migration correctness.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T17:29:36Z

roborev: Combined Review (`9efbed8`)

Verdict: The PR introduces significant regressions in test coverage and an incomplete testify migration, likely due to rebase conflicts.

High

Incomplete testify Migration
- Files: go.mod, go.sum, and all *_test.go files
- Description: Despite the commit history indicating a migration to testify, there are zero imports or usages
  of the package in the codebase (tests still rely exclusively on t.Fatalf/t.Errorf). Furthermore, github.com/stretchr/testify exists in go.sum but is completely missing from go.mod, confirming that the migration was incorrectly dropped during conflict resolution.
- Suggested fix: Re-apply the testify test assertions to the test files and run go mod tidy to sync the dependencies.
Severe Loss of Test Coverage
- File: internal/config/config_test.go (large deletion
  after ~old line 85)
- Description: The file drops from ~63 tests to 4, and many unique config-behavior tests are gone (not relocated), including coverage for repo config loading, workflow model resolution, sync config validation, review-type validation, and documented setting persistence
  .
- Suggested fix: Restore non-duplicate tests removed during rebase cleanup (or move them into split test files, but keep coverage).

Medium

Missing Database Migration Regression Tests
- File: internal/storage/db_migration_test.go (removed tests around old lines ~222, ~236, ~760, ~879)
- Description: Migration regression coverage was removed for key schema-evolution cases (session_id, verdict bool backfill, reasoning column, patch ID, stale temp cleanup
  /orphaned FK/column-order scenarios).
- Suggested fix: Reintroduce the deleted migration tests; these are important for SQLite compatibility and upgrade safety.
Missing Session Reuse Edge-Case Coverage
- File: internal/daemon/server_jobs_test .go (removed block starting ~old line 736)
- Description: Session reuse edge-case coverage was removed (findReusableSessionID lookback, invalid stored session IDs, unrelated-history branch reuse rejection, fallback behavior).
- Suggested fix: Restore these
  tests to protect ReuseReviewSession behavior from regressions.
Dropped Agent and Runtime Behavior Tests
- Files: internal/agent/codex_test.go, internal/agent/agent_test.go
- Description: Several unique tests were
  dropped, including Codex stream parsing/timeout/stdin/session-resume behavior and backup-agent fallback/alias resolution paths.
- Suggested fix: Restore the removed behavior tests; they cover failure-prone runtime paths not exercised by the remaining argument-construction tests.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T18:14:42Z

roborev: Combined Review (`96072a0`)

Verdict: The PR successfully migrates tests to testify, but introduces a high-risk pattern of replacing value assertions with require.NoError and inadvertently drops several important test cases.

High

Incorrect Assertion Conversions

Files: internal/github/comment_test.go:105, internal/github/comment_test.go:139, [internal/github/comment_test.go:200](/home/roborev
/repos/roborev/internal/github/comment_test.go:200), [internal/worktree/worktree_test.go:199](/home/roborev/repos/roborev/internal/worktree/worktree_test.go:1
99), internal/worktree/worktree_test.go:218, cmd/roborev/fix_test.go:10 88
Problem: Many assertions were accidentally converted into require.NoError(t, err, "...expected X..."), often inside conditionals unrelated to
err. This makes tests pass even when the intended condition is wrong (false positives).
Suggested Fix: Replace these with the intended assertion (require.Equal, require.Len, require.True/False, require.Error, or require.Failf) and avoid using NoError as a generic failure primitive.

Medium

Missing File Count Assertion

File: internal/git/git_test.go:728
Problem: TestGetRangeFilesChanged lost its length assertion; require.NoError(t, err, "expected 3 files...") does not validate the file count.
Suggested Fix: Restore require.Len(t, files, 3, ...) (or equivalent).

Dropped Coverage: Branch Validation and File Filtering

File: cmd/roborev/analyze_test.go
Problem: Coverage dropped: TestIsCode File and TestAnalyzeBranchFlagValidation were removed, leaving branch-argument contract checks and code-file classification matrix less protected.
Suggested Fix: Reintroduce these tests (or equivalent subtests) to keep branch validation and file-type filtering behavior locked.

Dropped Coverage: Agent Resolution Fall
backs

File: internal/agent/acp_test.go
Problem: TestGetAvailableWithConfigPassesBackupsThrough was removed, reducing
coverage of the backup-agent fallback order in agent resolution.
Suggested Fix: Restore explicit fallback-chain test coverage.

Dropped Coverage: Concurrency Correctness

File: [internal/storage/db_job_test.go](/home/roborev/repos/roborev
/internal/storage/db_job_test.go)
Problem: TestSaveJobSessionID_StaleWorkerIgnored was removed, dropping a race-safety test for stale worker session writes.
Suggested Fix: Re-add this stale-worker ownership/session-id
test to protect concurrency correctness.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T18:44:38Z

roborev: Combined Review (`3ba96a4`)

Verdict: Changes require updates to complete the test migration and restore dropped test coverage.

High

Missing testify dependency
- Location:
  go.mod
- Problem: The github.com/stretchr/testify dependency is entirely missing from the module despite the commit messages claiming a testify migration.
- Suggested fix: Run go get github.com/stretchr/testify to add the
  dependency.

Medium

Missing concurrency coverage for stale-worker session writes
- Location: internal/storage/db_job_test.go (removed test) / internal/storage/jobs.go:543-564

Problem:** The test (TestSaveJobSessionID_StaleWorkerIgnored) that verified stale worker IDs cannot overwrite session_id after cancel/re-enqueue was removed, and there is no replacement test. This is a concurrency-safety behavior that can regress silently.
* Suggested fix:
Re-add this scenario as a dedicated unit test/subtest (worker A writes, job re-enqueued, worker B claims, worker A write is ignored).

Incomplete testify migration
- Location: internal/skills/skills_test.go: 57 (and multiple other test files like internal/git/git_test.go)
- Problem: Tests still rely on standard library assertions (e.g., t.Error, t.Fatal) and have not been refactored to use testify.
- Suggested fix: Update the remaining test files to use testify/assert or testify/require.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

mariusvniekerk · 2026-03-09T18:46:28Z

Missing testify dependency
Location:
go.mod
Problem: The github.com/stretchr/testify dependency is entirely missing from the module despite the commit messages claiming a testify migration.

False..

roborev-ci · 2026-03-09T18:56:32Z

roborev: Combined Review (`ddb71c5`)

Verdict: The PR introduces a
test suite migration to testify, but requires dependency resolution in go.mod, corrections to inverted error assertions that could mask test failures, and completion of the migration in several test files.

🔴 High

Missing Dependency in go.mod
- File: go .mod
- Finding: The github.com/stretchr/testify dependency is present in go.sum but is completely missing from the require blocks in go.mod.
- Suggested Fix: Run go mod tidy to correctly register
  testify as a dependency.

🟡 Medium

Incorrect Error Assertion
- File: cmd/roborev/ci_test.go:75
- Finding: The check for expected error text is effectively disabled. Inside if !strings .Contains(err.Error(), tt.wantError), it calls assert.Errorf(t, err, ...), which only checks err != nil (already true), so the test can pass even when the message is wrong.
- Suggested Fix: Replace with require.ErrorContains( t, err, tt.wantError) (or assert.Contains(t, err.Error(), tt.wantError)).
Inverted Assertions in Error Checks
- Files:
  - cmd/roborev/fix_test.go:111 4
  - cmd/roborev/mock_daemon_test.go:150
  - internal/agent/acp_integration_test.go:52
  - internal/daemon/activitylog_test.go:11 9
  - internal/storage/ci_test.go:69
- Finding: require.Errorf(...) is used in if err != nil branches as if it were a failure assertion. That is inverted: require.Errorf passes when err is non-nil, so setup/decode/update failures can be silently accepted, weakening tests and masking regressions.
- Suggested Fix: Replace require.Errorf with require.NoError. In cases expecting an error, use require.Error or require.ErrorContains directly
  without inverted control flow.
Incomplete Migration to testify
- Files: Test files (e.g., internal/git/git_test.go, internal/daemon/server_test.go)
- Finding: The codebase still
  heavily relies on standard t.Fatal/t.Errorf calls and custom test helpers (e.g., assertExcluded, assertContainsArg) rather than utilizing the testify assert and require packages. This indicates the migration was either lost during rebase merges or is incomplete
  .
- Suggested Fix: Refactor the custom assertion helpers and manual error checks to use standard testify/assert and testify/require functions.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T19:08:59Z

roborev: Combined Review (`12c5e5b`)

Verdict: The PR introduces severe regressions in test assertions during the testify migration, effectively disabling several important validations.

High

Broken assertions after testify migration
(tests now pass when they should fail)
require.NoError(t, err, "expected X, got Y") is being used for non-error assertions (id, callCount, marker/truncation checks). Since err is already nil, these checks become no-ops.

Suggested fix: Replace with direct assertions (require.Equal, require.Zero, require.True/False, require.Contains, require.LessOrEqual).
Locations:
- [internal/github/comment_test.go:105](/home/rob
  orev/repos/roborev/internal/github/comment_test.go:105)
- [internal/github/comment_test.go:114](/home/roborev/repos/roborev/internal/github/comment_test.go:1

internal/github/comment_test.go:139
internal/github/comment_test.go:178
[internal/github/comment_test.go:201](/home/roborev/repos/roborev/internal/github/comment_test
.go:201)
internal/github/comment_test.go:320
Worktree behavior checks are effectively disabled

Examples include if patch == "" { require.NoError(t, err) } and if err == nil { require.NoError(t, err) }, which pass when the wrong behavior occurs.
Suggested fix: Use require.Empty/NotEmpty and require.Error/NoError directly on the intended value.
Locations:

internal/worktree/worktree_test.go:79
internal/
worktree/worktree_test.go:97
[internal/worktree/worktree_test.go:119](/home/robore
v/repos/roborev/internal/worktree/worktree_test.go:119)
[internal/worktree/worktree_test.go:200](/home/roborev/repos/roborev/internal/worktree/worktree_
test.go:200)
internal/worktree/worktree_test.go:219

Medium

Retry/
deadline tests in fix_test no longer validate intended outcomes
Conditions like "expected context.DeadlineExceeded" and "expected recovery call count > 0" are guarded by require.NoError(t, err) and may pass incorrectly.
Suggested fix: Use require .ErrorIs and explicit count assertions (assert.Greater, require.NotZero).
Locations:
- [cmd/roborev/fix_test.go:331](/home/roborev/repos/roborev/cmd/roborev/fix_
  test.go:331)
- cmd/roborev/fix_test.go:1554
- cmd/
  roborev/fix_test.go:1600
- cmd/roborev/fix_test.go:1603
- [cmd/roborev/fix_test.go:1643](/home/roborev/repos/roborev/cmd
  /roborev/fix_test.go:1643)
Result count assertion accidentally dropped in skills tests
require.NoError(t, err, "expected %d results, got %d", ...) is used where require.Len(t, results, tt .wantResults) is intended. This no longer verifies Update() result count.
Suggested fix: Replace with require.Len.
Location:
- [internal/skills/skills_test.go:284](/home/roborev/repos/roborev
  /internal/skills/skills_test.go:284)

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T19:21:20Z

roborev: Combined Review (`64c443e`)

Verdict: The PR consists primarily of a test-suite migration to testify, but introduces some medium
-severity test coverage regressions that should be addressed.

Medium Severity

Coverage regression in branch-arg validation tests
File: [cmd/roborev/analyze_test.go:575](/home/roborev/repos/roborev/cmd/roborev/
analyze_test.go:575)
The range removes TestAnalyzeBranchFlagValidation without replacement. This drops explicit checks for:

--branch requiring an analysis type
rejecting file patterns when --branch is set

Suggested fix: Restore these two validation cases (table
-driven is fine) so analyze arg validation regressions are caught.

Coverage loss / mixed concerns in git helper tests
File: [internal/git/git_test.go:173](/home/roborev/repos/roborev/internal/git/git_
test.go:173)
TestIsRebaseInProgress was removed; remaining rebase checks are now embedded under TestGetHooksPath, and key cases from earlier coverage (no rebase, rebase-merge) are no longer present. The custom core.hooksPath relative case was also dropped.

Suggested fix: Split rebase checks back into a dedicated TestIsRebaseInProgress and restore the missing core.hooksPath relative-path scenario.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T20:34:25Z

roborev: Combined Review (`2fe9057`)

Verdict: All agents agree the code is clean. No Medium, High, or Critical issues were found across the reviewed commits.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T20:49:50Z

roborev: Combined Review (`2d0dd4a`)

Verdict: The PR introduces a medium-severity test coverage gap, though a comprehensive review was partially blocked due to diff size constraints.

High

Missing Diff for Review
- Location: Input Prompt
- Problem: The combined diff
  was too large and excluded from the context, preventing one of the agents from performing a complete review of the code changes.
- Suggested Fix: Run the review on a smaller commit range or ensure the full diff is provided to all agents.

Medium

Testing gap: backup-agent passthrough coverage
was removed
- File: internal/agent/acp_test.go (removed test TestGetAvailableWithConfigPassesBackupsThrough, around old line ~1195)
- Problem: GetAvailableWithConfig(..., backups...) is a wrapper
  path, and this test specifically verified that backup agents are honored when the preferred agent is unavailable. Other tests still cover GetAvailable, but not this wrapper passthrough.
- Suggested Fix: Re-add a focused test for GetAvailableWithConfig("codex", cfg, "gemini") (or equivalent) asserting backup selection behavior.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

roborev-ci · 2026-03-09T22:49:40Z

roborev: Combined Review (`e5b2276`)

Verdict: All agents agree the code is clean; no medium, high, or critical severity issues were identified.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

wesm · 2026-03-09T23:03:33Z

thanks @mariusvniekerk !

Rewrite all assertions in control_test.go and control_unix_test.go to use testify assert/require instead of raw if/t.Errorf/t.Fatalf patterns, matching the project-wide migration in #487. Update CLAUDE.md and AGENTS.md to document that testify is the standard assertion library for all Go tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mariusvniekerk changed the title ~~Migrate tests to testify and add redundant-if assertion probe~~ Migrate tests to testify Mar 9, 2026

mariusvniekerk added 2 commits March 9, 2026 11:48

Migrate tests to testify and add redundant-if assertion probe

843b514

Remove redundant-if assert probe script

37c2c41

mariusvniekerk added 2 commits March 9, 2026 11:53

Fix post-rebase test merges

11c72c2

Fix duplicate tests after rebase

7a018d3

mariusvniekerk force-pushed the migrate-testify branch from f9e90f2 to 7a018d3 Compare March 9, 2026 16:03

Remove dead test helpers after merge cleanup

c1bc23b

Fix CI regressions in testify migration

9efbed8

Restore dropped test coverage after merge cleanup

96072a0

mariusvniekerk added 2 commits March 9, 2026 14:36

Finish testify migration in remaining tests

3ba96a4

Remove redundant assertion guards in migrated tests

ddb71c5

Restore stale worker session test and finish git/skills cleanup

12c5e5b

Fix broken error assertions in tests

64c443e

Restore analyze and git helper test coverage

2fe9057

Drop useless generic assertion messages

2d0dd4a

Restore backup-agent passthrough coverage

e5b2276

wesm merged commit 5d507e3 into roborev-dev:main Mar 9, 2026
8 checks passed

Conversation

mariusvniekerk commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (f9e90f2)

High

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (c1bc23b)

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (9efbed8)

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (96072a0)

High

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (3ba96a4)

High

Medium

Uh oh!

mariusvniekerk commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (ddb71c5)

🔴 High

🟡 Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (12c5e5b)

High

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (64c443e)

Medium Severity

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (2fe9057)

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (2d0dd4a)

High

Medium

Uh oh!

roborev-ci bot commented Mar 9, 2026

roborev: Combined Review (e5b2276)

Uh oh!

Uh oh!

wesm commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mariusvniekerk commented Mar 9, 2026 •

edited

Loading

roborev: Combined Review (`f9e90f2`)

roborev: Combined Review (`c1bc23b`)

roborev: Combined Review (`9efbed8`)

roborev: Combined Review (`96072a0`)

roborev: Combined Review (`3ba96a4`)

mariusvniekerk commented Mar 9, 2026 •

edited

Loading

roborev: Combined Review (`ddb71c5`)

roborev: Combined Review (`12c5e5b`)

roborev: Combined Review (`64c443e`)

roborev: Combined Review (`2fe9057`)

roborev: Combined Review (`2d0dd4a`)

roborev: Combined Review (`e5b2276`)