ci: add Google Test framework and test infrastructure - #20
Conversation
|
Warning Rate limit exceeded@nh13 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 29 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 (4)
WalkthroughAdds Google Test-based unit testing: new tests and test runner, Makefile test targets and flags, and CI steps to install libgtest, run the test suite, and exercise the Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
18-18: Declare "all" as a PHONY target.Static analysis correctly identifies that the
alltarget (line 28) should be declared PHONY to ensure it always runs even if a file named "all" exists.As per static analysis hints, the following improvement is recommended:
🔎 Proposed fix
-.PHONY: directories clean test +.PHONY: all directories clean test
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/check.ymlMakefiletests/test_caller.cpptests/test_main.cpp
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 18-18: Missing required phony target "all"
(minphony)
[warning] 28-28: Target "all" should be declared PHONY.
(phonydeclared)
🪛 GitHub Actions: Compilation Check
tests/test_main.cpp
[error] 1-1: C++ versions less than C++14 are not supported. The build uses -std=c++11; upgrade to a C++14 or newer standard (e.g., -std=c++14 or -std=c++17).
[error] 1-1: GTest requires C++14 features (e.g., std::make_unique); build is failing due to C++11 standard.
🔇 Additional comments (6)
.github/workflows/check.yml (1)
45-55: LGTM! Test execution steps are well-structured.The separation of unit tests and CLI tests provides clear test organization. The CLI tests cover version output and input processing scenarios appropriately.
tests/test_caller.cpp (1)
1-12: LGTM! Placeholder tests are appropriate for infrastructure verification.The tests correctly verify the Google Test framework is operational. The comments clearly indicate these are placeholders to be expanded in future work.
Makefile (3)
12-15: LGTM! Test configuration variables are well-defined.The test configuration properly separates test objects, program name, and required libraries. The inclusion of
-lpthreadis necessary for Google Test.
25-34: LGTM! Test build rules are correctly structured.The compilation rules properly handle test sources from the
TEST_DIR, and the test program linking correctly includes Google Test libraries.
41-42: LGTM! Test target correctly builds and executes tests.The test target properly depends on directories and the test binary, then executes the tests.
tests/test_main.cpp (1)
3-6: Update Makefile to C++14: Google Test requires C++14 or newer.Google Test requires at least C++14, with the 1.16.x branch being the last to support it. The current Makefile at line 6 uses C++11, which prevents test compilation on systems with modern Google Test versions.
Update the C++ standard:
-CXX_FLAGS= -std=c++11 +CXX_FLAGS= -std=c++14Likely an incorrect or invalid review comment.
| - name: Install dependencies | ||
| shell: bash -l {0} | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y libgtest-dev |
There was a problem hiding this comment.
Google Test compilation required after libgtest-dev installation.
The libgtest-dev package only installs source files and requires manual compilation to create the necessary library files. The directory structure and build procedure vary by Ubuntu version:
- For Ubuntu 20.04 and earlier:
cd /usr/src/gtest - For Ubuntu 22.04+:
cd /usr/src/googletest/googletest
Use a separate build directory (recommended) or run cmake in the source directory, then copy the built libraries. Verify the correct directory exists before running cmake, as the path varies across Ubuntu versions.
🤖 Prompt for AI Agents
In .github/workflows/check.yml around lines 21 to 25, after installing
libgtest-dev you must build the gtest libraries from source because the package
only provides sources; detect which source path exists (/usr/src/gtest for older
Ubuntus or /usr/src/googletest/googletest for newer), create and cd to a
separate build directory, run cmake .. and make (or cmake --build .) to produce
libgtest.a/libgtest_main.a, then copy or install the resulting library files to
a system library path (e.g., /usr/lib) so the tests can link; ensure each step
checks that the chosen source directory exists before running cmake and fail
clearly if neither path is present.
58b0642 to
0fc4026
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/check.yml (1)
21-25: Verify that libgtest-dev provides pre-built libraries on Ubuntu 24.04.A previous review flagged that libgtest-dev historically only installs source files requiring manual compilation. Since this workflow runs on ubuntu-24.04, verify whether the package now includes pre-built libraries or if the build step from the previous comment is still required.
Does Ubuntu 24.04 libgtest-dev package include pre-built gtest libraries or only source files?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/check.ymlMakefiletests/test_caller.cpptests/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_main.cpp
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 18-18: Missing required phony target "all"
(minphony)
[warning] 28-28: Target "all" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (8)
.github/workflows/check.yml (2)
45-48: LGTM! Unit test execution step is correct.The step correctly runs the test target, which will build and execute the GoogleTest suite.
50-55: LGTM! CLI test coverage is comprehensive.The CLI tests appropriately verify version output (both
-vand--version) and exercise the tool with piped input, providing good coverage of basic functionality.tests/test_caller.cpp (1)
1-12: LGTM! Placeholder tests appropriately verify GoogleTest infrastructure.The tests correctly validate that the GoogleTest framework is integrated and functional. The simple assertions serve their intended purpose of confirming the test infrastructure works before adding substantive test cases.
Makefile (5)
6-6: LGTM! C++ standard updated to C++14.The change from C++11 to C++14 resolves the previous critical issue, as GoogleTest requires C++14 features.
12-15: LGTM! Test configuration is correct.The test configuration properly defines test objects, program name, and required libraries (
-lgtest -lpthread).
25-26: LGTM! Test object build rule is correct.The pattern rule properly compiles test sources from the
tests/directory into the build directory.
33-34: LGTM! Test program link rule is correct.The rule properly links test objects with GoogleTest libraries to produce the test executable.
41-42: LGTM! Test target correctly builds and executes tests.The test target properly depends on the test program and executes it, integrating smoothly with the CI workflow.
- Add tests/ directory with test_main.cpp and test_caller.cpp - Update Makefile with 'test' target to build and run tests - Install libgtest-dev in CI workflow - Add 'Run unit tests' step to CI pipeline Closes #19 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0fc4026 to
a0139d2
Compare
1 participant
Summary
tests/directory with Google Test infrastructuretesttarget to build and run testsBackground
From code review (
research.md): The project currently has no formal testing framework. This PR sets up the infrastructure needed to add tests for subsequent bug fixes.Changes
tests/test_main.cpp- Google Test entry pointtests/test_caller.cpp- Placeholder tests to verify infrastructureMakefile- Addtesttarget.github/workflows/check.yml- Install gtest and run testsTest plan
make testruns successfully with gtest installedCloses #19
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
CI / Build
✏️ Tip: You can customize this high-level summary in your review settings.