-
Notifications
You must be signed in to change notification settings - Fork 6
Add fail during execution and start test + bug fixes + clean up of list_containers #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ner into two functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds new failure handling tests, status variants, and refactors orchestration logic to improve error reporting and cleanup.
- Introduce tests for pods that fail at start or during execution and refactor
basic_testintoexecute_wrapper - Extend orchestrator to report
FailedToStartPoderrors and newStarting/Undefinedstatuses, with polling for results - Clean up container listing filters and centralize run-info extraction in core implementation
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/orchestrator.rs | Refactored test helper, added failure tests, and cleaned up list assertions |
| src/uniffi/orchestrator/mod.rs | Added Starting/Undefined statuses and derived PartialEq on PodRun |
| src/uniffi/orchestrator/docker.rs | Wrapped start_container errors in a FailedToStartPod kind and added wait polling |
| src/uniffi/error.rs | Defined FailedToStartPod error variant and helper methods |
| src/core/orchestrator/docker.rs | Centralized run-info extraction and added name-based filters |
| src/core/error.rs | Updated Debug impl to include the new error variant |
Comments suppressed due to low confidence (5)
tests/orchestrator.rs:168
- The comment says the pod should be deleted, but the assertion below checks that it exists in the list. Update the comment to match the intent, or adjust the assertion.
// Make sure the pod has been deleted after failing to start
src/uniffi/error.rs:137
- The doc comment is incomplete. Expand it to explain when
get_container_namereturnsSomeand what it represents.
/// Returns container name if the
src/uniffi/orchestrator/mod.rs:27
- New
StartingandUndefinedstatus variants were added but not exercised by any tests. Consider adding tests that cover these states to ensure correct handling.
Starting,
tests/orchestrator.rs:1
- The crate-level attribute block is indented by one space, which prevents it from being applied. Move
#![expect(...)]to column 0 so it’s recognized.
#![expect(
src/core/orchestrator/docker.rs:12
- There is no
bollard::secretmodule. ImportContainerInspectResponseandContainerSummaryfrombollard::modelsinstead to compile correctly.
secret::{ContainerInspectResponse, ContainerSummary},
tests/orchestrator.rs
Outdated
| pod.image = "alpine:3.14".to_owned(); | ||
| pod.command = "python file_does_not_exist.py".to_owned(); | ||
|
|
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment is immediately overwritten on the next line. Remove the redundant pod.image (and the first pod.command) to keep the test setup clear.
| pod.image = "alpine:3.14".to_owned(); | |
| pod.command = "python file_does_not_exist.py".to_owned(); |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Added
Other stuff:
Agent: Add configurable storage ofPodResult#94 to allow test cases above to work