test: improve coverage with focused seam tests#111
Merged
Conversation
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefactors internal helpers into crate-visible functions, extracts many helpers from source modules, removes inline #[cfg(test)] unit modules, and relocates tests into a comprehensive external test suite under tests/ and tests/crate/. Changes
Sequence Diagram(s)(Not applicable; changes are refactors and test reorganisation without new multi-component sequential flows.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -95 |
TIP This summary will be updated as you push new changes. Give us feedback
seapagan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/format.rs (2)
48-67: Consider removing duplicated assertions intest_format_size_extreme.Lines 50-51, 56-57, 59-60, and 62-63 duplicate assertions already covered in
test_format_size. The only unique assertion in this test is the PB case (lines 53-54) and the boundary case at 1 MB - 1 (lines 65-66). Consider renaming or trimming this test to focus on truly extreme or boundary values.♻️ Suggested refactor to remove duplicates
#[test] fn test_format_size_extreme() { - let (size, unit) = human_readable_format(1024 * 1024 * 1024 * 1024); - assert_eq!(format!("{:.1} {}", size, unit), "1.0 TB"); - let (size, unit) = human_readable_format(1024 * 1024 * 1024 * 1024 * 1024); assert_eq!(format!("{:.1} {}", size, unit), "1.0 PB"); - let (size, unit) = human_readable_format(1024); - assert_eq!(format!("{:.1} {}", size, unit), "1.0 KB"); - - let (size, unit) = human_readable_format(1024 * 1024); - assert_eq!(format!("{:.1} {}", size, unit), "1.0 MB"); - - let (size, unit) = human_readable_format(1023); - assert_eq!(format!("{:.1} {}", size, unit), "1023.0 B"); - let (size, unit) = human_readable_format(1024 * 1024 - 1); assert_eq!(format!("{:.1} {}", size, unit), "1024.0 KB"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/format.rs` around lines 48 - 67, The test test_format_size_extreme duplicates many assertions from test_format_size; trim it to only the truly extreme/boundary cases by removing the redundant checks for 1 KB, 1 MB, and 1023 B and keeping the PB case (human_readable_format(1024^5)) and the boundary case just below 1 MB (human_readable_format(1024 * 1024 - 1)); optionally rename the test to test_format_size_boundaries or test_format_size_extremes to reflect its focused purpose and update assertions to only validate those remaining cases using the human_readable_format function and its returned (size, unit) tuple.
69-81: Minor overlap betweentest_format_modeandtest_format_mode_permissions.Both tests assert
mode_to_rwx(0o777)returns"rwxrwxrwx". Consider consolidating or ensuring each test has a distinct purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/format.rs` around lines 69 - 81, Two tests, test_format_mode and test_format_mode_permissions, both assert mode_to_rwx(0o777) == "rwxrwxrwx" which is redundant; update one of the tests so each has a distinct purpose — either remove the duplicated assertion from test_format_mode or replace it in test_format_mode_permissions with a different meaningful case (e.g., a mixed permission like 0o711 or 0o600) to cover additional behavior, keeping references to mode_to_rwx, test_format_mode, and test_format_mode_permissions to locate the assertions.tests/crate/icons.rs (1)
13-33: Test relies onCargo.tomlexisting in working directory.Line 15 uses
fs::metadata("Cargo.toml")to obtain metadata for testing icon selection. This works forcargo testbut could be fragile if tests are run from a different directory. Consider usingtempfileto create a test file with known metadata for better isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crate/icons.rs` around lines 13 - 33, The test currently depends on an external Cargo.toml via fs::metadata("Cargo.toml") which is brittle; modify test_get_item_icon_uses_known_file_types_and_names to create an isolated tempfile::TempDir, create files named "test.unknown", "test.rs", "Cargo.toml", and "nested/config/.gitignore" inside that temp dir (using std::fs::create_dir_all and File::create), obtain each file's metadata and pass Path::new relative to that tempdir to get_item_icon, then assert icons as before; this keeps the test independent of the working directory and uses the same function names (get_item_icon) and test name to locate the change.tests/crate/render.rs (1)
56-75: Fuzzy time test looks correct but verify boundary behaviour.The test creates a timestamp exactly 2 hours ago and expects "2 hours ago". This should be deterministic, but if the fuzzy time implementation has boundary conditions (e.g., 2 hours and 1 second showing "2 hours ago" vs "3 hours ago"), minor timing variations during test execution could cause flakiness. Consider using a more robust time offset (e.g., 2.5 hours) to avoid potential boundary issues.
♻️ Suggested safer time offset
let info = test_file_info( "aged.txt", None, 128, SystemTime::now() - .checked_sub(Duration::from_secs(2 * 60 * 60)) + .checked_sub(Duration::from_secs(2 * 60 * 60 + 30 * 60)) // 2.5 hours .unwrap(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crate/render.rs` around lines 56 - 75, The test test_build_long_format_table_uses_fuzzy_time_when_requested uses an exact 2-hour offset which risks flakiness at boundary conditions; change the timestamp subtraction in the call to test_file_info to use a safer offset (e.g., 2.5 hours) by replacing Duration::from_secs(2 * 60 * 60) with a duration representing 2.5 hours (such as Duration::from_secs(2 * 60 * 60 + 30 * 60)) so Params { fuzzy_time: true } and the assertions still validate "2 hours ago" without timing boundary issues.tests/crate/settings.rs (1)
14-21: Unix-specific path assumption in test.Line 18 uses
/tmp/homewhich assumes a Unix filesystem. If Windows support is needed, consider usingPathBuf::from("C:\\tmp\\home")conditionally or a platform-agnostic approach. However, this appears acceptable given the crate's Unix-focused nature (permissions, symlinks, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crate/settings.rs` around lines 14 - 21, The test uses a hard-coded Unix-style path string; update test_config_path_from_home to construct the input and expected PathBufs using PathBuf and join (e.g., let base = PathBuf::from("/tmp/home") or use tempfile::tempdir() for a temp dir, then let expected = base.join(".config").join("lsplus").join("config.toml")) so the assertion against config_path_from_home(Some(base)) is platform-agnostic; modify the test function name test_config_path_from_home_handles_some_and_none and references to use these PathBuf joins instead of a literal "/tmp/home" string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/gitignore.rs`:
- Around line 48-53: Replace the panic-on-error behavior when parsing repository
ignore files by handling builder.build() failures gracefully: instead of calling
builder.build().expect(...), catch the error and return None or an empty matcher
for gitignore_matcher so directory listing continues; apply the same
non-panicking pattern used by build_git_global_matcher for the other parser (the
expect at the info/exclude parsing referenced by build_git_exclude_matcher).
Locate the builder.build() call that produces gitignore_matcher and the other
expect around the info/exclude parsing, replace expects with error handling that
logs/debugs the parse error and yields a no-op matcher or None to preserve
existing behavior rather than crashing.
In `@tests/crate/file.rs`:
- Around line 1-21: The file imports std::os::unix::fs::PermissionsExt
unconditionally while tests like
test_create_file_info_handles_executables_and_large_files and
test_create_file_info_handles_regular_files_symlinks_and_special_cases call
Unix-only APIs (fs::Permissions::from_mode and std::os::unix::fs::symlink),
causing Windows compilation failures; fix by gating the Unix-only import and/or
tests with #[cfg(unix)] — either mark the PermissionsExt import with
#[cfg(unix)] and add #[cfg(unix)] on the two test functions (or wrap the whole
file with #[cfg(unix)] if all tests are Unix-only) so the Unix-specific symbols
are only compiled on Unix platforms.
In `@tests/main.rs`:
- Around line 9-23: The test uses a 0o000 directory (blocked_dir) to force a
failure, which is flaky on elevated runners; change the test to use a
deterministic failure input or skip when privileged: either (a) create a regular
temp file (e.g., temp_file.path()) and pass that path to
Command::cargo_bin("lsp") instead of blocked_dir so the binary errors
deterministically when a directory is expected, or (b) detect elevated
privileges at test start (e.g., via geteuid() == 0 using nix or users crate) and
early-return/skip the test; update the assertions accordingly and remove the
permission toggling on blocked_dir.
---
Nitpick comments:
In `@tests/crate/icons.rs`:
- Around line 13-33: The test currently depends on an external Cargo.toml via
fs::metadata("Cargo.toml") which is brittle; modify
test_get_item_icon_uses_known_file_types_and_names to create an isolated
tempfile::TempDir, create files named "test.unknown", "test.rs", "Cargo.toml",
and "nested/config/.gitignore" inside that temp dir (using
std::fs::create_dir_all and File::create), obtain each file's metadata and pass
Path::new relative to that tempdir to get_item_icon, then assert icons as
before; this keeps the test independent of the working directory and uses the
same function names (get_item_icon) and test name to locate the change.
In `@tests/crate/render.rs`:
- Around line 56-75: The test
test_build_long_format_table_uses_fuzzy_time_when_requested uses an exact 2-hour
offset which risks flakiness at boundary conditions; change the timestamp
subtraction in the call to test_file_info to use a safer offset (e.g., 2.5
hours) by replacing Duration::from_secs(2 * 60 * 60) with a duration
representing 2.5 hours (such as Duration::from_secs(2 * 60 * 60 + 30 * 60)) so
Params { fuzzy_time: true } and the assertions still validate "2 hours ago"
without timing boundary issues.
In `@tests/crate/settings.rs`:
- Around line 14-21: The test uses a hard-coded Unix-style path string; update
test_config_path_from_home to construct the input and expected PathBufs using
PathBuf and join (e.g., let base = PathBuf::from("/tmp/home") or use
tempfile::tempdir() for a temp dir, then let expected =
base.join(".config").join("lsplus").join("config.toml")) so the assertion
against config_path_from_home(Some(base)) is platform-agnostic; modify the test
function name test_config_path_from_home_handles_some_and_none and references to
use these PathBuf joins instead of a literal "/tmp/home" string.
In `@tests/format.rs`:
- Around line 48-67: The test test_format_size_extreme duplicates many
assertions from test_format_size; trim it to only the truly extreme/boundary
cases by removing the redundant checks for 1 KB, 1 MB, and 1023 B and keeping
the PB case (human_readable_format(1024^5)) and the boundary case just below 1
MB (human_readable_format(1024 * 1024 - 1)); optionally rename the test to
test_format_size_boundaries or test_format_size_extremes to reflect its focused
purpose and update assertions to only validate those remaining cases using the
human_readable_format function and its returned (size, unit) tuple.
- Around line 69-81: Two tests, test_format_mode and
test_format_mode_permissions, both assert mode_to_rwx(0o777) == "rwxrwxrwx"
which is redundant; update one of the tests so each has a distinct purpose —
either remove the duplicated assertion from test_format_mode or replace it in
test_format_mode_permissions with a different meaningful case (e.g., a mixed
permission like 0o711 or 0o600) to cover additional behavior, keeping references
to mode_to_rwx, test_format_mode, and test_format_mode_permissions to locate the
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51c6996c-e6ca-4e0f-aa96-3e8ab122fe88
📒 Files selected for processing (23)
src/app.rssrc/cli.rssrc/lib.rssrc/settings.rssrc/structs.rssrc/utils/file.rssrc/utils/format.rssrc/utils/fuzzy_time.rssrc/utils/gitignore.rssrc/utils/icons.rssrc/utils/render.rstests/crate/app.rstests/crate/cli.rstests/crate/file.rstests/crate/gitignore.rstests/crate/icons.rstests/crate/render.rstests/crate/settings.rstests/format.rstests/fuzzy_time.rstests/main.rstests/seams.rstests/structs.rs
💤 Files with no reviewable changes (3)
- src/utils/format.rs
- src/structs.rs
- src/utils/fuzzy_time.rs
Comment on lines
+9
to
+23
| let temp_dir = tempdir().unwrap(); | ||
| let blocked_dir = temp_dir.path().join("blocked"); | ||
| fs::create_dir(&blocked_dir).unwrap(); | ||
| fs::set_permissions(&blocked_dir, fs::Permissions::from_mode(0o000)) | ||
| .unwrap(); | ||
|
|
||
| let mut cmd = Command::cargo_bin("lsp").unwrap(); | ||
| cmd.arg(&blocked_dir) | ||
| .assert() | ||
| .failure() | ||
| .stderr(contains("Error:")); | ||
|
|
||
| fs::set_permissions(&blocked_dir, fs::Permissions::from_mode(0o755)) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
This failure path is runner-dependent.
A 0o000 directory only guarantees an error for an unprivileged process. On root-capable CI runners the binary can still read it, so this assertion becomes flaky. Please switch to a deterministic error input, or explicitly skip when running with elevated privileges.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/main.rs` around lines 9 - 23, The test uses a 0o000 directory
(blocked_dir) to force a failure, which is flaky on elevated runners; change the
test to use a deterministic failure input or skip when privileged: either (a)
create a regular temp file (e.g., temp_file.path()) and pass that path to
Command::cargo_bin("lsp") instead of blocked_dir so the binary errors
deterministically when a directory is expected, or (b) detect elevated
privileges at test start (e.g., via geteuid() == 0 using nix or users crate) and
early-return/skip the test; update the assertions accordingly and remove the
permission toggling on blocked_dir.
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This improves test coverage for the new feature work while continuing the shift toward behavior-focused checks at the
app,settings, andrenderseams instead of broad smoke-style coverage.The branch moves the relevant inline
srctests intotests/, adds focused seam coverage for the file and gitignore helpers, and keeps the extracted helper boundaries tighter by using crate-local tests andpub(crate)visibility where possible instead of widening the public API just for test access.The result is broader and more accurate production-code coverage, with tests centered on module behavior and boundary contracts rather than incidental implementation details.
Summary by CodeRabbit
Tests
Refactor
Bug Fixes