-
Notifications
You must be signed in to change notification settings - Fork 146
Initial set of system-reinstall-bootc integration tests #1348
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
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
This PR adds a new set of integration tests for the system-reinstall-bootc functionality. The key changes include:
- Introducing a new enum variant (SystemReinstall) and corresponding test module (system_reinstall.rs) to handle system-reinstall-bootc tests.
- Updating supporting modules (install.rs), documentation (README.md), Cargo.toml, and the CI workflow (ci.yml) to integrate these 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 |
|---|---|
| tests-integration/src/tests-integration.rs | Integrates the new SystemReinstall variant into the test CLI interface. |
| tests-integration/src/system_reinstall.rs | Implements two integration tests for system-reinstall-bootc functionality. |
| tests-integration/src/install.rs | Exposes helper functions used by both system-reinstall and install tests. |
| tests-integration/README.md | Updates documentation with a section for system-reinstall tests. |
| tests-integration/Cargo.toml | Adds the rexpect dependency required for system-reinstall testing. |
| .github/workflows/ci.yml | Adds a CI step to run system-reinstall tests. |
Comments suppressed due to low confidence (1)
tests-integration/src/system_reinstall.rs:24
- [nitpick] Consider providing a more detailed error message or handling cases with multiple deployment directories if that situation might occur in practice.
assert_eq!(entries.len(), 1, "Expected exactly one deployment directory");
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.
Code Review
This pull request introduces a valuable set of integration tests for the system-reinstall-bootc functionality. The tests cover the default behavior and a disk space check scenario, utilizing rexpect for PTY interaction, which is appropriate for this kind of CLI testing. The changes to existing files to make functions pub(crate) for reusability are logical.
Overall, the additions are well-structured and enhance the test coverage significantly. I've identified a couple of areas where the robustness of the tests could be improved, detailed in the comments below. No style guide was explicitly provided, so feedback is based on general Rust best practices and idiomatic code.
Summary of Findings
- Fragile
dfoutput parsing: The testdisk space checkparsesdfoutput using fixed line and field indices, which could be brittle ifdfoutput format varies. This was commented on withmediumseverity. - Overly permissive regex in PTY expectation: The regex used to match the podman command in
system-reinstall-bootcoutput uses(.*)for a temporary filename, which is less specific than it could be. This was commented on withmediumseverity. - Cargo.toml dependency sorting (tests-integration): In
tests-integration/Cargo.toml, the newly addedrexpectdependency is not alphabetically sorted with other dependencies. This is a minor style issue (low severity) and was not commented on directly due to review settings.
Merge Readiness
The pull request is a good step towards comprehensive testing for system-reinstall-bootc. However, there are a couple of medium severity issues identified related to the robustness of test assertions (parsing df output and regex specificity). It's recommended to address these points to improve test stability before merging.
As an AI, I am not authorized to approve pull requests. Please ensure further review and approval from authorized maintainers.
3b277db to
90cd81b
Compare
cgwalters
left a comment
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.
Nice! I just have some minor nits.
I've been thinking a lot about CI lately and our sprawl around different systems BTW, right now it's a bit annoying to replicate the things we have triggered via GHA outside of it. Totally good as is, just noting.
This adds a few basic integration tests for system-reinstall-bootc, adds a system-reinstall option to tests-integration to run them, and executes them as part of the github action. Signed-off-by: ckyrouac <[email protected]>
90cd81b to
72c3c74
Compare
This adds a few basic integration tests for system-reinstall-bootc, adds a system-reinstall option to tests-integration to run them, and executes them as part of the github action.