fix: add file open error handling - #22
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis change adds error handling to file opening in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
58b0642 to
0fc4026
Compare
f5be4e9 to
6662c3a
Compare
0fc4026 to
a0139d2
Compare
a691eb5 to
f8177ba
Compare
f8177ba to
8eca760
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_caller.cpp (1)
22-33: Consider verifying temporary file creation.The test doesn't check if the temporary file was successfully created before running the program. If
/tmpis not writable (unlikely but possible), the test could give misleading results.🔎 Suggested enhancement
TEST(FileHandlingTest, ValidFileWorks) { // Create a temporary test file std::ofstream testfile("/tmp/test_callerpp.fa"); + ASSERT_TRUE(testfile.is_open()) << "Failed to create temporary test file"; testfile << ">test\nACGT\nACGT\n"; testfile.close();Optional improvements:
- Use
std::tmpfile()or a unique filename (e.g., with process ID) to avoid potential race conditions if tests run concurrently.- Consider using a cross-platform temporary directory approach for better portability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/caller.cpptests/test_caller.cpp
🔇 Additional comments (2)
src/caller.cpp (1)
328-332: LGTM! Error handling is correct.The implementation properly detects file open failures, provides a descriptive error message, cleans up allocated resources (
free(opt)), and returns an appropriate non-zero exit code.tests/test_caller.cpp (1)
2-3: LGTM! Necessary headers added.The
<cstdlib>header is required forstd::system()and the<fstream>header forstd::ofstream.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_caller.cpp (1)
25-39: Consider using unique temp filenames and ensuring cleanup on assertion failures.The test creates a temporary file at a hardcoded path
/tmp/test_callerpp.fa. This has two potential issues:
- Race condition: If tests run in parallel, multiple instances could conflict over the same filename.
- Incomplete cleanup: The
std::remove()at line 38 only executes if all assertions pass. If anyASSERT_*fails, the test aborts and the temporary file remains in/tmp.🔎 Suggested improvements
Option 1: Use a unique filename incorporating process ID
TEST(FileHandlingTest, ValidFileWorks) { // Create a temporary test file - std::ofstream testfile("/tmp/test_callerpp.fa"); + std::string temp_path = "/tmp/test_callerpp_" + std::to_string(getpid()) + ".fa"; + std::ofstream testfile(temp_path); ASSERT_TRUE(testfile.is_open()) << "Failed to create temporary test file"; testfile << ">test\nACGT\nACGT\n"; testfile.close(); - int result = std::system("./bin/callerpp -i /tmp/test_callerpp.fa >/dev/null 2>&1"); + std::string cmd = "./bin/callerpp -i " + temp_path + " >/dev/null 2>&1"; + int result = std::system(cmd.c_str()); ASSERT_NE(result, -1) << "std::system() failed to execute"; ASSERT_TRUE(WIFEXITED(result)) << "Process did not exit normally"; EXPECT_EQ(WEXITSTATUS(result), 0); // Cleanup - std::remove("/tmp/test_callerpp.fa"); + std::remove(temp_path.c_str()); }Option 2: Use RAII for automatic cleanup (requires
<unistd.h>forgetpid())Create a helper class or use a scope guard pattern to ensure cleanup happens even if assertions fail. For example:
#include <unistd.h> TEST(FileHandlingTest, ValidFileWorks) { std::string temp_path = "/tmp/test_callerpp_" + std::to_string(getpid()) + ".fa"; // Cleanup guard struct FileGuard { std::string path; ~FileGuard() { std::remove(path.c_str()); } } guard{temp_path}; std::ofstream testfile(temp_path); ASSERT_TRUE(testfile.is_open()) << "Failed to create temporary test file"; testfile << ">test\nACGT\nACGT\n"; testfile.close(); std::string cmd = "./bin/callerpp -i " + temp_path + " >/dev/null 2>&1"; int result = std::system(cmd.c_str()); ASSERT_NE(result, -1) << "std::system() failed to execute"; ASSERT_TRUE(WIFEXITED(result)) << "Process did not exit normally"; EXPECT_EQ(WEXITSTATUS(result), 0); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_caller.cpp
🔇 Additional comments (2)
tests/test_caller.cpp (2)
2-4: LGTM!The added includes are appropriate for the new test functionality:
<cstdlib>forstd::system(),<fstream>for file I/O, and<sys/wait.h>for POSIX exit status macros.
17-23: Excellent fix for system() return value handling!This test now correctly handles
std::system()return values on POSIX systems by checking for execution failure (-1), verifying normal process exit (WIFEXITED), and extracting the actual exit code (WEXITSTATUS). This directly addresses the previous review comment and ensures robust test behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_caller.cpp (1)
18-24: Excellent fix forstd::system()handling; consider making the test path unique.The test now properly uses
WIFEXITEDandWEXITSTATUS, which addresses the previous portability concern. The error-checking logic is correct.As an optional improvement, consider making the non-existent path more unique to avoid any theoretical collision:
🔎 Optional: use a unique non-existent path
- int result = std::system("./bin/callerpp -i /nonexistent/path/file.fa 2>/dev/null"); + std::string nonexistent = "/tmp/nonexistent_" + std::to_string(getpid()) + "_" + std::to_string(time(nullptr)) + ".fa"; + std::string cmd = "./bin/callerpp -i " + nonexistent + " 2>/dev/null"; + int result = std::system(cmd.c_str());Note: This requires
#include <ctime>fortime().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_caller.cpp
🔇 Additional comments (2)
tests/test_caller.cpp (2)
2-5: LGTM! Necessary includes for the new tests.All added headers are properly utilized:
<cstdlib>forstd::system,<fstream>for file I/O,<sys/wait.h>for POSIX wait macros, and<unistd.h>forgetpid().
26-47: Excellent test design with robust cleanup and race-condition prevention!This test demonstrates several best practices:
- Unique filename with
getpid()prevents race conditions in concurrent test runs.- RAII
FileGuardensures cleanup even if assertions fail.- Properly verifies file creation before proceeding.
- Closes the file before invoking the program under test.
- Correct use of
WIFEXITEDandWEXITSTATUSfor portable exit-code checking.The test effectively validates the successful execution path.
39f4525 to
9af06b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_caller.cpp (1)
21-21: Consider extracting unique filename generation to a helper function.Both tests generate unique filenames independently. While the current duplication is minimal, a helper function could improve consistency and maintainability if more file-based tests are added in the future.
🔎 Example helper function
// Helper to generate unique temp filenames std::string make_unique_temp_path(const std::string& prefix) { return "/tmp/" + prefix + "_" + std::to_string(getpid()) + "_" + std::to_string(time(nullptr)) + ".fa"; }Also applies to: 31-31
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_caller.cpp
🔇 Additional comments (3)
tests/test_caller.cpp (3)
2-6: LGTM!The includes are all necessary for the test functionality and properly support the POSIX-compliant return value handling.
20-27: Excellent POSIX return value handling!The test properly addresses the past review feedback by checking for
std::system()failure (-1), verifying normal exit withWIFEXITED(), and extracting the exit code withWEXITSTATUS().
34-37: Well-structured RAII cleanup pattern!The
FileGuardensures the temporary file is removed even if assertions fail, preventing test pollution.
- Add error handling for file open failures in caller.cpp - Add Google Test framework and test infrastructure - Add tests for file handling (non-existent and valid files) - Use unique temp filenames and RAII cleanup in tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
9af06b4 to
b292711
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_caller.cpp (1)
1-7: Document POSIX-specific test implementation.The tests use POSIX-specific headers (
<sys/wait.h>,<unistd.h>) and rely on/tmpdirectory conventions. While this is appropriate for the Ubuntu CI environment, consider adding a comment noting these tests are POSIX-specific and won't compile on Windows.🔎 Suggested documentation addition
Add a comment at the top of the file:
#include <gtest/gtest.h> #include <cstdlib> #include <ctime> #include <fstream> +// POSIX-specific headers for process control and file system operations +// These tests are designed for POSIX-compliant systems (Linux, macOS) #include <sys/wait.h> #include <unistd.h>Also applies to: 34-36
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/check.ymlMakefilesrc/caller.cpptests/test_caller.cpptests/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/caller.cpp
🔇 Additional comments (8)
tests/test_main.cpp (1)
1-6: LGTM! Standard GoogleTest entry point.This is the standard boilerplate for a GoogleTest main function and is correctly implemented.
.github/workflows/check.yml (2)
21-26: LGTM! Correct dependency installation.The libgtest-dev package installation is necessary for building the GoogleTest-based tests and is correctly placed before the build step.
45-55: LGTM! Good separation of test concerns.Splitting unit tests (
make test) from CLI integration tests provides better clarity in CI output and makes it easier to identify which type of test failed.Makefile (2)
4-4: LGTM! Test infrastructure correctly configured.The test configuration properly:
- Defines test directories and object files
- Links against GoogleTest libraries (
-lgtest -lpthread)- Provides compilation rules for test sources
- Creates a test target that builds and runs the test executable
The tests are pure CLI integration tests (no main program objects linked), which is appropriate for this PR's scope.
Also applies to: 12-16, 25-27, 33-35, 41-43
6-6: C++14 upgrade is required for GoogleTest 1.14.0 compatibility.The CI environment on Ubuntu 24.04 provides GoogleTest 1.14.0, which requires C++14. The upgrade from C++11 to C++14 is necessary and cannot be reverted to C++11.
Likely an incorrect or invalid review comment.
tests/test_caller.cpp (3)
8-17: LGTM! Placeholder tests verify framework.These simple tests confirm the GoogleTest infrastructure is working correctly before adding more complex tests.
20-27: LGTM! Robust error handling test.The test correctly:
- Generates a unique nonexistent path to avoid conflicts
- Uses
WIFEXITEDandWEXITSTATUSfor proper exit status checking- Redirects stderr to keep test output clean
- Verifies the program returns a non-zero exit code on file open failure
29-50: The minimal FASTA input is sufficient for callerpp. The code requires only a valid header and at least one sequence (lines 345–349 in src/caller.cpp return an error only if the sequence vector is empty), with no minimum length or sequence count restrictions beyond that. The test's 2 sequences of 4bp each satisfy all requirements, and the default alignment parameters work correctly for generating a consensus. The test is appropriately scoped for verifying file opening and basic program execution.
1 participant
Summary
-ioptionBackground
From code review (
research.md§1.1, MEDIUM severity):Previously the program would silently process empty input on file errors because there was no
is_open()check after opening the file.Changes
src/caller.cpp: Addin.is_open()check after opening file, return error code 1 with message if failedtests/test_caller.cpp: Add tests for file handling (non-existent file, valid file)Test plan
./bin/callerpp -i /nonexistent/filereturns error with messageCloses #21
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.