From 86b6dfb9650becdd0d76883ca1d1d49e1b8faa11 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Mon, 31 Aug 2026 15:19:34 +0000 Subject: [PATCH] fix: avoid running split ITs unexpectedly on unrelated changes (#12029) This PR is part 1 of 2 in a stacked series: 1. This PR (#14217): fix: avoid running split ITs unexpectedly on unrelated changes (#12029) 2. #14218: perf(ci): replace mvn help:evaluate with native bash and sed extraction ## Problem In split integration and GraalVM tests (e.g. integration-single and graalvm-single), module modification detection previously checked if the module name was present as a substring in modified_module_list. For modules with shared prefixes (such as java-bigquery vs java-bigquerystorage, java-bigquery-jdbc, or java-bigqueryconnection), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Modular Helper Functions with Single Responsibility**: - `populate_modified_files`: Encapsulates diff retrieval, caching in `modified_files`, and configuring git `safe.directory` for Docker container ownership mismatches. - `should_test_all_modules`: Purely checks global overrides that require testing all modules (parent POMs, core shared dependencies, and `TEST_ALL_MODULES="true"`). - `is_module_modified`: Strictly checks if files within a specific module directory were modified (`^${module}/`), avoiding prefix collisions. - **Explicit Job Control in Split Jobs**: - In `.kokoro/build.sh` (`integration-single` and `graalvm-single`), runs tests if either monorepo-wide testing is required or the specific module was modified: `! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"` -> skip. - **Updated generate_modified_modules_list**: - Reuses `populate_modified_files` and `should_test_all_modules`. - **Unit Tests**: - Added unit tests in `.kokoro/common_test.sh` for `is_module_modified` and `should_test_all_modules`. Fixes #12029 --- .kokoro/build.sh | 10 ++-- .kokoro/common.sh | 86 ++++++++++++++++++++++------- .kokoro/common_test.sh | 120 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 22 deletions(-) diff --git a/.kokoro/build.sh b/.kokoro/build.sh index 5930c8e13381..5c396a83889c 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -119,10 +119,11 @@ case ${JOB_TYPE} in fi ;; integration-single) - generate_modified_modules_list false if [[ "$(release_please_snapshot_pull_request)" == "true" ]]; then echo "Not running integration checks -- this is Release Please SNAPSHOT pull request." - elif [[ ! " ${modified_module_list[*]} " =~ " ${BUILD_SUBDIR} " ]]; then + # Run tests if either global overrides require testing all modules (e.g. parent POM or + # shared dependencies) OR if this specific module was modified. Otherwise skip. + elif ! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"; then echo "${BUILD_SUBDIR} not modified, skipping split integration test" else echo "${BUILD_SUBDIR} modified, running split integration test" @@ -188,10 +189,11 @@ case ${JOB_TYPE} in fi ;; graalvm-single) - generate_modified_modules_list false if [[ "$(release_please_snapshot_pull_request)" == "true" ]]; then echo "Not running GraalVM checks -- this is Release Please SNAPSHOT pull request." - elif [[ ! " ${modified_module_list[*]} " =~ " ${BUILD_SUBDIR} " ]]; then + # Run tests if either global overrides require testing all modules (e.g. parent POM or + # shared dependencies) OR if this specific module was modified. Otherwise skip. + elif ! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"; then echo "${BUILD_SUBDIR} not modified, skipping split GraalVM test" else echo "${BUILD_SUBDIR} modified, running split GraalVM test" diff --git a/.kokoro/common.sh b/.kokoro/common.sh index 4e50b68bd5c0..ee6770a9d6e1 100644 --- a/.kokoro/common.sh +++ b/.kokoro/common.sh @@ -231,21 +231,56 @@ function release_please_snapshot_pull_request() { fi } -# Sets bash variables for maven_modules and modified_module_list -# maven_modules is the list of all maven submodules of the root pom -# modified_module_list is the subset of maven_modules that have been touched -# in the current pull request +# Returns the list of modified files in the current PR diff via git diff. +function get_modified_files() { + # In Kokoro Docker containers, the build runs as root (UID 0) while repository files + # belong to the host user (UID 1000). Git 2.35.2+ flags this UID mismatch as 'dubious ownership' + # and aborts git commands; safe.directory allows Git to operate in this directory. + git config --global --add safe.directory "$(realpath .)" 2>/dev/null || true + + # '${VAR:-DEFAULT}' uses $VAR if set and non-empty, otherwise falls back to DEFAULT. + # This allows developers to run these scripts locally outside the Kokoro CI environment. + local target_branch="${KOKORO_GITHUB_PULL_REQUEST_TARGET_BRANCH:-origin/main}" + local target_commit="${KOKORO_GITHUB_PULL_REQUEST_COMMIT:-HEAD}" + + # 'git diff A...B' (triple-dot) diffs between the merge-base (common ancestor) of + # target_branch and target_commit, listing only files changed in this branch. + git diff --name-only "${target_branch}...${target_commit}" +} + +# Determines if the entire monorepo must be tested. +# +# Monorepo-wide testing is triggered under three conditions: +# 1. TEST_ALL_MODULES is set to "true" (used by nightly and scheduled CI builds). +# 2. Root parent POMs (google-cloud-jar-parent or google-cloud-pom-parent) are modified, +# as changes to parent POMs affect shared dependency versions and compiler/build plugins. +# 3. Core shared dependencies (sdk-platform-java/java-shared-dependencies) are modified, +# as gax, auth, and transport changes can break downstream client library integration tests. +function should_test_all_modules() { + local files + files=$(get_modified_files) + + # '<<< STRING' is a Bash "here-string" that feeds the string variable directly to + # stdin of grep, avoiding an external subshell pipeline (like 'echo "$var" | grep'). + if [[ "${TEST_ALL_MODULES}" == "true" ]] || \ + grep -q -E '^google-cloud-(pom|jar)-parent/pom.xml$' <<< "${files}" || \ + grep -q -E '^sdk-platform-java/java-shared-dependencies/' <<< "${files}"; then + return 0 + fi + return 1 +} + +# Generates the list of modified Maven modules for batch integration/GraalVM test jobs. +# Sets global variables: +# - maven_modules: list of all Maven modules defined in the root POM. +# - modified_module_list: modules that need to be tested for the current PR. # -# The first positional argument is a value true/false. If true (default), then -# exclude modules from the global exclusion list. +# Positional parameter $1 (default "true") specifies whether to filter out modules +# defined in the 'excluded_modules' array. function generate_modified_modules_list() { - # Find the files changed from when the PR branched to the last commit - # Filter for java modules and get all the unique elements - # grep returns 1 (error code) and exits the pipeline if there is no match - # If there is no match, it will return true so the rest of the commands can run - git config --global --add safe.directory $(realpath .) - modified_files=$(git diff --name-only "${KOKORO_GITHUB_PULL_REQUEST_TARGET_BRANCH}...${KOKORO_GITHUB_PULL_REQUEST_COMMIT}") - printf "Modified files:\n%s\n" "${modified_files}" + local files + files=$(get_modified_files) + printf "Modified files:\n%s\n" "${files}" # Generate the list of valid maven modules maven_modules_list=$(mvn help:evaluate -Dexpression=project.modules | grep '<.*>.*' | sed -e 's/<.*>\(.*\)<\/.*>/\1/g') @@ -265,14 +300,12 @@ function generate_modified_modules_list() { fi modified_module_list=() - # If either parent pom.xml is touched, run ITs on all the modules - parent_pom_modified=$(echo "${modified_files}" | grep -E '^google-cloud-(pom|jar)-parent/pom.xml$' || true) - shared_dependencies_modified=$(echo "${modified_files}" | grep -E '^java-shared-dependencies' || true) - if [[ (-n $parent_pom_modified) || (-n $shared_dependencies_modified) || ("${TEST_ALL_MODULES}" == "true") ]]; then + # If either parent pom.xml or core shared dependency is touched, run ITs on all the modules + if should_test_all_modules; then modified_module_list=(${maven_modules[*]}) echo "Testing the entire monorepo" else - modules=$(echo "${modified_files}" | grep -E '(google-auth|java)-.*' || true) + modules=$(echo "${files}" | grep -E '(google-auth|java)-.*' || true) printf "Files in java modules:\n%s\n" "${modules}" if [[ -n $modules ]]; then modules=$(echo "${modules}" | cut -d '/' -f1 | sort -u) @@ -287,6 +320,23 @@ function generate_modified_modules_list() { fi } +# Checks if files within a specific module directory were modified in the PR diff. +# +# Uses exact directory prefix matching ('^${module}/') to prevent substring collisions +# where modifying one module triggers tests for another module that shares its prefix +# (e.g. java-bigquery vs java-bigquerystorage). +function is_module_modified() { + local module="$1" + if [[ -z "${module}" ]]; then + return 1 + fi + + local files + files=$(get_modified_files) + # '<<< "${files}"' feeds the diff string directly to grep via stdin. + grep -q -E "^${module}/" <<< "${files}" +} + # Filters the modified_module_list to only include modules that contain # integration test files (matching IT*.java or *IT.java in src/test/java). # Not all modules will have ITs written and there is not need to test diff --git a/.kokoro/common_test.sh b/.kokoro/common_test.sh index e78e4d36131b..247a6a26cbfc 100755 --- a/.kokoro/common_test.sh +++ b/.kokoro/common_test.sh @@ -19,6 +19,12 @@ source "$scriptDir/common.sh" mkdir -p target cd target +# Mock get_modified_files for unit testing to avoid running Git commands. +# Individual tests set TEST_MODIFIED_FILES to simulate changed files. +function get_modified_files() { + printf '%s\n' "${TEST_MODIFIED_FILES:-}" +} + function test_find_all_poms_with_versioned_dependency { mkdir -p test_find_all_poms_with_dependency pushd test_find_all_poms_with_dependency @@ -75,6 +81,120 @@ function test_parse_pom_version { popd } +# Tests that is_module_modified strictly matches the module directory prefix, +# preventing prefix collisions (e.g. java-bigquery vs java-bigquerystorage). +function test_is_module_modified { + if is_module_modified ""; then + echo "is_module_modified should return non-zero for empty module name" + exit 1 + fi + + # Touching 'java-bigquery' should not match 'java-bigquerystorage', 'java-bigquery-jdbc', etc. + TEST_MODIFIED_FILES="java-bigquery/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java +java-bigquery/pom.xml" + + if ! is_module_modified "java-bigquery"; then + echo "is_module_modified failed to detect java-bigquery modification" + exit 1 + fi + if is_module_modified "java-bigquerystorage"; then + echo "is_module_modified incorrectly matched java-bigquerystorage for java-bigquery" + exit 1 + fi + if is_module_modified "java-bigquery-jdbc"; then + echo "is_module_modified incorrectly matched java-bigquery-jdbc for java-bigquery" + exit 1 + fi + if is_module_modified "java-bigqueryconnection"; then + echo "is_module_modified incorrectly matched java-bigqueryconnection for java-bigquery" + exit 1 + fi + + # Touching 'java-bigquerystorage' should match only java-bigquerystorage + TEST_MODIFIED_FILES="java-bigquerystorage/google-cloud-bigquerystorage/src/main/java/Foo.java" + if is_module_modified "java-bigquery"; then + echo "is_module_modified incorrectly matched java-bigquery for java-bigquerystorage" + exit 1 + fi + if ! is_module_modified "java-bigquerystorage"; then + echo "is_module_modified failed to detect java-bigquerystorage modification" + exit 1 + fi + + # External / root files should not be considered module modifications + TEST_MODIFIED_FILES="google-cloud-jar-parent/pom.xml +sdk-platform-java/java-shared-dependencies/pom.xml" + if is_module_modified "java-bigquery"; then + echo "is_module_modified should return false for java-bigquery when only external files changed" + exit 1 + fi + + unset TEST_MODIFIED_FILES +} + +# Test should_test_all_modules triggers properly on global changes. +function test_should_test_all_modules { + # When only a normal library is modified, should return false (1) + TEST_MODIFIED_FILES="java-bigquery/pom.xml" + if should_test_all_modules; then + echo "should_test_all_modules should return false for single module change" + exit 1 + fi + + # Root jar parent pom + TEST_MODIFIED_FILES="google-cloud-jar-parent/pom.xml" + if ! should_test_all_modules; then + echo "should_test_all_modules should return true for google-cloud-jar-parent change" + exit 1 + fi + + # Root pom parent + TEST_MODIFIED_FILES="google-cloud-pom-parent/pom.xml" + if ! should_test_all_modules; then + echo "should_test_all_modules should return true for google-cloud-pom-parent change" + exit 1 + fi + + # Core shared dependencies + TEST_MODIFIED_FILES="sdk-platform-java/java-shared-dependencies/pom.xml" + if ! should_test_all_modules; then + echo "should_test_all_modules should return true for java-shared-dependencies change" + exit 1 + fi + + # Prefix collision check: sibling paths starting with java-shared-dependencies must not match + TEST_MODIFIED_FILES="sdk-platform-java/java-shared-dependencies-bom/pom.xml" + if should_test_all_modules; then + echo "should_test_all_modules should return false for java-shared-dependencies prefix match" + exit 1 + fi + + # TEST_ALL_MODULES=true + TEST_MODIFIED_FILES="" + if ! TEST_ALL_MODULES="true" should_test_all_modules; then + echo "should_test_all_modules should return true when TEST_ALL_MODULES is true" + exit 1 + fi + + unset TEST_MODIFIED_FILES +} + +# Test mock get_modified_files returns simulated files. +function test_mock_get_modified_files { + TEST_MODIFIED_FILES="dummy/file.txt" + local files + files=$(get_modified_files) + if [[ "${files}" != "dummy/file.txt" ]]; then + echo "mock get_modified_files failed to return expected files" + exit 1 + fi + unset TEST_MODIFIED_FILES +} + test_find_all_poms_with_versioned_dependency test_update_pom_dependency test_parse_pom_version +test_mock_get_modified_files +test_should_test_all_modules +test_is_module_modified +