feat: add unit testing infrastructure #95
Conversation
|
Warning Rate limit exceeded@nh13 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds a lightweight unit testing infrastructure: a header-only test framework, a test runner, Makefile targets for building/running/cleaning tests, updated CI steps to run unit and integration tests separately, and .gitignore entries for test artifacts. (47 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![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 (2)
tests/test_framework.h (1)
59-109: Optional: Consider more unique macro variable names.The assertion macros use variable names like
_exp,_act,_nexpwhich, while legal in local scope, could be more defensive. Consider using double underscores or a prefix like__test_expto further reduce collision risk with user code.This is a minor concern since these are macro-local variables, but it's a best practice for macro hygiene in widely-used headers.
Makefile (1)
75-93: Consider consolidating or clarifying test target naming.Currently:
make test→ runs integration tests (testdata/test.sh)make test-unit→ runs unit testsSuggested improvements:
- Have
make testrun both integration and unit tests- Provide separate
make test-integrationandmake test-unittargets- Document the distinction clearly
This would provide a better user experience where
make testruns all tests, while still allowing selective execution of each test suite.🔎 Proposed approach
+test: test-integration test-unit + +test-integration: if [ -d tmp ]; then rm -r tmp; fi /bin/bash testdata/test.sh # Unit test target TEST_OBJS = tests/test_main.o TEST_PROG = tests/run_tests tests/test_main.o: tests/test_main.c tests/test_framework.h $(CC) -c $(CFLAGS) $(DFLAGS) -I. -Isrc tests/test_main.c -o $@ $(TEST_PROG): $(TEST_OBJS) $(CC) $(CFLAGS) -o $@ $(TEST_OBJS) -lm test-unit: $(TEST_PROG) ./$(TEST_PROG) clean-tests: rm -f tests/*.o $(TEST_PROG)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignoreMakefiletests/test_framework.htests/test_main.c
🔇 Additional comments (9)
.gitignore (1)
3-4: LGTM!The ignore patterns correctly cover test artifacts introduced by the new unit test infrastructure.
tests/test_framework.h (4)
1-26: LGTM!The header documentation clearly explains usage patterns, and the include guards and standard library dependencies are appropriate.
34-49: LGTM!The
TESTandRUN_TESTmacro design is clean and follows standard patterns. The failure tracking correctly detects whether a test passed by comparing failure counts before and after execution.
51-57: LGTM!The summary calculation correctly computes passed tests as
test_count - test_failures. This works because the assertion macros return immediately after the first failure, ensuringtest_failurescounts failed tests rather than failed assertions.
126-222: LGTM!The floating-point comparison correctly uses epsilon-based comparison, pointer assertions are straightforward, and the comparison macros (
ASSERT_LT,ASSERT_LE, etc.) appropriately cast tolong longfor consistent comparison. TheTEST_SUITEmacro provides clear test organization.tests/test_main.c (2)
15-58: LGTM!The framework verification tests provide good coverage of all assertion macros with passing test cases. This validates that the basic framework infrastructure works correctly.
60-80: LGTM!The main function correctly orchestrates test execution and returns an appropriate exit code (
1for failures,0for success), enabling integration with build systems and CI pipelines.Makefile (2)
36-36: LGTM!The
.PHONYdeclarations correctly mark the test targets as non-file targets.
79-88: LGTM!The test compilation and linking rules correctly:
- Include
-Isrcto allow tests to access source headers- Link with
-lmfor the math library (required byfabsinASSERT_FLOAT_EQ)- Specify proper dependencies on both the
.cfile and the headerThe omission of
-lzand-lpthreadcompared to the main programs is appropriate since the test framework doesn't need these libraries.
| /* Global test counters */ | ||
| static int test_count = 0; | ||
| static int test_failures = 0; | ||
| static int test_assertions = 0; | ||
| static const char *current_test = NULL; |
There was a problem hiding this comment.
Critical: Static globals in header limit scalability to single translation unit.
Defining static global variables in a header file means each .c file that includes this header gets its own independent copy of these counters. This design only works correctly if the header is included in exactly one .c file.
Impact:
- If future test files include this header separately (as proper
.cfiles), their test counts won't aggregate - The framework cannot track results across multiple test compilation units
Current status: This works now because only test_main.c includes the header, but it will break if the codebase scales to multiple test files compiled separately.
Recommended solutions:
- Move these definitions to
test_main.cand declare themexternin the header - Use a single-file pattern where all tests are included as
.cfiles (as hinted intest_main.ccomments), though this has other downsides
Consider documenting this limitation in the header comment to warn future contributors.
🤖 Prompt for AI Agents
In tests/test_framework.h around lines 28-32, the static global counters are
defined in the header which creates per-translation-unit copies; change the
header to declare these as extern (e.g. extern int test_count; extern int
test_failures; extern int test_assertions; extern const char *current_test;) and
move the actual definitions (without extern and without static) into test_main.c
(or another single implementation file) so there is one shared instance;
additionally update any include patterns or add a short header comment
documenting that the real definitions live in test_main.c.
| /* Assert string equality */ | ||
| #define ASSERT_STR_EQ(expected, actual) do { \ | ||
| test_assertions++; \ | ||
| const char *_exp = (expected); \ | ||
| const char *_act = (actual); \ | ||
| if (_exp == NULL || _act == NULL || strcmp(_exp, _act) != 0) { \ | ||
| printf(" [FAIL] %s\n", current_test); \ | ||
| printf(" Expected: \"%s\", Actual: \"%s\"\n", \ | ||
| _exp ? _exp : "(null)", _act ? _act : "(null)"); \ | ||
| printf(" at %s:%d\n", __FILE__, __LINE__); \ | ||
| test_failures++; \ | ||
| return; \ | ||
| } \ | ||
| } while (0) |
There was a problem hiding this comment.
Reconsider NULL handling in ASSERT_STR_EQ.
The current logic treats NULL == NULL as unequal, which may surprise users:
ASSERT_STR_EQ(NULL, NULL); // This would FAILThe condition if (_exp == NULL || _act == NULL || strcmp(...)) fails when either pointer is NULL, even if both are NULL.
Most test frameworks allow NULL == NULL to pass. Consider revising to:
if ((_exp == NULL) != (_act == NULL) || (_exp != NULL && strcmp(_exp, _act) != 0))This would pass when both are NULL, but fail when only one is NULL or when non-NULL strings differ.
🔎 Proposed fix
- if (_exp == NULL || _act == NULL || strcmp(_exp, _act) != 0) {
+ if ((_exp == NULL) != (_act == NULL) || (_exp != NULL && strcmp(_exp, _act) != 0)) {
printf(" [FAIL] %s\n", current_test);
printf(" Expected: \"%s\", Actual: \"%s\"\n", \
_exp ? _exp : "(null)", _act ? _act : "(null)");🤖 Prompt for AI Agents
In tests/test_framework.h around lines 111 to 124, the ASSERT_STR_EQ macro
treats NULL == NULL as a failure because it unconditionally fails when either
pointer is NULL; change the boolean condition to first detect a nullity mismatch
or, only when both are non-NULL, call strcmp. Specifically, replace the current
if-check with logic equivalent to: fail if exactly one of _exp/_act is NULL, or
if both non-NULL and strcmp reports inequality; keep the existing printing
behavior (using "(null)" when appropriate) and return on failure.
| /* Include test files here as they are added */ | ||
| /* #include "test_position.c" */ | ||
| /* #include "test_memory.c" */ | ||
| /* #include "test_parsing.c" */ |
There was a problem hiding this comment.
Reconsider the pattern of including .c files.
The commented examples suggest including .c files directly:
/* #include "test_position.c" */Issues with this approach:
- Violates standard C compilation model (separate compilation units)
- Can cause symbol conflicts and duplicate definitions
- Makes build dependencies unclear
- While this pattern sidesteps the static global issue in
test_framework.h(by keeping everything in one translation unit), it introduces other problems
Better alternatives:
- Compile test files separately and link them, fixing the static globals issue properly
- Use header files (
.h) for test declarations if tests must be included
Consider documenting the intended test file organization pattern in the header comment or README.
🤖 Prompt for AI Agents
In tests/test_main.c around lines 10 to 13, avoid including .c test files
directly (e.g., /* #include "test_position.c" */) because that forces single
translation-unit builds and causes duplicate symbols and unclear deps; instead,
compile each test_*.c separately and link them into the test binary (or move
shared declarations into headers and include .h only), fix the static/global
symbol management in test_framework.h so separate compilation works, and add a
brief header comment or README note explaining the intended build pattern and
how to add new tests.
nh13
left a comment
nh13
left a comment
There was a problem hiding this comment.
Please make sure nothing needs to be added to .github/workflows/testing.yml for CI to run the unit tests
|
Addressed the review feedback:
The test targets are now:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
96-97: Consider integrating test cleanup into main clean target.The
clean-teststarget correctly removes test artifacts, but it's not invoked by the maincleantarget (line 57). This meansmake cleanleaves test artifacts behind. Consider addingclean-testsas a dependency ofcleanorcleanlocalfor consistency.🔎 Proposed integration
Add
clean-teststo the main clean target:-clean:cleanlocal-recur +clean:cleanlocal-recur clean-tests
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/testing.ymlMakefile
🔇 Additional comments (8)
.github/workflows/testing.yml (1)
20-23: LGTM: Test separation improves CI feedback loop.Splitting unit and integration tests into separate steps provides faster feedback and clearer failure diagnostics.
Makefile (7)
36-36: LGTM: Proper PHONY declarations.All new test targets are correctly declared as PHONY, preventing conflicts with potential files of the same name.
75-76: LGTM: Unified test target.The consolidated
testtarget provides a convenient way to run both unit and integration tests, improving developer experience.
78-82: LGTM: Integration test target.The
test-integrationtarget correctly executes existing integration tests and handles temporary directory cleanup.
84-85: LGTM: Clear test artifact definitions.Test variable naming is clear and the structure is extensible for future test additions.
87-88: LGTM: Correct test compilation rule.The compilation rule properly handles dependencies, includes, and compiler flags.
93-94: LGTM: Test execution target.The
test-unittarget correctly builds and executes the test program.
90-91: The test program linking is correct. The test code currently only verifies the test framework itself with basic assertions and does not call any DWGSIM library functions that would require-lzor-lpthread. The-lmlibrary is sufficient for the current test suite.Likely an incorrect or invalid review comment.
| steps: | ||
| - name: Checkout sources | ||
| uses: actions/checkout@v2 | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Consider upgrading to v6 for the latest features and security updates.
While upgrading from v2 to v4 is an improvement, v6.0.1 is the latest version. Recommend updating to actions/checkout@v6 to align with current best practices.
🤖 Prompt for AI Agents
In .github/workflows/testing.yml around line 15 the workflow uses
actions/checkout@v4; update this to actions/checkout@v6 (or
actions/checkout@v6.0.1) to pick up the latest features and security fixes, then
run the workflow locally or in CI to verify there are no breaking changes and
adjust any checkout-related inputs if needed.
|
Addressed the additional review feedback:
|
Add a lightweight unit test framework for DWGSIM: - tests/test_framework.h: Minimal test macros (ASSERT, ASSERT_EQ, etc.) - tests/test_main.c: Test runner with framework verification tests - Makefile updates: - `make test` runs both unit and integration tests - `make test-unit` runs only unit tests - `make test-integration` runs only integration tests - `make clean-tests` cleans test artifacts - `clean` target now includes `clean-tests` - .gitignore: Ignore test build artifacts - CI workflow: Run unit tests explicitly in pipeline, upgrade actions/checkout to v4 Closes #94 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
nh13
force-pushed
the
feature/add-unit-test-framework
branch
from
2678931 to
4c897d2
Compare
Summary
tests/test_framework.hwith assertion macros (ASSERT, ASSERT_EQ, ASSERT_STR_EQ, etc.)tests/test_main.ctest runner with framework verification testsmake test-unitandmake clean-testsMakefile targetsCloses #94
Test plan
make test-unit- all tests should passmake test- integration tests should still passmake clean-tests- test artifacts should be cleaned🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.