✅ Expand test case for create subcommand #2509
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUnix-specific integration tests are added for the Changes
Sequence Diagram(s)(omitted — changes are test additions and do not introduce new multi-component control flow warranting a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @ChanTsune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the test coverage for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request expands the test suite for the create subcommand by adding tests for the --one-file-system option. The new tests are well-structured, covering both the case where different filesystems are involved (using tmpfs on Linux) and the case where all files are on the same filesystem.
I've provided a few suggestions to improve the new test file's robustness and maintainability, mainly concerning resource cleanup in tests that modify system state, reducing code duplication, and improving readability by removing hardcoded strings. Overall, this is a great addition to the test suite.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/tests/cli/create/option_one_file_system.rs (2)
39-44: Consider adding cleanup on test completion.The test removes the directory at the start but doesn't clean up at the end. While this is common in tests (start-clean approach), consider also cleaning up on success to avoid accumulating test artifacts, especially since this test creates archives.
Additionally, if the test panics after mounting but before the umount on line 102-104, the mount will remain. Consider using a scope guard or cleanup struct to ensure unmounting occurs even on panic.
🔎 Suggested approach using Drop guard
struct MountGuard { path: &'static str, } impl Drop for MountGuard { fn drop(&mut self) { let _ = Command::new("umount").arg(self.path).status(); } } // Usage in test: let _guard = MountGuard { path: "option_one_file_system_mount/in/subdir" };
229-289: Consider extracting shared test logic to reduce duplication.The
create_with_one_file_system_same_fstest is identical between Linux and non-Linux Unix platforms. Consider extracting the common logic into a helper function that both platform modules can call.🔎 Suggested refactor
+fn create_with_one_file_system_same_fs_impl() { + setup(); + + let _ = fs::remove_dir_all("option_one_file_system_local"); + fs::create_dir_all("option_one_file_system_local/in/subdir").unwrap(); + + fs::write("option_one_file_system_local/in/file1.txt", "content1").unwrap(); + fs::write("option_one_file_system_local/in/subdir/file2.txt", "content2").unwrap(); + + cli::Cli::try_parse_from([ + "pna", + "--quiet", + "create", + "option_one_file_system_local/archive.pna", + "--overwrite", + "--keep-dir", + "--unstable", + "--one-file-system", + "option_one_file_system_local/in/", + ]) + .unwrap() + .execute() + .unwrap(); + + let mut seen = HashSet::new(); + archive::for_each_entry("option_one_file_system_local/archive.pna", |entry| { + seen.insert(entry.header().path().to_string()); + }) + .unwrap(); + + assert!(seen.contains("option_one_file_system_local/in")); + assert!(seen.contains("option_one_file_system_local/in/file1.txt")); + assert!(seen.contains("option_one_file_system_local/in/subdir")); + assert!(seen.contains("option_one_file_system_local/in/subdir/file2.txt")); + assert_eq!(seen.len(), 4); +} #[cfg(target_os = "linux")] mod platform { // ... existing code ... #[test] fn create_with_one_file_system_same_fs() { - // ... duplicated implementation ... + super::create_with_one_file_system_same_fs_impl() } } #[cfg(all(unix, not(target_os = "linux")))] mod platform { #[test] fn create_with_one_file_system_same_fs() { - // ... duplicated implementation ... + super::create_with_one_file_system_same_fs_impl() } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/tests/cli/create.rscli/tests/cli/create/option_one_file_system.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tests/cli/create/option_one_file_system.rs (1)
cli/tests/cli/utils/archive.rs (1)
for_each_entry(122-127)
⏰ 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: tier1 (macos-latest, stable)
🔇 Additional comments (5)
cli/tests/cli/create/option_one_file_system.rs (4)
1-4: LGTM!Imports are appropriate for the test functionality:
HashSetfor tracking entries,fsfor file operations, and the necessary test utilities.
12-32: Helper function is well-structured with proper cleanup.The
can_mount_tmpfshelper correctly checks root privileges before attempting mount operations and ensures cleanup in all code paths. The use ofunsafe { libc::getuid() }is acceptable here given this is test code and the operation is straightforward.
106-167: Thorough verification logic.The archive verification is comprehensive: it checks both the presence of expected entries and the absence of entries that should be excluded by
--one-file-system. The entry count assertions provide an additional safeguard against unexpected entries. Good use of descriptive assertion messages for debugging.
170-226: Good complementary test case.This test correctly validates that
--one-file-systemdoesn't incorrectly exclude content when everything is on the same filesystem. The test logic is sound.cli/tests/cli/create.rs (1)
13-14: LGTM!The module declaration is correctly guarded with
#[cfg(unix)]to match the platform-specific nature of the--one-file-systemtests, and follows the existing naming conventions in this file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/tests/cli/create/option_one_file_system.rs (2)
108-110: Redundant explicit umount call.This explicit umount is now handled by the
scopeguard::defer!block above (lines 50-53), so it can be removed.🔎 Proposed fix
- let _ = Command::new("umount") - .arg("option_one_file_system_mount/in/subdir") - .status(); - // Verify archive with --one-file-system
235-296: Duplicate test function should be consolidated.This test is identical to the one in the Linux module (lines 180-232). Since
--one-file-systembehavior on the same filesystem is not Linux-specific, consider moving this function to the top level of the file (outside bothmod platformblocks) to eliminate duplication. The same missing cleanup issue applies here as well.🔎 Proposed refactor - move to top level
// At top level of file, outside both platform modules: /// Precondition: A directory contains files and subdirectories all on the same filesystem. /// Action: Run `pna create` with `--one-file-system`. /// Expectation: All entries are included in the archive. #[cfg(unix)] #[test] fn create_with_one_file_system_same_fs() { setup(); let _ = fs::remove_dir_all("option_one_file_system_local"); fs::create_dir_all("option_one_file_system_local/in/subdir").unwrap(); scopeguard::defer! { let _ = fs::remove_dir_all("option_one_file_system_local"); }; // ... rest of test ... }Then remove the
#[cfg(all(unix, not(target_os = "linux")))] mod platformblock entirely.
🧹 Nitpick comments (1)
cli/tests/cli/create/option_one_file_system.rs (1)
180-232: Missing cleanup for test directory.Unlike
create_with_one_file_system, this test does not clean up theoption_one_file_system_localdirectory after completion. Consider adding a cleanup mechanism (e.g.,scopeguard::defer!or explicit removal at end).🔎 Proposed fix
fn create_with_one_file_system_same_fs() { setup(); let _ = fs::remove_dir_all("option_one_file_system_local"); fs::create_dir_all("option_one_file_system_local/in/subdir").unwrap(); + + scopeguard::defer! { + let _ = fs::remove_dir_all("option_one_file_system_local"); + }; fs::write("option_one_file_system_local/in/file1.txt", "content1").unwrap();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/tests/cli/create/option_one_file_system.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tests/cli/create/option_one_file_system.rs (1)
cli/tests/cli/utils/archive.rs (1)
for_each_entry(122-127)
🔇 Additional comments (3)
cli/tests/cli/create/option_one_file_system.rs (3)
1-4: LGTM!Imports are appropriate for the test utilities needed.
12-32: LGTM!The capability check is well-designed: it verifies both root privileges and actual mount capability, with proper cleanup regardless of outcome.
112-174: LGTM!The verification logic is thorough: it checks both the presence/absence of specific entries and validates total entry counts for both archive variants.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChanTsune
force-pushed
the
cli/create/test-option-one-file-system
branch
from
256df34 to
e103b5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/tests/cli/create/option_one_file_system.rs (2)
116-118: Remove redundant umount call.This explicit
umountis unnecessary since thescopeguard::defer!block at lines 50-53 already handles cleanup, including unmounting. The defer block ensures cleanup even if the test panics, making this redundant.🔎 Suggested fix
- let _ = Command::new("umount") - .arg("option_one_file_system_mount/in/subdir") - .status(); - // Verify archive with --one-file-systemBased on learnings, a past review identified this redundancy.
250-315: Deduplicate the same-filesystem test.The
create_with_one_file_system_same_fstest is duplicated between the Linux platform module (lines 191-247) and the non-Linux Unix module (lines 250-315). Since this test doesn't use platform-specific features (unlike the tmpfs test which requires Linux), it should be moved to the top level outside both platform modules.🔎 Suggested refactor
Move the test outside the platform-specific modules:
+/// Precondition: A directory contains files and subdirectories all on the same filesystem. +/// Action: Run `pna create` with `--one-file-system`. +/// Expectation: All entries are included in the archive. +#[test] +fn create_with_one_file_system_same_fs() { + setup(); + + let _ = fs::remove_dir_all("option_one_file_system_local"); + fs::create_dir_all("option_one_file_system_local/in/subdir").unwrap(); + + fs::write("option_one_file_system_local/in/file1.txt", "content1").unwrap(); + fs::write( + "option_one_file_system_local/in/subdir/file2.txt", + "content2", + ) + .unwrap(); + + cli::Cli::try_parse_from([ + "pna", + "--quiet", + "create", + "option_one_file_system_local/archive.pna", + "--overwrite", + "--keep-dir", + "--unstable", + "--one-file-system", + "option_one_file_system_local/in/", + ]) + .unwrap() + .execute() + .unwrap(); + + let mut seen = HashSet::new(); + archive::for_each_entry("option_one_file_system_local/archive.pna", |entry| { + seen.insert(entry.header().path().to_string()); + }) + .unwrap(); + + assert!( + seen.contains("option_one_file_system_local/in"), + "input directory should be included" + ); + assert!( + seen.contains("option_one_file_system_local/in/file1.txt"), + "file1.txt should be included" + ); + assert!( + seen.contains("option_one_file_system_local/in/subdir"), + "subdir directory should be included" + ); + assert!( + seen.contains("option_one_file_system_local/in/subdir/file2.txt"), + "file2.txt should be included" + ); + assert_eq!( + seen.len(), + 4, + "Expected exactly 4 entries (2 directories and 2 files), but found {}: {seen:?}", + seen.len() + ); +} + #[cfg(target_os = "linux")] mod platform { // ... keep only create_with_one_file_system and can_mount_tmpfs - // Remove create_with_one_file_system_same_fs } -#[cfg(all(unix, not(target_os = "linux")))] -mod platform { - // Remove entire module as it becomes empty -}This eliminates duplication and ensures the test runs for all Unix platforms.
Based on learnings, a past review identified this duplication.
🧹 Nitpick comments (1)
cli/tests/cli/create/option_one_file_system.rs (1)
46-69: Verify the use of hardcoded path strings.The test uses the hardcoded string
"option_one_file_system_mount"repeatedly throughout. While this works, consider extracting it to a constant at the beginning of the test for better maintainability.🔎 Suggested refactor to use a constant
fn create_with_one_file_system() { setup(); + const TEST_DIR: &str = "option_one_file_system_mount"; + const MOUNT_SUBDIR: &str = "option_one_file_system_mount/in/subdir"; + if !can_mount_tmpfs() { eprintln!("Skipping test: cannot create tmpfs mount (requires root)"); return; } - let _ = fs::remove_dir_all("option_one_file_system_mount"); - fs::create_dir_all("option_one_file_system_mount/in/subdir").unwrap(); + let _ = fs::remove_dir_all(TEST_DIR); + fs::create_dir_all(MOUNT_SUBDIR).unwrap(); // Defer cleanup to ensure it runs even if the test panics. scopeguard::defer! { - let _ = Command::new("umount").arg("option_one_file_system_mount/in/subdir").status(); - let _ = fs::remove_dir_all("option_one_file_system_mount"); + let _ = Command::new("umount").arg(MOUNT_SUBDIR).status(); + let _ = fs::remove_dir_all(TEST_DIR); };Based on learnings, a past review suggested this refactor for readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/Cargo.tomlcli/tests/cli/create.rscli/tests/cli/create/option_one_file_system.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/tests/cli/create/option_one_file_system.rs (1)
cli/tests/cli/utils/archive.rs (1)
for_each_entry(122-127)
🔇 Additional comments (7)
cli/tests/cli/create.rs (1)
23-24: LGTM!The module inclusion is correctly placed and properly guarded with
#[cfg(unix)]for Unix-specific test coverage of the--one-file-systemoption.cli/tests/cli/create/option_one_file_system.rs (5)
1-11: LGTM!Imports are well-organized and appropriate for the test scenarios.
12-32: LGTM!The helper function correctly checks for mount capabilities with appropriate UID verification and cleanup. The use of
unsafeforlibc::getuid()is necessary and correct.
71-114: LGTM!The test correctly verifies that files are on different filesystems using device IDs, then creates both archives (with and without
--one-file-system) for comparison. The assertion on different device IDs provides strong evidence of the test setup.
120-184: LGTM!The verification logic correctly validates that
--one-file-systemexcludes entries from the mounted tmpfs (2 entries) while the default behavior includes all entries (4 entries). The assertions are clear and comprehensive.
187-247: LGTM!This test correctly validates that
--one-file-systemincludes all entries when they reside on the same filesystem, confirming the flag only affects cross-filesystem boundaries.cli/Cargo.toml (1)
72-72: Scopeguard version 1.2.0 is valid and current. Version 1.2.0 is available and is the recommended specification. No known security advisories have been found for this crate.
github-actions
bot
added
the
dependencies
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.