Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions .github/workflows/unittest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,40 @@ permissions:
contents: read

jobs:
discover:
runs-on: ubuntu-latest
outputs:
matrix: ${{ steps.set-matrix.outputs.matrix }}
steps:
- name: Checkout
uses: actions/checkout@v4
with:
fetch-depth: 0
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.10"
- name: Get package shards
id: set-matrix
env:
BUILD_TYPE: presubmit
TARGET_BRANCH: ${{ github.base_ref || github.event.merge_group.base_ref }}
run: |
if [ -n "$TARGET_BRANCH" ]; then
git fetch origin "$TARGET_BRANCH" --deepen=200 || true
fi
echo "matrix=$(python3 ci/get_package_shards.py)" >> $GITHUB_OUTPUT

unit:
needs: discover
if: needs.discover.outputs.matrix != '[]' && needs.discover.outputs.matrix != ''
runs-on: ubuntu-22.04
strategy:
fail-fast: false
matrix:
python: ['3.9', '3.10', "3.11", "3.12", "3.13", "3.14"]
package_shard: ${{ fromJson(needs.discover.outputs.matrix) }}
name: unit (${{ matrix.package_shard.name }}, ${{ matrix.python }})
steps:
- name: Checkout
uses: actions/checkout@v4
Expand All @@ -36,18 +65,31 @@ jobs:
python -m pip install nox
- name: Run unit tests
env:
COVERAGE_FILE: .coverage-${{ matrix.python }}
BUILD_TYPE: presubmit
TARGET_BRANCH: ${{ github.base_ref || github.event.merge_group.base_ref }}
TEST_TYPE: unit
PY_VERSION: ${{ matrix.python }}
PACKAGE_LIST: ${{ matrix.package_shard.packages }}
run: |
ci/run_conditional_tests.sh
- name: Upload coverage results
uses: actions/upload-artifact@v4
with:
name: coverage-artifact-${{ '{{' }} matrix.python {{ '}}' }}
path: .coverage-${{ matrix.python }}
name: coverage-artifact-${{ matrix.python }}-${{ matrix.package_shard.index }}
path: .coverage.${{ matrix.python }}.*

unit-complete:
needs: [discover, unit]
if: always()
runs-on: ubuntu-latest
steps:
- name: Check unit test results
run: |
if [[ "${{ needs.unit.result }}" != "success" && "${{ needs.unit.result }}" != "skipped" ]]; then
echo "Unit tests failed"
exit 1
fi
echo "All unit tests passed or were skipped"

cover:
runs-on: ubuntu-latest
Expand All @@ -67,20 +109,19 @@ jobs:
python-version: "3.10"
- name: Set number of files changes in packages directory
id: packages
run: echo "::set-output name=num_files_changed::$(git diff HEAD~1 -- packages | wc -l)"
run: echo "num_files_changed=$(git diff HEAD~1 -- packages | wc -l)" >> $GITHUB_OUTPUT
- name: Install coverage
if: steps.packages.num_files_changed > 0
if: steps.packages.outputs.num_files_changed > 0
run: |
python -m pip install --upgrade setuptools pip wheel
python -m pip install coverage
- name: Download coverage results
if: ${{ steps.date.packages.num_files_changed > 0 }}
if: steps.packages.outputs.num_files_changed > 0
uses: actions/download-artifact@v4
with:
path: .coverage-results/
- name: Report coverage results
if: ${{ steps.date.packages.num_files_changed > 0 }}
if: steps.packages.outputs.num_files_changed > 0
run: |
find .coverage-results -type f -name '*.zip' -exec unzip {} \;
coverage combine .coverage-results/**/.coverage*
coverage report --show-missing --fail-under=100
97 changes: 95 additions & 2 deletions .kokoro/system.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ run_package_test() {
local package_name=$1
local package_path="packages/${package_name}"

# Isolate gcloud config for parallel execution
export CLOUDSDK_CONFIG=$(mktemp -d)
Comment on lines +42 to +43

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since run_package_test runs with set -e enabled, any test failure or error will cause the subshell to exit immediately. This prevents the manual cleanup at the end of the function from running, leading to leaked temporary directories in /tmp.

Using an EXIT trap ensures that the temporary directory is reliably cleaned up when the subshell exits, regardless of whether the tests succeeded or failed.

Suggested change
# Isolate gcloud config for parallel execution
export CLOUDSDK_CONFIG=$(mktemp -d)
# Isolate gcloud config for parallel execution
export CLOUDSDK_CONFIG=$(mktemp -d)
trap 'rm -rf "${CLOUDSDK_CONFIG}"' EXIT


# Declare local overrides to prevent bleeding into the next loop iteration
local PROJECT_ID
local GOOGLE_APPLICATION_CREDENTIALS
Expand Down Expand Up @@ -87,12 +90,59 @@ run_package_test() {
set -e
popd > /dev/null

# Clean up isolated gcloud config
rm -rf "${CLOUDSDK_CONFIG}"
Comment on lines +93 to +94

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

With the EXIT trap handling the cleanup of CLOUDSDK_CONFIG automatically, this manual cleanup is redundant and can be safely removed.


return $res
}

# A file for running system tests
system_test_script="${PROJECT_ROOT}/.kokoro/system-single.sh"

# Parallel execution settings
MAX_PARALLEL=4
running_pids=()
declare -A pid_to_pkg
declare -A pid_to_log
declare -A pid_to_resfile

# Array to keep track of results for the final summary
results=()

handle_finished_job() {
local pid=$1
local pkg=${pid_to_pkg[$pid]}
local log=${pid_to_log[$pid]}
local resfile=${pid_to_resfile[$pid]}

# wait $pid might fail if it was already reaped by wait -n,
# so we ignore its exit code and use the resfile.
wait "$pid" 2>/dev/null || true

local res=1
if [ -f "$resfile" ]; then
res=$(cat "$resfile")
rm "$resfile"
fi

echo "------------------------------------------------------------"
echo "System tests for ${pkg} finished (Exit code: ${res})"
echo "------------------------------------------------------------"
if [ -f "$log" ]; then
cat "$log"
rm "$log"
else
echo "Log file missing for ${pkg}"
fi

if [ "${res}" -ne 0 ]; then
RETVAL=${res}
results+=("${pkg}: FAILED")
else
results+=("${pkg}: PASSED")
fi
}

# Run system tests for each package with directory packages/*/tests/system
for path in `find 'packages' \
\( -type d -wholename 'packages/*/tests/system' \) -o \
Expand Down Expand Up @@ -140,10 +190,53 @@ for path in `find 'packages' \
set -e

if [[ "${package_modified}" -gt 0 || "$KOKORO_BUILD_ARTIFACTS_SUBDIR" == *"continuous"* ]]; then
# Call the function - its internal exports won't affect the next loop
run_package_test "$package_name" || RETVAL=$?
# Wait if we have reached MAX_PARALLEL
while [[ ${#running_pids[@]} -ge $MAX_PARALLEL ]]; do
set +e
wait -n
set -e
# Find which job finished
new_pids=()
for pid in "${running_pids[@]}"; do
if kill -0 "$pid" 2>/dev/null; then
new_pids+=("$pid")
else
handle_finished_job "$pid"
fi
done
running_pids=("${new_pids[@]}")
done

# Start the next test in the background
log_file=$(mktemp)
res_file=$(mktemp)
(
run_package_test "$package_name" > "$log_file" 2>&1
echo $? > "$res_file"
) &
pid=$!
running_pids+=($pid)
pid_to_pkg[$pid]=$package_name
pid_to_log[$pid]=$log_file
pid_to_resfile[$pid]=$res_file
echo "Started system tests for ${package_name} (PID: ${pid})"
else
echo "No changes in ${package_name} and not a continuous build, skipping."
fi
done

# Wait for all remaining jobs
for pid in "${running_pids[@]}"; do
handle_finished_job "$pid"
done

echo "------------------------------------------------------------"
echo "System Test Summary"
echo "------------------------------------------------------------"
for res in "${results[@]}"; do
echo "$res"
done
echo "------------------------------------------------------------"

exit ${RETVAL}

92 changes: 92 additions & 0 deletions ci/get_package_shards.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import os
import subprocess
import json
import math
import sys

def get_packages():
subdirs = ['packages']
packages = []
for subdir in subdirs:
if not os.path.exists(subdir):
continue
# Use the same sorting as the shell script
pkg_dirs = [os.path.join(subdir, d) + '/' for d in os.listdir(subdir) if os.path.isdir(os.path.join(subdir, d))]
packages.extend(sorted(pkg_dirs))
return packages

def get_packages_to_test():
build_type = os.environ.get('BUILD_TYPE', 'presubmit')
target_branch = os.environ.get('TARGET_BRANCH', 'main')

all_packages = get_packages()

if build_type == 'presubmit':
git_diff_arg = f"origin/{target_branch}..."
elif build_type == 'continuous':
git_diff_arg = "HEAD~.."
else:
return all_packages

# Check if ci/ changed
try:
subprocess.check_call(['git', 'diff', '--quiet', git_diff_arg, 'ci'])
ci_changed = False
except subprocess.CalledProcessError:
ci_changed = True

if ci_changed:
return all_packages

try:
res = subprocess.check_output(['git', 'diff', '--name-only', git_diff_arg]).decode('utf-8')
changed_files = res.splitlines()
except subprocess.CalledProcessError:
return all_packages

to_test = []
for pkg in all_packages:
# Check if any changed file starts with the package path
if any(f.startswith(pkg) for f in changed_files):
to_test.append(pkg)

return to_test

def group_packages(packages, max_packages_per_shard=50, max_total_shards=10):
if not packages:
return []

num_packages = len(packages)

# Calculate number of shards based on packages per shard
num_shards = math.ceil(num_packages / max_packages_per_shard)

# Cap the total number of shards
num_shards = min(num_shards, max_total_shards)

# Recalculate shard size to be as even as possible given the capped shards
shard_size = math.ceil(num_packages / num_shards)

shards = []
for i in range(num_shards):
start = i * shard_size
end = min((i + 1) * shard_size, num_packages)
if start >= num_packages:
break

shard_packages = packages[start:end]
index = i + 1
name = f"Shard {index}"

shards.append({
"name": name,
"index": index,
"packages": " ".join(shard_packages)
})
return shards

if __name__ == "__main__":
packages = get_packages_to_test()
# Shard into groups of ~50 libraries, up to 10 parallel jobs
shards = group_packages(packages, max_packages_per_shard=50, max_total_shards=10)
print(json.dumps(shards))
41 changes: 40 additions & 1 deletion ci/run_conditional_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,42 @@ subdirs=(
packages
)

if [ -n "${PACKAGE_LIST}" ]; then
echo "Using provided PACKAGE_LIST"
to_test=(${PACKAGE_LIST})
RETVAL=0
for d in ${to_test[@]}; do
echo "running test in ${d}"
pushd ${d}
set +e
${test_script}
ret=$?
set -e
Comment on lines +92 to +95

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Toggling set -e globally can lead to unexpected side effects or mask errors in other parts of the script if not carefully managed. A more idiomatic and safer way to capture the exit status of a command under set -e is to use the || operator, which prevents the shell from exiting on failure without needing to disable set -e globally.

Suggested change
set +e
${test_script}
ret=$?
set -e
ret=0
${test_script} || ret=$?

if [ ${ret} -ne 0 ]; then
RETVAL=${ret}
fi
popd
done
exit ${RETVAL}
fi

# Sharding logic (fallback for manual runs)
TOTAL_SHARDS="${TOTAL_SHARDS:-1}"
SHARD_INDEX="${SHARD_INDEX:-1}"
count=0

RETVAL=0

for subdir in ${subdirs[@]}; do
for d in `ls -d ${subdir}/*/`; do
# Sort the directories to ensure consistent sharding across jobs
for d in `ls -d ${subdir}/*/ | sort`; do
# Sharding logic: only process directories that belong to this shard
if (( count % TOTAL_SHARDS != SHARD_INDEX - 1 )); then
((count++))
continue
fi
((count++))

should_test=false
if [ -n "${GIT_DIFF_ARG}" ]; then
echo "checking changes with 'git diff --quiet ${GIT_DIFF_ARG} ${d}'"
Expand All @@ -108,6 +140,13 @@ for subdir in ${subdirs[@]}; do
pushd ${d}
# Temporarily allow failure.
set +e

# Ensure unique coverage file per package to avoid DataError
# when combining statement and branch coverage.
# Strip trailing slash from directory name for the filename.
pkg_name_clean=$(echo ${d} | sed 's|/$||' | sed 's|/|_|g')
export COVERAGE_FILE="${PROJECT_ROOT}/.coverage.${PY_VERSION}.${pkg_name_clean}"

${test_script}
ret=$?
set -e
Expand Down
1 change: 1 addition & 0 deletions packages/django-google-spanner/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-appoptimize/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-artifact-registry/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-asset/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-assured-workloads/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-audit-log/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-auditmanager/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-automl/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-backupdr/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-bare-metal-solution/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
1 change: 1 addition & 0 deletions packages/google-cloud-batch/SHARD_TEST.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test change for sharding
Loading
Loading