🧪 Add error path test for pdf_extract::extract_text#47
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
Adds a regression test to cover the filesystem error path of pdf_extract::extract_text when the target PDF file cannot be read (e.g., missing file).
Changes:
- Add
extract_text_returns_error_for_missing_fileunit test validating thatextract_textreturns an error for an unreadable/missing path. - Assert the returned error string includes the expected
"failed to read PDF:"prefix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let path = Path::new("/path/that/does/not/exist.pdf"); | ||
|
|
||
| // when | ||
| let result = extract_text(path); |
There was a problem hiding this comment.
This test hard-codes an absolute Unix-style path ("/path/that/does/not/exist.pdf"), which can be non-portable and potentially flaky (e.g., different path semantics on Windows, or the path unexpectedly existing in some environments). Prefer constructing a definitely-nonexistent path under std::env::temp_dir() (and ensure the file does not exist) so the failure mode is consistent across platforms.
| let path = Path::new("/path/that/does/not/exist.pdf"); | |
| // when | |
| let result = extract_text(path); | |
| let path = std::env::temp_dir().join(format!( | |
| "clawd-missing-{}-{}.pdf", | |
| std::process::id(), | |
| "extract-text-test" | |
| )); | |
| let _ = std::fs::remove_file(&path); | |
| // when | |
| let result = extract_text(&path); |
|
@claude code review |
|
@codex code review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@gemini-code-assist, what are the steps and line numbers to make this fix? |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
@copilot, make changes based on:
See: #47 (comment) |
🎯 What: The
extract_textfunction handles filesystem reads but lacked test coverage for when the file is missing or inaccessible.📊 Coverage: Adds the
extract_text_returns_error_for_missing_filetest to cover the error path natively.✨ Result: Enhanced test coverage providing better confidence against regressions when modifying file loading logic.
PR created automatically by Jules for task 2389079918942033131 started by @badMade