Add memory check tests #34
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR adds memory check tests via Valgrind, refactors the CMake build into modular scripts, and updates Docker/CI to enable and run memory checks.
- Removes legacy tutorial run scripts in favor of new modular tutorial examples
- Adds a Fortran example (
box_model.F90) and accompanying JSON data files for the tutorial - Splits the monolithic CMake script into several utility modules and integrates Valgrind tests
- Updates Dockerfiles and GitHub Actions to install Valgrind, pass
CAMP_ENABLE_MEMCHECK, and runctestunder Memcheck
Reviewed Changes
Copilot reviewed 136 out of 136 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| doc/camp_tutorial/boot_camp/part__code/.sh | Removed legacy run_part_* scripts from all tutorial parts |
| doc/camp_tutorial/boot_camp/part_2_code/box_model.F90 | Added a Fortran example program for tutorial memory checks |
| doc/camp_tutorial/boot_camp/part_1_code/*.json | Added mechanism and config JSON files for the tutorial |
| cmake/test_util.cmake | New utility for adding standard and Memcheck-enabled tests |
| cmake/silence_warnings.cmake | New function to suppress common compiler warnings |
| cmake/set_defaults.cmake | New defaults for C/CUDA/Fortran flags |
| cmake/dependencies.cmake | New file to find and configure external dependencies (SUNDIALS, SuiteSparse, etc.) |
| cmake/cmake_uninstall.cmake.in | New script for uninstall support |
| cmake/campConfig.cmake.in | New CMake package config template |
| Dockerfile*, .github/workflows/main.yml, .dockerignore | Installed Valgrind, enabled CAMP_ENABLE_MEMCHECK, updated CI to run Memcheck tests |
Comments suppressed due to low confidence (4)
.github/workflows/main.yml:13
- The workflow tags both debug and release images as 'camp-test', causing the debug image to be overwritten when building the release. Use distinct image names or tags (e.g., 'camp-test-debug' and 'camp-test-release') to avoid conflicts.
run: docker build -t camp-test . -f Dockerfile.debug
doc/camp_tutorial/boot_camp/part_2_code/run_part_2.sh:1
- Removing all
run_part_*scripts may break the step-by-step tutorial. Ensure you provide updated instructions or replacement scripts so users can still run each tutorial part.
#!/bin/bash
doc/camp_tutorial/boot_camp/part_1_code/my_simple_mechanism.json:48
- [nitpick] The key
"my photo label"is inconsistent with the rest of the schema. Consider using a consistent field name like"photo_label"or"label"for clarity.
"my photo label" : "NO2 photolysis"
doc/camp_tutorial/boot_camp/part_2_code/box_model.F90:82
- Deallocating a pointer variable that wasn’t explicitly allocated may lead to runtime errors. Verify that
camp_coreis allocatable or consider providing a proper finalization method instead of a directdeallocate.
deallocate( camp_core )
jtgasparik
reviewed
Jun 11, 2025
| #include "../test_common.h" | ||
| #include "../../src/Jacobian.h" | ||
| #include <camp/Jacobian.h> | ||
| #include "..//test_common.h" |
jtgasparik
approved these changes
Jun 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds memory check tests using valgrind to test suite. Also breaks up monolithic CMake build script and some minor memory leaks in MPI tests.