-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1057 trigger running foxx tests as part of ci #1059
1057 trigger running foxx tests as part of ci #1059
Conversation
…st cases if specified
Reviewer's Guide by SourceryThis PR implements the ability to run Foxx tests as part of the CI pipeline. The implementation adds a new CMake option ENABLE_FOXX_TESTS and modifies the Docker entrypoint script to conditionally run tests when this option is enabled. The changes also include updating the CI configuration to enable these tests. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JoshuaSBrown - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
-DBUILD_DOCS=False \ | ||
-DBUILD_PYTHON_CLIENT=False \ | ||
-DBUILD_FOXX=True \ | ||
-DINSTALL_FOXX=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider consolidating the duplicate CMake configuration to reduce maintenance burden
The CMake configuration is identical between both branches except for -DENABLE_FOXX_TESTS. Consider moving the common arguments before the if statement and only conditionally adding the test flag.
# Check to see if foxx has previously been installed
"${PROJECT_ROOT}/scripts/generate_datafed.sh"
CMAKE_ARGS="-DBUILD_REPO_SERVER=False -DBUILD_COMMON=False -DBUILD_AUTHZ=False -DBUILD_CORE_SERVER=False -DBUILD_WEB_SERVER=False -DBUILD_DOCS=False -DBUILD_PYTHON_CLIENT=False -DBUILD_FOXX=True -DINSTALL_FOXX=True"
if [ "$ENABLE_FOXX_TESTS" == "TRUE" ]; then
"${DATAFED_DEPENDENCIES_INSTALL_PATH}/bin/cmake" -S. -B build ${CMAKE_ARGS} -DENABLE_FOXX_TESTS=True
else
"${DATAFED_DEPENDENCIES_INSTALL_PATH}/bin/cmake" -S. -B build ${CMAKE_ARGS}
fi
docker/entrypoint_foxx.sh
Outdated
@@ -55,6 +70,13 @@ | |||
touch "$install_flag" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Move install flag creation after test execution to ensure installation is only marked complete on test success
Currently, the installation is marked as complete before running tests. If tests fail, the installation should not be considered successful. Consider moving the install flag creation after the test execution block.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
1053 enable foxx tests in ci pipeline
…tup-container-to-stdout Add log output from ci end-to-end arango-setup job
…-node-setup-job 1073 add missing deployment key to node setup job
grab log output even if container fails
move flag to the correct position after ps
6d9f332
into
1056-foxx-test-script-fix
Description
The CI pipeline even though enabling the foxx tests does not have an option to run them.
Summary by Sourcery
CI: