Skip to content

test(tracker): close Diagnose/Audit/Doctor API coverage gaps and align MostRecent warning routing#138

Merged
clintecker merged 3 commits intomainfrom
copilot/add-coverage-tests-for-diagnose-audit-doctor
Apr 21, 2026
Merged

test(tracker): close Diagnose/Audit/Doctor API coverage gaps and align MostRecent warning routing#138
clintecker merged 3 commits intomainfrom
copilot/add-coverage-tests-for-diagnose-audit-doctor

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 21, 2026

This PR addresses outstanding test coverage gaps called out in review for the library APIs added in #101, specifically around untested error/selection branches in Diagnose/Audit/Doctor and repetitive CLI doctor flag-parsing tests.

  • Diagnose/resolve-path coverage

    • Added DiagnoseMostRecent coverage for selecting the newest run by checkpoint timestamp and delegating to Diagnose.
    • Added MostRecentRunID branch coverage for:
      • newest valid checkpoint selection
      • missing runs directory (no runs found)
      • runs present but no valid checkpoints
      • malformed checkpoint handling while still selecting a valid run
    • Added missing ResolveRunDir no-match test (no run found matching %q).
  • Diagnose malformed status warning path

    • Added a test for corrupted node status.json to verify warning emission via DiagnoseConfig.LogWriter and graceful continuation (no hard failure).
  • Audit error-path coverage

    • Added tests for:
      • missing checkpoint.json
      • malformed checkpoint JSON
      • empty run directory
  • Doctor probe path without real credentials

    • Introduced a minimal test seam in tracker_doctor.go:
      • probeProviderFn variable (defaults to probeProvider)
      • checkProviders now calls probeProviderFn(...)
    • Added unit test for ProbeProviders: true that mocks probe behavior and verifies probe-path execution without network/API keys.
  • CLI doctor flag parsing cleanup

    • Refactored 8 structurally identical TestParseFlagsDoctor* tests in cmd/tracker/doctor_test.go into one table-driven test while preserving assertions and coverage intent.
  • Feedback follow-up: library warning side effects

    • Removed direct os.Stderr warning output from exported MostRecentRunID so library callers do not get unavoidable stderr side effects.
    • Routed malformed-checkpoint warnings through configurable writer flow when diagnosing most-recent runs (DiagnoseMostRecent uses DiagnoseConfig.LogWriter).
    • Updated tests accordingly:
      • MostRecentRunID remains silent by default.
      • DiagnoseMostRecent emits malformed-checkpoint warnings via the provided LogWriter.
// tracker_doctor.go
var probeProviderFn = probeProvider

// checkProviders(...)
ok, probeMsg, isAuth := probeProviderFn(ctx, p, key)

Copilot AI changed the title [WIP] Add tests for coverage gaps in Diagnose/Audit/Doctor library APIs test(tracker): close Diagnose/Audit/Doctor API coverage gaps and refactor doctor flag parsing tests Apr 21, 2026
Copilot AI requested a review from clintecker April 21, 2026 17:05
@clintecker clintecker requested a review from Copilot April 21, 2026 18:01
@clintecker clintecker marked this pull request as ready for review April 21, 2026 18:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens test coverage for the newly promoted tracker library APIs (Diagnose/Audit/Doctor) by exercising previously untested selection and error/warning paths, and it refactors repetitive CLI doctor flag-parsing tests into a table-driven form.

Changes:

  • Added coverage for DiagnoseMostRecent, MostRecentRunID, ResolveRunDir, and Diagnose warning-on-malformed-status behavior.
  • Added Audit error-path coverage for missing/malformed checkpoints and empty run directories.
  • Added a minimal Doctor probe seam (probeProviderFn) to enable credential-free provider probing tests, plus refactored CLI doctor flag tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tracker_doctor.go Adds a probe function indirection to allow mocking provider probes in tests.
tracker_doctor_test.go Adds a unit test verifying probe-path execution via the probe seam.
tracker_diagnose_test.go Adds tests for newest-run selection and malformed status warning continuation.
tracker_audit_test.go Adds tests for Audit checkpoint-related error paths.
tracker_activity.go Updates MostRecentRunID to emit warnings on malformed checkpoints.
tracker_activity_test.go Adds tests for no-match resolution and most-recent-run selection/error branches (including stderr warning capture).
cmd/tracker/doctor_test.go Refactors multiple flag-parsing tests into a single table-driven test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tracker_activity.go
Comment on lines +84 to +86
if !os.IsNotExist(err) {
fmt.Fprintf(os.Stderr, "warning: cannot load checkpoint for run %s: %v\n", e.Name(), err)
}
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

MostRecentRunID now writes warnings directly to os.Stderr for malformed checkpoints. This introduces an unavoidable side effect for library callers (and for DiagnoseMostRecent, which calls MostRecentRunID) and is inconsistent with the existing pattern where warnings are routed through configurable LogWriter fields that default to io.Discard (e.g., DiagnoseConfig/AuditConfig). Consider keeping MostRecentRunID silent and surfacing malformed-checkpoint warnings via a configurable writer (e.g., passed in from DiagnoseMostRecent/CLI) or by returning warnings alongside the selected run ID.

Suggested change
if !os.IsNotExist(err) {
fmt.Fprintf(os.Stderr, "warning: cannot load checkpoint for run %s: %v\n", e.Name(), err)
}

Copilot uses AI. Check for mistakes.
@clintecker
Copy link
Copy Markdown
Collaborator

@copilot reivew all the feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 21, 2026

@copilot reivew all the feedback

Addressed the feedback in commit 7e5a992. MostRecentRunID is silent again (no direct os.Stderr writes), and malformed-checkpoint warnings are now routed through DiagnoseMostRecent via DiagnoseConfig.LogWriter. Tests were updated to assert both behaviors.

@clintecker clintecker requested a review from Copilot April 21, 2026 18:17
Copilot AI changed the title test(tracker): close Diagnose/Audit/Doctor API coverage gaps and refactor doctor flag parsing tests test(tracker): close Diagnose/Audit/Doctor API coverage gaps and align MostRecent warning routing Apr 21, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@clintecker clintecker merged commit a7c56b2 into main Apr 21, 2026
5 checks passed
@clintecker clintecker mentioned this pull request Apr 21, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(tracker): coverage gaps for Diagnose/Audit/Doctor library APIs

3 participants