Option to build tests #100
Conversation
WalkthroughThe changes in this pull request introduce two new matrix parameters, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Alan Jowett <[email protected]>
Alan-Jowett
force-pushed
the
option_to_build_tests
branch
from
036dd88 to
866964d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build.yml (2)
58-58: LGTM: Windows build job updated for conditional test buildsThe changes in the
build_windowsjob correctly mirror the modifications made in thebuild_ubuntujob:
- Addition of
build_teststo the matrix- Update of the CMake command to include the
VERIFIER_ENABLE_TESTSoption- Conditional execution of unit tests
These changes ensure consistent behavior across both Ubuntu and Windows builds, which is excellent.
Consider extracting the common matrix configuration (including
build_tests) into a reusable workflow or a top-levelstrategyto reduce duplication between the Ubuntu and Windows jobs. This would make future updates easier and ensure consistency.Also applies to: 73-73, 77-78
Line range hint
1-79: Overall assessment: Changes successfully implement conditional test buildsThe modifications to
.github/workflows/build.ymleffectively introduce conditional test builds for both Ubuntu and Windows environments. The changes are well-implemented, consistent across jobs, and align perfectly with the PR objectives.Key points:
- Introduction of
build_testsmatrix parameter- CMake configuration updated to respect the
build_testsvalue- Conditional execution of unit tests and memory leak checks
These changes will significantly enhance the flexibility of the CI process by allowing builds with and without tests.
To further improve the workflow, consider the following:
- Extract common matrix configurations into a reusable workflow or top-level strategy to reduce duplication.
- If not already done, update the project documentation to reflect this new build option and how it affects the CI process.
🧰 Tools
🪛 actionlint
39-39: shellcheck reported issue in this script: SC2046:warning:3:24: Quote this to prevent word splitting
(shellcheck)
CMakeLists.txt (1)
Line range hint
179-200: LGTM with a minor suggestion: Conditional compilation and linking of test executables.The conditional setting of compilation options and library linking for test executables based on
VERIFIER_ENABLE_TESTSis correct and aligns with the PR objectives. The maintenance of different compilation flags for various build types is good practice.Minor suggestion: Consider moving the
message("Boost_LIBRARY_DIRS: ${Boost_LIBRARY_DIRS}")line outside of the conditional block, as it might be useful for debugging even when tests are not enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (4 hunks)
- CMakeLists.txt (8 hunks)
🧰 Additional context used
🔇 Additional comments (11)
.github/workflows/build.yml (4)
21-21: LGTM: Addition ofbuild_teststo the matrixThe introduction of
build_tests: [true, false]to the matrix is a good approach to enable conditional test builds. This change aligns well with the PR objective and follows GitHub Actions best practices for matrix strategies.
41-41: LGTM: CMake configuration updated for conditional test buildsThe addition of
-DVERIFIER_ENABLE_TESTS=${{matrix.build_tests}}to the CMake command correctly integrates the new build option. This change ensures that the CMake configuration respects thebuild_testsmatrix value, enabling or disabling test builds as intended.
45-46: LGTM: Conditional execution of unit testsThe addition of
if: matrix.build_testscondition ensures that unit tests are only run whenbuild_testsis true. This is consistent with the PR objective and correctly implements conditional test execution.
49-50: LGTM: Conditional execution of memory leak testsThe addition of
if: matrix.build_testscondition ensures that memory leak tests are only run whenbuild_testsis true. This is consistent with the PR objective and correctly implements conditional test execution for memory leak checks.CMakeLists.txt (7)
8-9: LGTM: New option for conditional test builds.The addition of
VERIFIER_ENABLE_TESTSoption aligns well with the PR objective of introducing a conditional build configuration for tests. Setting the default to ON maintains backward compatibility.
18-25: LGTM: Conditional inclusion of Catch2.The conditional inclusion of Catch2 based on
VERIFIER_ENABLE_TESTSis a good practice. It ensures that the testing framework is only fetched and made available when tests are enabled, which can save time and resources during builds where tests are not needed.
Line range hint
98-107: LGTM: Conditional compilation of test source files.The conditional inclusion of test source files based on
VERIFIER_ENABLE_TESTSis appropriate. This ensures that test-related code is only compiled when tests are enabled, which can reduce compilation time and binary size when tests are not needed.
124-129: LGTM: Conditional addition of test executables.The conditional addition of test executables (
check,tests,run_yaml, andconformance_check) based onVERIFIER_ENABLE_TESTSis correct. This ensures that these executables are only built when tests are enabled, which is in line with the PR objectives.
Line range hint
133-151: LGTM: Conditional setting of target properties for test executables.The conditional setting of target properties for test executables based on
VERIFIER_ENABLE_TESTSis correct. This ensures that these properties are only set when tests are enabled. The addition of the dependency between 'tests' and 'conformance_check' is also appropriately conditioned.
159-161: LGTM: Conditional addition of bpf_conformance subdirectory.The conditional addition of the bpf_conformance subdirectory based on
VERIFIER_ENABLE_TESTSis appropriate. This ensures that this subdirectory is only included in the build when tests are enabled, which can simplify the build process when tests are not needed.
Line range hint
1-201: Overall assessment: Well-implemented conditional test builds.The changes in this
CMakeLists.txtfile successfully implement the PR objective of introducing a conditional build configuration for tests. The newVERIFIER_ENABLE_TESTSoption is consistently used throughout the file to conditionally include, compile, and link test-related components. This implementation allows for more flexible builds, potentially reducing build times and resource usage when tests are not needed.Key points:
- Conditional inclusion of the Catch2 testing framework
- Conditional compilation of test source files
- Conditional addition and configuration of test executables
- Conditional inclusion of the bpf_conformance subdirectory
These changes enhance the build system's flexibility without breaking existing functionality, as tests are enabled by default. Great job on implementing this feature!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CMakeLists.txt (2)
Line range hint
179-200: LGTM with a minor suggestion: Conditional compilation and linking of test executables.The conditional application of compilation options and library linking for test executables based on the
VERIFIER_ENABLE_TESTSoption is well-implemented. This ensures that test-related build configurations are only applied when tests are enabled, maintaining a clean build configuration and reducing unnecessary compilations when tests are not needed.Consider removing or conditionally including the
message("Boost_LIBRARY_DIRS: ${Boost_LIBRARY_DIRS}")line, as it may not be necessary when tests are disabled.
Line range hint
1-200: Overall assessment: Well-implemented conditional test building.The changes in this
CMakeLists.txtfile effectively implement the PR objective of introducing a conditional build configuration for tests. The newVERIFIER_ENABLE_TESTSoption is consistently used throughout the file to conditionally:
- Fetch the Catch2 testing framework
- Include test source files
- Add test executables
- Set test-related target properties and dependencies
- Include the bpf_conformance subdirectory
- Apply compilation options and link libraries for test executables
This implementation provides flexibility for users who don't need to build tests while maintaining backward compatibility by enabling tests by default. The changes are well-structured and follow CMake best practices for conditional compilation.
Consider adding documentation comments in the CMakeLists.txt file to explain the usage of the
VERIFIER_ENABLE_TESTSoption, especially for users who might want to disable test building.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (4 hunks)
- CMakeLists.txt (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🔇 Additional comments (6)
CMakeLists.txt (6)
8-9: LGTM: New option for conditional test building.The addition of the
VERIFIER_ENABLE_TESTSoption with a default value of ON aligns well with the PR objective. This change allows for conditional compilation of tests while maintaining backward compatibility by building tests by default.
18-25: LGTM: Conditional fetching of Catch2.The conditional fetching of Catch2 based on the
VERIFIER_ENABLE_TESTSoption is a good optimization. It ensures that the testing framework is only downloaded and made available when tests are enabled, reducing unnecessary dependencies when tests are not being built.
Line range hint
98-107: LGTM: Conditional compilation of test source files.The conditional inclusion of test source files based on the
VERIFIER_ENABLE_TESTSoption is a good practice. This ensures that test files are only compiled when tests are enabled, which can significantly reduce compilation time and resource usage when tests are not needed.
124-129: LGTM: Conditional addition of test executables.The conditional addition of test executables (
check,tests,run_yaml, andconformance_check) based on theVERIFIER_ENABLE_TESTSoption is appropriate. This ensures that these targets are only created when tests are enabled, simplifying the build process and reducing the number of targets when tests are not needed.
Line range hint
133-151: LGTM: Conditional configuration of test executable properties and dependencies.The conditional setting of target properties and dependencies for test executables based on the
VERIFIER_ENABLE_TESTSoption is well-implemented. This ensures that test-related configurations are only applied when tests are enabled, maintaining a clean build configuration when tests are not needed.
159-161: LGTM: Conditional inclusion of bpf_conformance subdirectory.The conditional addition of the bpf_conformance subdirectory based on the
VERIFIER_ENABLE_TESTSoption is appropriate. This ensures that this dependency is only included when tests are enabled, reducing unnecessary dependencies and simplifying the build process when tests are not needed.
Signed-off-by: Alan Jowett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
.github/workflows/build.yml (2)
41-45: LGTM: Conditional cmake command correctly implements test build option.The conditional
cmakecommand effectively implements theVERIFIER_ENABLE_TESTSoption based on thematrix.targetvalue. This aligns well with the PR objectives and the corresponding changes inCMakeLists.txt.For improved clarity, consider explicitly handling both cases:
if: ${{ matrix.target == 'library' }} cmake -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_CONFIGURATION}} -DVERIFIER_ENABLE_TESTS=OFF if: ${{ matrix.target == 'tests' }} cmake -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_CONFIGURATION}} -DVERIFIER_ENABLE_TESTS=ONThis makes the intention clearer and guards against potential future additions to the
targetmatrix.
77-81: LGTM: Consistent implementation of conditional builds and test execution for Windows.The changes to the
build_windowsjob correctly implement conditionalcmakecommands and test execution, mirroring the functionality in thebuild_ubuntujob. This ensures consistent behavior across different operating systems.For improved consistency with the
build_ubuntujob, consider using a similar structure for thecmakecommand:if ("${{matrix.target}}" -eq "library") { cmake -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_CONFIGURATION}} -DVERIFIER_ENABLE_TESTS=OFF } else { cmake -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_CONFIGURATION}} -DVERIFIER_ENABLE_TESTS=ON }This makes the Windows job more closely mirror the Ubuntu job, potentially making future maintenance easier.
Also applies to: 85-85
CMakeLists.txt (3)
Line range hint
133-151: LGTM with a suggestion: Conditional setting of target propertiesThe conditional setting of target properties and dependencies for test executables is consistent with the
VERIFIER_ENABLE_TESTSoption. This ensures that test-related configurations are only applied when tests are enabled.Consider using a variable for the common RUNTIME_OUTPUT_DIRECTORY value to improve maintainability. For example:
set(TEST_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/..") set_target_properties(check tests run_yaml conformance_check PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${TEST_RUNTIME_OUTPUT_DIRECTORY}")This would make it easier to change the output directory for all test executables in the future if needed.
Line range hint
179-200: LGTM with a suggestion: Conditional compilation and linking of test executablesThe conditional compilation and linking of test executables based on
VERIFIER_ENABLE_TESTSis consistent with the new option. This ensures that test-related build configurations are only applied when tests are enabled.Consider using a function or macro to set common compilation options for test executables to reduce code duplication. For example:
function(set_test_executable_options target) target_compile_options(${target} PRIVATE ${COMMON_FLAGS}) target_compile_options(${target} PUBLIC "$<$<CONFIG:DEBUG>:${DEBUG_FLAGS}>") target_compile_options(${target} PUBLIC "$<$<CONFIG:RELEASE>:${RELEASE_FLAGS}>") target_compile_options(${target} PUBLIC "$<$<CONFIG:SANITIZE>:${SANITIZE_FLAGS}>") endfunction() set_test_executable_options(check) set_test_executable_options(tests)This would make the CMakeLists.txt file more maintainable and reduce the risk of inconsistencies between test executables.
Line range hint
1-201: Overall assessment: Well-implemented test build optionThe introduction of the
VERIFIER_ENABLE_TESTSoption and its implementation throughout the CMakeLists.txt file is well-done. The changes consistently apply the option to all test-related aspects of the build process, including:
- Fetching of the Catch2 testing framework
- Inclusion of test source files
- Addition of test executables
- Setting of test target properties and dependencies
- Inclusion of the bpf_conformance subdirectory
- Compilation and linking of test executables
This new option provides greater flexibility in the build process, potentially saving time and resources when tests are not needed, while maintaining the default behavior for backwards compatibility.
Consider the following suggestions for further improvements:
- Add documentation comments for the
VERIFIER_ENABLE_TESTSoption to explain its purpose and default value.- Consider grouping all test-related configurations together in the CMakeLists.txt file for better organization and maintainability.
- Implement the suggestions provided in previous comments to reduce code duplication and improve maintainability.
These changes would further enhance the clarity and maintainability of the build configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (4 hunks)
- CMakeLists.txt (8 hunks)
🧰 Additional context used
🔇 Additional comments (9)
.github/workflows/build.yml (4)
21-21: LGTM: Addition oftargetparameter enhances build flexibility.The introduction of the
targetparameter with valuestestsandlibraryaligns well with the PR objective of enabling conditional test builds. This change allows for more granular control over the CI/CD process, potentially improving efficiency by allowing separate builds for tests and the library.
49-49: LGTM: Conditional test execution properly implemented.The addition of
if: ${{matrix.target == 'tests'}}to the "Run unit tests" and "Test for memory leaks" steps ensures that these steps are only executed when building tests. This is consistent with the PR objectives and provides the desired flexibility in the CI/CD workflow.Also applies to: 53-53
62-62: LGTM: Consistent addition oftargetparameter to Windows build job.The addition of the
targetparameter to thebuild_windowsjob matrix is consistent with the change made to thebuild_ubuntujob. This ensures that the same build flexibility is available for both Ubuntu and Windows environments.
Line range hint
21-85: Overall, the changes effectively implement conditional test builds and executions.The modifications to
.github/workflows/build.ymlsuccessfully introduce the ability to conditionally build and run tests based on the newtargetmatrix parameter. This implementation is consistent across both Ubuntu and Windows build jobs, with appropriate syntax adjustments for each environment. The changes align well with the PR objectives and provide the desired flexibility in the CI/CD workflow.Key points:
- Introduction of
target: [tests, library]in both job matrices.- Conditional
cmakecommands to setVERIFIER_ENABLE_TESTSbased on the target.- Conditional execution of test steps.
These changes should significantly enhance the flexibility of the build process, allowing for more efficient CI/CD pipelines by separating library and test builds when necessary.
🧰 Tools
🪛 actionlint
39-39: shellcheck reported issue in this script: SC2046:warning:7:24: Quote this to prevent word splitting
(shellcheck)
CMakeLists.txt (5)
8-9: LGTM: New option for enabling testsThe introduction of the
VERIFIER_ENABLE_TESTSoption is a good addition. It provides flexibility for users who may not want to build tests, while maintaining the existing behavior by default.
18-25: LGTM: Conditional fetching of Catch2The conditional fetching of Catch2 based on
VERIFIER_ENABLE_TESTSis a good optimization. It ensures that the testing framework is only downloaded when needed, which can save time and resources during builds where tests are disabled.
Line range hint
98-107: LGTM: Conditional inclusion of test filesThe conditional globbing of test files based on
VERIFIER_ENABLE_TESTSis a good practice. It ensures that test files are only included in the build when tests are enabled, keeping the build process clean and efficient when tests are not needed.
124-129: LGTM: Conditional addition of test executablesThe conditional addition of test executables (check, tests, run_yaml, and conformance_check) based on
VERIFIER_ENABLE_TESTSis consistent with the new option. This ensures that these executables are only built when tests are enabled, which is an efficient use of build resources.
159-161: LGTM: Conditional inclusion of bpf_conformanceThe conditional inclusion of the bpf_conformance subdirectory based on
VERIFIER_ENABLE_TESTSis consistent with the new option. This ensures that bpf_conformance is only built when tests are enabled, which is an efficient use of build resources.
This pull request introduces a conditional build configuration for tests in the CI workflow and updates the CMake build configuration to support this feature. The most important changes include adding a matrix configuration for building tests, conditionally running tests based on this configuration, and updating the CMakeLists.txt file to include or exclude test-related targets and dependencies based on a new option.
CI Workflow Enhancements:
.github/workflows/build.yml: Addedbuild_teststo the matrix configuration and updated the build and test steps to conditionally include tests based on this matrix value. [1] [2] [3] [4]CMake Configuration Updates:
CMakeLists.txt: IntroducedVERIFIER_ENABLE_TESTSoption to control whether tests are built, and wrapped test-related declarations and dependencies within conditional blocks based on this option. [1] [2] [3] [4] [5] [6] [7] [8]Summary by CodeRabbit
New Features
build_testsparameter.Bug Fixes