feat: Improve auto-updater coverage and fix CI coverage command#9
feat: Improve auto-updater coverage and fix CI coverage command#9echobt merged 2 commits intoPlatformNetwork:mainfrom
Conversation
- add test-only pull hook, shutdown delay toggle, and new unit tests to `updater.rs`, plus extra coverage for `version.rs` and `watcher.rs` - update the CI workflow to use distinct output flags for json vs html in the `cargo llvm-cov nextest` step, resolving the duplicate `--output-path` error
📝 WalkthroughWalkthroughAdds test-only hook support to the auto-updater pull flow, expands unit tests across updater, version, and watcher modules, and updates the CI workflow to change the cargo-llvm-cov HTML output option. Changes
Sequence Diagram(s)(omitted — changes are test hooks and unit tests without a new multi-component runtime control flow that warrants a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @crates/auto-updater/src/updater.rs:
- Around line 280-286: The test currently calls
updater.check_and_update().await.unwrap() and then discards the boolean returned
by matches!(result, UpdateResult::NoUpdateAvailable |
UpdateResult::AlreadyUpToDate), so the assertion has no effect; replace the bare
matches! usage with an actual assertion such as assert!(matches!(result,
UpdateResult::NoUpdateAvailable | UpdateResult::AlreadyUpToDate)) (or an
appropriate assert_eq!/matches! variant) so the test fails when
updater.check_and_update().await.unwrap() does not return one of the expected
UpdateResult variants.
🧹 Nitpick comments (1)
crates/auto-updater/src/updater.rs (1)
311-322: Edge case:future_version()returns current version when all components areu32::MAX.While extremely unlikely in practice, if
currenthas all components atu32::MAX, this returns the same version rather than a "future" one. The test using this helper (make_future_requirement) would then test the "already up to date" path instead of the intended "needs update" path.This is a minor robustness concern for test reliability. Consider adding a
panic!or using a sentinel version for clarity.🔎 Optional: fail explicitly if no future version is possible
} else if current.major < u32::MAX { Version::new(current.major + 1, 0, 0) } else { - current + panic!("Cannot create future version: all components are u32::MAX") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/auto-updater/src/updater.rscrates/auto-updater/src/version.rscrates/auto-updater/src/watcher.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/auto-updater/src/updater.rs (2)
crates/auto-updater/src/version.rs (2)
new(16-22)current(25-31)crates/auto-updater/src/watcher.rs (2)
new(20-29)new(113-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Clippy
🔇 Additional comments (8)
.github/workflows/ci.yml (1)
74-74: LGTM!The separation of
--output-pathfor JSON and--output-dirfor HTML correctly resolves the duplicate flag conflict. The downstream references tocoverage-html/in the artifact upload and GitHub Pages deployment are consistent with this change.crates/auto-updater/src/version.rs (1)
138-148: LGTM!The new tests provide good coverage for:
- Verifying
Defaulttrait consistency withcurrent()- Validating
parse()correctly rejects malformed version strings (too few/many parts, non-numeric)crates/auto-updater/src/watcher.rs (1)
158-191: LGTM!The new tests provide comprehensive coverage for:
- Current version accessor returning expected value
- Builder pattern with custom and default intervals
- Status correctly reports
UpToDatewhen requirement matches current versionAccessing
check_intervaldirectly is acceptable since tests are in the same module.crates/auto-updater/src/updater.rs (5)
9-30: LGTM!The test-only infrastructure is well-designed:
PullHooktype alias enables mocking Docker pull operationsSHUTDOWN_DELAY_SECSeliminates test delays while preserving production graceful shutdown timing- Conditional compilation keeps production binaries clean
110-118: LGTM!The conditional handling for mandatory updates is correct. In tests, returning
Ok(())allows the test to continue and verify behavior, while production code exits to trigger orchestrator restart.
126-129: LGTM!Clean hook injection pattern that short-circuits real Docker calls during tests while preserving production behavior.
336-449: LGTM!Excellent test coverage for the update flow:
test_handle_update_triggers_restart_signalverifies pull hook invocation and restart signal emissiontest_check_and_update_reports_updatedvalidates the full update path with watcher integrationtest_handle_update_mandatory_triggers_shutdownconfirms mandatory flag handlingThe hook-based approach cleanly isolates tests from Docker dependencies.
451-469: LGTM!Good negative test case verifying that pull errors propagate correctly through
handle_update.
updater.rs, plus extra coverage forversion.rsandwatcher.rscargo llvm-cov nexteststep, resolving the duplicate--output-patherrorSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.