Skip to content

🧪 Add unit tests for path_to_string_lossy utility#55

Open
bashandbone wants to merge 6 commits intomainfrom
testing-improvement-path-to-string-lossy-16782817780805708676
Open

🧪 Add unit tests for path_to_string_lossy utility#55
bashandbone wants to merge 6 commits intomainfrom
testing-improvement-path-to-string-lossy-16782817780805708676

Conversation

@bashandbone
Copy link
Copy Markdown
Owner

🎯 What: The testing gap addressed for path_to_string_lossy utility function.
📊 Coverage: Added tests for both valid UTF-8 paths and invalid UTF-8 paths (verifying replacement character insertion).
Result: Increased test coverage for path utility functions in src/utilities.rs.


PR created automatically by Jules for task 16782817780805708676 started by @bashandbone

- Added `test_path_to_string_lossy_valid` to verify standard UTF-8 path conversion.
- Added `test_path_to_string_lossy_invalid` (Unix-only) to verify replacement character insertion for invalid UTF-8 paths.
- Verified test logic with a standalone script using `rustc --test`.

Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copilot AI review requested due to automatic review settings April 20, 2026 00:09
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 unit test coverage for the path_to_string_lossy utility in src/utilities.rs, validating behavior for both UTF-8-clean paths and (Unix-only) paths containing invalid bytes.

Changes:

  • Add a unit test verifying lossy conversion returns the same string for valid UTF-8 paths.
  • Add a Unix-only unit test verifying invalid path bytes are converted with the Unicode replacement character.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/utilities.rs Outdated
Comment thread src/utilities.rs
Comment thread src/utilities.rs
Comment on lines +448 to +451
fn test_path_to_string_lossy_valid() {
let path = std::path::Path::new("valid_utf8");
assert_eq!(path_to_string_lossy(path), "valid_utf8");
}
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path_to_string_lossy() currently always prints the "non-UTF-8" warning via eprintln! even when the input path is valid UTF-8. This new test will therefore emit a misleading warning (and adds noise when running tests with --nocapture). Consider changing path_to_string_lossy to only warn when the conversion is actually lossy (e.g., when to_string_lossy() returns an owned Cow, or when path.to_str() is None).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address and correct this issue you identified

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed the issue by modifying path_to_string_lossy to only print a warning when an actual lossy conversion occurs. I've also added unit tests for both valid and invalid UTF-8 paths to ensure the behavior is correct and doesn't regress.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 4250cd4. path_to_string_lossy now checks path.to_str() first and returns early for valid UTF-8 paths without any warning. The warning is only emitted when to_str() returns None (i.e., the path truly contains non-UTF-8 bytes and the conversion is lossy).

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/utilities.rs 76.92% <100.00%> (+2.87%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bashandbone and others added 2 commits April 25, 2026 23:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
google-labs-jules Bot and others added 3 commits April 26, 2026 03:15
…tring_lossy

- Modified `path_to_string_lossy` to only print a warning when non-UTF-8 characters are actually encountered.
- Added `test_path_to_string_lossy_valid` to verify standard UTF-8 path conversion without warnings.
- Added `test_path_to_string_lossy_invalid` (Unix-only) to verify replacement character insertion and warning for invalid UTF-8 paths.

Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants