Transition testing framework from custom assert-based tests to Catch2#22
Transition testing framework from custom assert-based tests to Catch2#22
Conversation
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Migrates the test suite from custom asserts to Catch2 and updates build and docs to make Catch2 the primary testing path while keeping legacy tests available.
- Integrates Catch2 header and adds a consolidated Catch2 test file
- Updates Makefile with a new catch2_tests target and test alias; updates run_tests.sh to run Catch2 and legacy tests
- Refreshes README with Catch2 usage and commands
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| test/test_microbiome.cpp | New Catch2-based test suite covering AppConfig, Microorganism, MicroorganismFactory, Microbiome, and Simulation |
| Makefile | Adds catch2_tests target, test alias to Catch2, and clean rule; preserves legacy tests |
| run_tests.sh | Builds and runs Catch2 tests, then runs legacy tests; cleans old binaries before running |
| README.md | Documents Catch2 as primary framework, commands, and features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (config.getMaxTicks() > 0) { | ||
| REQUIRE(simulation.getTicksElapsed() >= config.getMaxTicks()); | ||
| } |
There was a problem hiding this comment.
This assertion is inverted for a freshly constructed Simulation: ticksElapsed should be 0 before run() is called, not >= maxTicks. Replace with REQUIRE(simulation.getTicksElapsed() == 0); If the goal is only to ensure it hasn't exceeded the max, use REQUIRE(simulation.getTicksElapsed() <= config.getMaxTicks()).
| if (config.getMaxTicks() > 0) { | |
| REQUIRE(simulation.getTicksElapsed() >= config.getMaxTicks()); | |
| } | |
| REQUIRE(simulation.getTicksElapsed() == 0); |
| microbiome.addMicroorganism(microorganism); | ||
|
|
||
| // Verify presence | ||
| REQUIRE(microbiome.isMicroorganismPresent(microorganism.getId()) == true); |
There was a problem hiding this comment.
[nitpick] Prefer idiomatic Catch2 assertions over comparing to true/false. For example: 29: REQUIRE(config.isSimulationOutputEnabled()); 115: REQUIRE(microbiome.isMicroorganismPresent(microorganism.getId())); 121: REQUIRE_FALSE(microbiome.isMicroorganismPresent(microorganism.getId())); 131: REQUIRE_FALSE(microorganismFound);
| # Remove old executables | ||
| rm -f ./tests ./mb_tests | ||
|
|
||
| # compile | ||
| make tests | ||
| # Compile Catch2 tests (default) |
There was a problem hiding this comment.
[nitpick] Add a shebang and strict mode to make the script fail fast and be more robust. Suggested header: #!/usr/bin/env bash followed by set -euo pipefail.
| echo "Running legacy tests for comparison..." | ||
| make tests | ||
| ./tests |
There was a problem hiding this comment.
The comment says 'Optionally' but the script always runs legacy tests. Either update the comment or gate these lines behind a flag, e.g.: if [ "${RUN_LEGACY:-0}" = "1" ]; then ... fi.
| echo "Running legacy tests for comparison..." | |
| make tests | |
| ./tests | |
| if [ "${RUN_LEGACY:-0}" = "1" ]; then | |
| echo "Running legacy tests for comparison..." | |
| make tests | |
| ./tests | |
| fi |
| @@ -10,5 +11,16 @@ all: mbapp tests | |||
| mbapp: src/mbapp.cpp | |||
| g++ $(WARNING_FLAGS) $(MAIN_FILE) $(PROJECT_FILES) $(ENVLIBCPP_FILES) -o mb_app | |||
There was a problem hiding this comment.
[nitpick] Specify the C++ standard explicitly to ensure consistent builds across environments (Catch2 v2 requires C++11+). For example, add -std=c++17 to these compile commands, or define CXXFLAGS = -std=c++17 $(WARNING_FLAGS) and reuse it.
| # Legacy tests (for backward compatibility) | ||
| tests: src/tests.cpp | ||
| g++ $(WARNING_FLAGS) $(PROJECT_FILES) $(ENVLIBCPP_FILES) $(TESTS_FILE) -o tests No newline at end of file | ||
| g++ $(WARNING_FLAGS) $(PROJECT_FILES) $(ENVLIBCPP_FILES) $(TESTS_FILE) -o tests |
There was a problem hiding this comment.
[nitpick] Specify the C++ standard explicitly to ensure consistent builds across environments (Catch2 v2 requires C++11+). For example, add -std=c++17 to these compile commands, or define CXXFLAGS = -std=c++17 $(WARNING_FLAGS) and reuse it.
|
|
||
| # Catch2 tests (new) | ||
| catch2_tests: test/test_microbiome.cpp | ||
| g++ $(WARNING_FLAGS) -I test $(PROJECT_FILES) $(ENVLIBCPP_FILES) $(CATCH2_TEST_FILE) -o mb_tests |
There was a problem hiding this comment.
[nitpick] Specify the C++ standard explicitly to ensure consistent builds across environments (Catch2 v2 requires C++11+). For example, add -std=c++17 to these compile commands, or define CXXFLAGS = -std=c++17 $(WARNING_FLAGS) and reuse it.
This PR implements the transition from the existing custom assert-based testing framework to Catch2, a modern C++ testing framework, as requested in issue #18.
What's Changed
The project now uses Catch2 v2.13.10 as the primary testing framework while maintaining full backward compatibility with the existing test suite. All original test functionality has been preserved and enhanced with better organization and reporting.
New Testing Structure
test/lib/catch2.hpp)test/test_microbiome.cppwith all tests converted to Catch2 format[appconfig]- Configuration validation tests[microorganism]- Microorganism behavior tests[factory]- MicroorganismFactory tests[microbiome]- Microbiome management tests[simulation]- Simulation execution testsEnhanced Build System
The Makefile has been updated with new targets while preserving existing functionality:
Improved Developer Experience
Catch2 provides significant improvements over the previous assert-based approach:
Better Error Reporting:
Selective Test Execution:
Enhanced Output Options:
Backward Compatibility
make testsrun_tests.shupdated to run both test systems for comparisonTest Coverage
All original tests have been successfully migrated:
Result: 10 test cases with 26 assertions - all passing ✅
Documentation Updates
.gitignoreto exclude new build artifactsThis transition provides a solid foundation for future test development with industry-standard tooling while ensuring zero disruption to existing development workflows.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.