-
Notifications
You must be signed in to change notification settings - Fork 173
[FEA] Add selective test execution for PR builds #931
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
base: release/26.06
Are you sure you want to change the base?
Changes from 1 commit
b93bd54
09489bf
60af7c6
1e33a30
675eac2
a39469c
616ab1b
88eb4cc
620ec72
11d3454
808ff45
b59a15b
47bc15e
c0eeb44
cf79254
52a11a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| #!/bin/bash | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # Helper script to detect which test components should run based on changed files. | ||
| # Source this script to set the CUOPT_TEST_COMPONENTS variable. | ||
| # | ||
| # Usage: source ci/detect_test_components.sh | ||
| # | ||
| # Sets CUOPT_TEST_COMPONENTS to a comma-separated list of components (routing,lp,mip) | ||
| # or "all" if broad/shared changes are detected or if not in a pull-request build. | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| detect_test_components() { | ||
| # If CUOPT_TEST_COMPONENTS is already set externally, respect it | ||
| if [[ -n "${CUOPT_TEST_COMPONENTS:-}" ]]; then | ||
| export CUOPT_TEST_COMPONENTS | ||
| return | ||
| fi | ||
|
|
||
| # Only apply selective testing for pull-request builds | ||
| if [[ "${RAPIDS_BUILD_TYPE:-}" != "pull-request" ]]; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
|
|
||
| # In RAPIDS CI pull-request builds, the branch is a merge commit so | ||
| # HEAD~1..HEAD gives the full PR diff. If the parent isn't reachable | ||
| # (e.g. shallow clone or non-merge workflow), fall back to running all. | ||
| local changed_files | ||
| if ! git rev-parse --verify HEAD~1 &>/dev/null; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
| if ! changed_files=$(git diff --name-only HEAD~1..HEAD 2>/dev/null); then | ||
| # If git diff fails, run all tests to be safe | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's look at the full script context to understand the code better
cd /tmp && git ls-files | head -20Repository: NVIDIA/cuopt Length of output: 124 🏁 Script executed: # Find and examine the script
find . -name "detect_test_components.sh" -type f 2>/dev/nullRepository: NVIDIA/cuopt Length of output: 86 🏁 Script executed: # Check GitHub Actions workflows
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -20Repository: NVIDIA/cuopt Length of output: 505 🏁 Script executed: # Read the full detect_test_components.sh script
cat -n ci/detect_test_components.shRepository: NVIDIA/cuopt Length of output: 6237 🏁 Script executed: # Check the PR workflow configuration
cat -n .github/workflows/pr.yamlRepository: NVIDIA/cuopt Length of output: 20632 🏁 Script executed: # Also check test.yaml
cat -n .github/workflows/test.yamlRepository: NVIDIA/cuopt Length of output: 4785 🏁 Script executed: # Look for any documentation about how RAPIDS CI handles PR checkouts
find . -name "*.md" -o -name "*.txt" | xargs grep -l "RAPIDS_BUILD_TYPE\|pull-request" 2>/dev/null | head -5Repository: NVIDIA/cuopt Length of output: 73 🏁 Script executed: # Check if there are any comments/docs in the script about the merge commit assumption
grep -n "merge\|parent\|HEAD~1" ci/detect_test_components.shRepository: NVIDIA/cuopt Length of output: 427 🏁 Script executed: # Search for any references to the merge commit handling in other CI scripts
grep -r "HEAD~1\|merge.*commit" ci/ --include="*.sh" 2>/dev/null | head -10Repository: NVIDIA/cuopt Length of output: 452 Add a merge-commit check before using The current check at line 32 only verifies that Proposed fix if ! git rev-parse --verify HEAD~1 &>/dev/null; then
export CUOPT_TEST_COMPONENTS="all"
return
fi
+ # Require merge-commit context for HEAD~1..HEAD PR diff assumption.
+ # `git rev-list --parents -n 1 HEAD` returns: <sha> <parent1> [parent2...]
+ if [[ $(git rev-list --parents -n 1 HEAD | awk '{print NF}') -lt 3 ]]; then
+ export CUOPT_TEST_COMPONENTS="all"
+ return
+ fi
if ! changed_files=$(git diff --name-only HEAD~1..HEAD 2>/dev/null); then🤖 Prompt for AI Agents |
||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
|
|
||
| if [[ -z "${changed_files}" ]]; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
|
|
||
| local components="" | ||
| local run_all=false | ||
|
|
||
| # Check for shared/infrastructure changes that should trigger all tests | ||
| while IFS= read -r file; do | ||
| case "${file}" in | ||
| cpp/include/*|cpp/cmake/*|cpp/CMakeLists.txt|cpp/src/utilities/*) | ||
| run_all=true | ||
| break | ||
| ;; | ||
| # Changes to test infrastructure | ||
| cpp/tests/CMakeLists.txt|cpp/tests/utilities/*) | ||
| run_all=true | ||
| break | ||
| ;; | ||
| # Changes to CI infrastructure | ||
| ci/test_cpp.sh|ci/test_python.sh|ci/test_wheel_cuopt.sh|ci/run_ctests.sh|ci/run_cuopt_pytests.sh|ci/detect_test_components.sh) | ||
| run_all=true | ||
| break | ||
| ;; | ||
| # Changes to conda/build config | ||
| conda/*|dependencies.yaml) | ||
| run_all=true | ||
| break | ||
| ;; | ||
| esac | ||
| done <<< "${changed_files}" | ||
|
|
||
| if ${run_all}; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
|
|
||
| # Detect individual components | ||
| local has_routing=false | ||
| local has_lp=false | ||
| local has_mip=false | ||
|
|
||
| while IFS= read -r file; do | ||
| case "${file}" in | ||
| # Routing component | ||
| cpp/src/routing/*|cpp/src/distance/*|cpp/tests/routing/*|cpp/tests/distance_engine/*|cpp/tests/examples/routing/*) | ||
| has_routing=true | ||
| ;; | ||
| python/cuopt/cuopt/routing/*|python/cuopt/cuopt/tests/routing/*) | ||
| has_routing=true | ||
| ;; | ||
| regression/routing*) | ||
| has_routing=true | ||
| ;; | ||
| # LP component | ||
| cpp/src/dual_simplex/*|cpp/src/barrier/*|cpp/src/pdlp/*|cpp/src/math_optimization/*) | ||
| has_lp=true | ||
| ;; | ||
| cpp/tests/linear_programming/*|cpp/tests/dual_simplex/*|cpp/tests/qp/*) | ||
| has_lp=true | ||
| ;; | ||
| python/cuopt/cuopt/tests/linear_programming/*|python/cuopt/cuopt/tests/quadratic_programming/*) | ||
| has_lp=true | ||
| ;; | ||
| # LP regression | ||
| regression/lp*) | ||
| has_lp=true | ||
| ;; | ||
| # MIP component | ||
| cpp/src/branch_and_bound/*|cpp/src/cuts/*|cpp/src/mip_heuristics/*) | ||
| has_mip=true | ||
| ;; | ||
| cpp/tests/mip/*) | ||
| has_mip=true | ||
| ;; | ||
| regression/mip*) | ||
| has_mip=true | ||
| ;; | ||
| # Python source changes that could affect any component | ||
| python/cuopt/cuopt/*.py|python/cuopt/cuopt/distance_engine/*|python/cuopt/cuopt/utils/*) | ||
| has_routing=true | ||
| has_lp=true | ||
| ;; | ||
| python/libcuopt/*) | ||
| has_routing=true | ||
| has_lp=true | ||
| has_mip=true | ||
| ;; | ||
| esac | ||
| done <<< "${changed_files}" | ||
|
|
||
| # Build the components string | ||
| if ${has_routing}; then | ||
| components="${components:+${components},}routing" | ||
| fi | ||
| if ${has_lp}; then | ||
| components="${components:+${components},}lp" | ||
| fi | ||
| if ${has_mip}; then | ||
| components="${components:+${components},}mip" | ||
| fi | ||
|
|
||
| # If no specific component was detected, default to all | ||
| if [[ -z "${components}" ]]; then | ||
| components="all" | ||
| fi | ||
|
|
||
| export CUOPT_TEST_COMPONENTS="${components}" | ||
| } | ||
|
|
||
| detect_test_components | ||
| echo "CUOPT_TEST_COMPONENTS=${CUOPT_TEST_COMPONENTS}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,44 @@ | |
|
|
||
| set -euo pipefail | ||
|
|
||
| # Determine which test binary belongs to which component | ||
| should_run_test() { | ||
| local test_name="$1" | ||
| local components="${CUOPT_TEST_COMPONENTS:-all}" | ||
|
|
||
| # If all components should run, return true | ||
| if [[ "${components}" == "all" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| # Map test binary names to components | ||
| case "${test_name}" in | ||
| ROUTING_*|WAYPOINT_*|VEHICLE_*|OBJECTIVE_*|RETAIL_*) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a suggestion, please explore if you can come across better approach. I am trying to see how to make it easier to manage, if we can have these tests installed per module, like routing, lp, milp, it would be easier to just add those directory instead of adding each new test to this list. We may have to update cpp/test/CmakeLists.txt to keep the lower folders intact so we can use them
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rgsl888prabhu Great suggestion! I think we can update ConfigureTest() to install tests into per-module subdirectories (like bin/gtests/libcuopt/routing/, lp/, mip/ run_ctests.sh Since it touches the CMake build system and all the test CMakeLists, I'd prefer to keep it as a separate follow-up PR to avoid mixing it with the CI changes here. I'll open one once this is merged. Does that sound good?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered cmake test labels? I'm pretty concerned about the maintainability of having hardcoded test names in this file
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, harcoded test cases might create missing tests. I was thinking to tackle this in a follow-up PR, but since we have crossed 26.04 release, may be we can push updates as well in the same PR so there are no breakages in between.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mlubin Added label based gtests
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mlubin May I get your review on this ? |
||
| [[ "${components}" == *"routing"* ]] && return 0 | ||
| ;; | ||
| LP_*|PDLP_*|C_API_*|DUAL_SIMPLEX_*|QP_*) | ||
| [[ "${components}" == *"lp"* ]] && return 0 | ||
| ;; | ||
| MIP_*|PROBLEM_*|ELIM_*|STANDARDIZATION_*|MULTI_PROBE_*|INCUMBENT_*|DOC_EXAMPLE_*|CUTS_*|EMPTY_*|DETERMINISM_*) | ||
|
rgsl888prabhu marked this conversation as resolved.
Outdated
|
||
| [[ "${components}" == *"mip"* ]] && return 0 | ||
| ;; | ||
| # UNIT_TEST and PRESOLVE_TEST can belong to LP or MIP | ||
| UNIT_TEST|PRESOLVE_TEST) | ||
| [[ "${components}" == *"mip"* || "${components}" == *"lp"* ]] && return 0 | ||
| ;; | ||
| # CLI_TEST is a general utility test, run if any component is selected | ||
| CLI_TEST) | ||
| return 0 | ||
| ;; | ||
| # Unknown tests: run them to be safe | ||
| *) | ||
| return 0 | ||
| ;; | ||
| esac | ||
|
|
||
| return 1 | ||
| } | ||
|
|
||
| # Support customizing the gtests' install location | ||
| # First, try the installed location (CI/conda environments) | ||
| installed_test_location="${INSTALL_PREFIX:-${CONDA_PREFIX:-/usr}}/bin/gtests/libcuopt/" | ||
|
|
@@ -23,6 +61,11 @@ fi | |
|
|
||
| for gt in "${GTEST_DIR}"/*_TEST; do | ||
| test_name=$(basename "${gt}") | ||
| echo "Running gtest ${test_name}" | ||
| "${gt}" "$@" | ||
| if should_run_test "${test_name}"; then | ||
| echo "Running gtest ${test_name}" | ||
| "${gt}" "$@" | ||
| else | ||
| echo "Skipping gtest ${test_name} (not in CUOPT_TEST_COMPONENTS=${CUOPT_TEST_COMPONENTS:-all})" | ||
| fi | ||
| done | ||
|
|
||
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.
This is not true.
copy-pr-botcopies commits from the PR branch to a branch on the repo calledpull-request/{pr-number}and there HEAD is exactly HEAD of the PR branch.It isn't like in other open-source projects on GitHub where HEAD is a merge commit created by GitHub that merges the PR branch into the target branch.
That is why in the RAPIDS
changed-filesaction, we do some work to figure out what the target branch of the PR was:https://github.com/rapidsai/shared-actions/blob/f722a09c99e883ccca580bbb7135945d8efb90e0/changed-files/action.yml#L40-L45
I think this check
HEAD~1..HEADis only going to compare between the difference between the latest and previous-to-that commit on the PR branch, which is not what you want.You may want to look into something like
step-security/changed-files, which the existing RAPIDSchanged-filesaction uses:It produces an output listing all the files that changed: https://github.com/step-security/changed-files?tab=readme-ov-file#outputs-