adding tests and script - #5
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive unit testing infrastructure to the Trigger shell project. It introduces a custom lightweight testing framework and test suites for all three main modules (input, execute, and builtins), along with build system integration and documentation updates.
Changes:
- Added custom test framework with colored output and assertion macros
- Created unit tests for input parsing, command execution, and built-in commands
- Added test build configuration to CMakeLists.txt and a convenience script for running tests
- Updated README with project structure, building/testing instructions, and status updates
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_framework.h | Custom lightweight testing framework with macros for assertions and test suite management |
| tests/test_input.c | Unit tests for the input module, covering tokenization and edge cases |
| tests/test_execute.c | Unit tests for command execution, including built-ins and external commands |
| tests/test_builtins.c | Unit tests for all built-in commands (cd, help, exit) |
| run_tests.sh | Bash script to build the project and run all tests with ctest |
| CMakeLists.txt | Added test executable definitions and CTest integration |
| README.md | Updated with project structure diagram, build/test instructions, and improved status section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ASSERT_TRUE((expected) == (actual), message) | ||
|
|
||
| #define ASSERT_STR_EQUAL(expected, actual, message) \ | ||
| ASSERT_TRUE(strcmp((expected), (actual)) == 0, message) |
There was a problem hiding this comment.
The ASSERT_STR_EQUAL macro calls strcmp directly without checking if either argument is NULL. If either 'expected' or 'actual' is NULL, this will result in undefined behavior (likely a segmentation fault). Consider adding NULL checks or documenting that NULL values should be tested with ASSERT_NULL instead. While current usage is safe, this could be a source of bugs as the test suite grows.
| ASSERT_TRUE(strcmp((expected), (actual)) == 0, message) | |
| ASSERT_TRUE((((expected) == NULL && (actual) == NULL) || \ | |
| ((expected) != NULL && (actual) != NULL && strcmp((expected), (actual)) == 0)), \ | |
| message) |
| │ ├── test_input.c # Input module tests | ||
| │ ├── test_execute.c # Execute module tests | ||
| │ └── test_builtins.c # Built-ins module tests | ||
| └── CMakeLists.txt # Build configuration |
There was a problem hiding this comment.
The project structure diagram is missing run_tests.sh, which is a new file added in this PR and mentioned in the Testing section. Consider adding it to the structure diagram for completeness.
| └── CMakeLists.txt # Build configuration | |
| ├── CMakeLists.txt # Build configuration | |
| └── run_tests.sh # Test runner script |
| getcwd(cwd_before, sizeof(cwd_before)); | ||
|
|
||
| char *args[] = {"cd", "/tmp", NULL}; | ||
| int result = trigger_cd(args); | ||
|
|
||
| getcwd(cwd_after, sizeof(cwd_after)); |
There was a problem hiding this comment.
The getcwd call on line 49 could return NULL if the current working directory path exceeds the buffer size or if there's an error. In such cases, cwd_after would contain an indeterminate value, and the subsequent ASSERT_STR_EQUAL on line 52 would have undefined behavior. Consider checking if getcwd returns NULL and handling that case appropriately, or using a larger buffer size.
| getcwd(cwd_before, sizeof(cwd_before)); | |
| char *args[] = {"cd", "/tmp", NULL}; | |
| int result = trigger_cd(args); | |
| getcwd(cwd_after, sizeof(cwd_after)); | |
| if (getcwd(cwd_before, sizeof(cwd_before)) == NULL) { | |
| ASSERT_TRUE(0, "getcwd failed to get initial current directory"); | |
| return; | |
| } | |
| char *args[] = {"cd", "/tmp", NULL}; | |
| int result = trigger_cd(args); | |
| if (getcwd(cwd_after, sizeof(cwd_after)) == NULL) { | |
| ASSERT_TRUE(0, "getcwd failed to get current directory after cd"); | |
| return; | |
| } |
| getcwd(cwd_before, sizeof(cwd_before)); | ||
|
|
||
| char *args[] = {"cd", "/tmp", NULL}; | ||
| int result = trigger_cd(args); | ||
|
|
||
| getcwd(cwd_after, sizeof(cwd_after)); |
There was a problem hiding this comment.
The getcwd call on line 44 could return NULL if the current working directory path exceeds the buffer size or if there's an error. In such cases, cwd_before would contain an indeterminate value, which would cause undefined behavior when used with chdir on line 55. Consider checking if getcwd returns NULL and handling that case appropriately, or using a larger buffer size.
| getcwd(cwd_before, sizeof(cwd_before)); | |
| char *args[] = {"cd", "/tmp", NULL}; | |
| int result = trigger_cd(args); | |
| getcwd(cwd_after, sizeof(cwd_after)); | |
| if (getcwd(cwd_before, sizeof(cwd_before)) == NULL) { | |
| ASSERT_TRUE(0, "Failed to get current working directory before cd"); | |
| return; | |
| } | |
| char *args[] = {"cd", "/tmp", NULL}; | |
| int result = trigger_cd(args); | |
| if (getcwd(cwd_after, sizeof(cwd_after)) == NULL) { | |
| ASSERT_TRUE(0, "Failed to get current working directory after cd"); | |
| // Attempt to restore original directory even if checking after cd fails | |
| chdir(cwd_before); | |
| return; | |
| } |
|
|
||
| ## Overview | ||
| This project aims to create a clean, efficient shell. The current README was written quickly, but the code itself will be written without AI assistance. I plan to build the shell based on solid fundamentals and personal expertise. | ||
| This project aims to create a clean, efficient shell. The current README was written quickly, but the code itself will be written without AI assistance - exception being repetitive tasks like folder organization and testing. I plan to build the shell based on solid fundamentals and personal expertise. |
There was a problem hiding this comment.
Minor grammatical issue: "exception being" should be "with the exception of" or "the exception being" for better clarity and grammatical correctness.
| This project aims to create a clean, efficient shell. The current README was written quickly, but the code itself will be written without AI assistance - exception being repetitive tasks like folder organization and testing. I plan to build the shell based on solid fundamentals and personal expertise. | |
| This project aims to create a clean, efficient shell. The current README was written quickly, but the code itself will be written without AI assistance—with the exception of repetitive tasks like folder organization and testing. I plan to build the shell based on solid fundamentals and personal expertise. |
| The project includes comprehensive unit tests for all modules. To run the tests: | ||
|
|
||
| ```bash | ||
| # Quick way |
There was a problem hiding this comment.
The instructions for running the test script don't mention that the script may need to be made executable first with 'chmod +x run_tests.sh'. Consider adding this note to help users who encounter permission errors.
| # Quick way | |
| # Quick way | |
| # If you get a permission error, make the script executable first: | |
| # chmod +x run_tests.sh |
No description provided.