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
117 changes: 113 additions & 4 deletions .kokoro/system.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ run_package_test() {
# Inherit NOX_SESSION from environment to allow configs (like prerelease.cfg) to pass it in
local NOX_SESSION="${NOX_SESSION}"

# ISOLATION: Create a unique gcloud config dir for this run
local gcloud_config_dir=$(mktemp -d -t "gcloud-config-${package_name}-XXXXXX")
local CLOUDSDK_CONFIG="${gcloud_config_dir}"
Comment on lines +50 to +51

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.

high

Since set -e is active, if any command fails during the test execution, the function/subshell will exit immediately, and the manual cleanup at the end of the function will be bypassed, leaking the temporary directory.

Using a trap on EXIT ensures that the temporary directory is cleaned up regardless of how the subshell exits. We should use double quotes in the trap definition so that the local variable gcloud_config_dir is expanded immediately, as the local variable will be out of scope when the subshell actually exits.

Additionally, if package_name contains any slashes, mktemp will fail because the template cannot contain directory separators. Sanitizing the package name first prevents this issue.

Suggested change
local gcloud_config_dir=$(mktemp -d -t "gcloud-config-${package_name}-XXXXXX")
local CLOUDSDK_CONFIG="${gcloud_config_dir}"
local safe_package_name=$(echo "${package_name}" | tr '/' '_')
local gcloud_config_dir=$(mktemp -d -t "gcloud-config-${safe_package_name}-XXXXXX")
trap "rm -rf '${gcloud_config_dir}'" EXIT
local CLOUDSDK_CONFIG="${gcloud_config_dir}"


echo "------------------------------------------------------------"
echo "Configuring environment for: ${package_name}"
echo "------------------------------------------------------------"
Expand Down Expand Up @@ -80,7 +84,7 @@ run_package_test() {
esac

# Export variables for the duration of this function's sub-processes
export PROJECT_ID GOOGLE_APPLICATION_CREDENTIALS NOX_FILE NOX_SESSION
export PROJECT_ID GOOGLE_APPLICATION_CREDENTIALS NOX_FILE NOX_SESSION CLOUDSDK_CONFIG
export GOOGLE_CLOUD_PROJECT="${PROJECT_ID}"

gcloud auth activate-service-account --key-file="$GOOGLE_APPLICATION_CREDENTIALS"
Expand All @@ -94,12 +98,15 @@ run_package_test() {
set -e
popd > /dev/null

rm -rf "${gcloud_config_dir}"
return $res
Comment on lines +101 to 102

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 trap on EXIT configured at the start of the function, we can safely remove the manual rm -rf call here. We should also unregister the trap using trap - EXIT before returning to prevent it from running again if the shell continues.

Suggested change
rm -rf "${gcloud_config_dir}"
return $res
trap - EXIT
return $res

}

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

PACKAGES_TO_TEST=()

# 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 @@ -147,10 +154,112 @@ 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=$?
PACKAGES_TO_TEST+=("$package_name")
else
echo "No changes in ${package_name} and not a continuous build, skipping."
fi
done
exit ${RETVAL}

# Parallel Execution Logic
MAX_JOBS=${MAX_JOBS:-4}
active_jobs=0
declare -A job_pids
declare -A job_pkgs
failed_packages=()
passed_packages=()
Comment on lines +163 to +169

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

To avoid duplicating the complex process reaping logic, we can extract it into a helper function reap_finished_jobs. This improves maintainability and readability.

# Parallel Execution Logic
MAX_JOBS=${MAX_JOBS:-4}
active_jobs=0
declare -A job_pids
declare -A job_pkgs
failed_packages=()
passed_packages=()

reap_finished_jobs() {
  for pid in "${!job_pids[@]}"; do
    if ! kill -0 "$pid" 2>/dev/null; then
      wait "$pid" && status=0 || status=$?
      finished_pkg=${job_pkgs[$pid]}
      if [ "$status" -eq 0 ]; then
        echo "✔ [SUCCESS] ${finished_pkg}"
        passed_packages+=("$finished_pkg")
      else
        echo "✘ [FAILURE] ${finished_pkg} (Exit Code: ${status})"
        failed_packages+=("$finished_pkg")
      fi
      unset "job_pids[$pid]"
      unset "job_pkgs[$pid]"
      active_jobs=$((active_jobs - 1))
    fi
  done
}


# Temporary directory for clean log segregation
LOG_DIR=$(mktemp -d -t test-logs-XXXXXX)
# Clean up logs on exit
trap 'rm -rf "$LOG_DIR"' EXIT

if [ ${#PACKAGES_TO_TEST[@]} -eq 0 ]; then
echo "No packages to test."
exit 0
fi

echo "=================================================="
echo "Starting parallel test execution for ${#PACKAGES_TO_TEST[@]} packages"
echo "Concurrency limit: ${MAX_JOBS}"
echo "=================================================="

for pkg in "${PACKAGES_TO_TEST[@]}"; do
# Maintain concurrency limit
while [ "$active_jobs" -ge "$MAX_JOBS" ]; do
for pid in "${!job_pids[@]}"; do
if ! kill -0 "$pid" 2>/dev/null; then
wait "$pid" && status=0 || status=$?
finished_pkg=${job_pkgs[$pid]}
if [ "$status" -eq 0 ]; then
echo "✔ [SUCCESS] ${finished_pkg}"
passed_packages+=("$finished_pkg")
else
echo "✘ [FAILURE] ${finished_pkg} (Exit Code: ${status})"
failed_packages+=("$finished_pkg")
fi
unset "job_pids[$pid]"
unset "job_pkgs[$pid]"
active_jobs=$((active_jobs - 1))
fi
done
sleep 0.1
done
Comment on lines +188 to +206

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

Use the extracted reap_finished_jobs helper function here to simplify the concurrency limit loop.

Suggested change
while [ "$active_jobs" -ge "$MAX_JOBS" ]; do
for pid in "${!job_pids[@]}"; do
if ! kill -0 "$pid" 2>/dev/null; then
wait "$pid" && status=0 || status=$?
finished_pkg=${job_pkgs[$pid]}
if [ "$status" -eq 0 ]; then
echo "✔ [SUCCESS] ${finished_pkg}"
passed_packages+=("$finished_pkg")
else
echo "✘ [FAILURE] ${finished_pkg} (Exit Code: ${status})"
failed_packages+=("$finished_pkg")
fi
unset "job_pids[$pid]"
unset "job_pkgs[$pid]"
active_jobs=$((active_jobs - 1))
fi
done
sleep 0.1
done
while [ "$active_jobs" -ge "$MAX_JOBS" ]; do
reap_finished_jobs
sleep 0.1
done


safe_pkg_name=$(echo "$pkg" | tr '/' '_')
log_file="${LOG_DIR}/${safe_pkg_name}.log"

echo "Spawning tests for ${pkg}..."
run_package_test "$pkg" > "$log_file" 2>&1 &
pid=$!
job_pids["$pid"]=$pid
job_pkgs["$pid"]=$pkg
active_jobs=$((active_jobs + 1))
done

# Reap remaining processes
while [ "$active_jobs" -gt 0 ]; do
for pid in "${!job_pids[@]}"; do
if ! kill -0 "$pid" 2>/dev/null; then
wait "$pid" && status=0 || status=$?
finished_pkg=${job_pkgs[$pid]}
if [ "$status" -eq 0 ]; then
echo "✔ [SUCCESS] ${finished_pkg}"
passed_packages+=("$finished_pkg")
else
echo "✘ [FAILURE] ${finished_pkg} (Exit Code: ${status})"
failed_packages+=("$finished_pkg")
fi
unset "job_pids[$pid]"
unset "job_pkgs[$pid]"
active_jobs=$((active_jobs - 1))
fi
done
sleep 0.1
done
Comment on lines +220 to +238

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

Use the extracted reap_finished_jobs helper function here to simplify the final reaping loop.

Suggested change
while [ "$active_jobs" -gt 0 ]; do
for pid in "${!job_pids[@]}"; do
if ! kill -0 "$pid" 2>/dev/null; then
wait "$pid" && status=0 || status=$?
finished_pkg=${job_pkgs[$pid]}
if [ "$status" -eq 0 ]; then
echo "✔ [SUCCESS] ${finished_pkg}"
passed_packages+=("$finished_pkg")
else
echo "✘ [FAILURE] ${finished_pkg} (Exit Code: ${status})"
failed_packages+=("$finished_pkg")
fi
unset "job_pids[$pid]"
unset "job_pkgs[$pid]"
active_jobs=$((active_jobs - 1))
fi
done
sleep 0.1
done
while [ "$active_jobs" -gt 0 ]; do
reap_finished_jobs
sleep 0.1
done


echo ""
echo "=================================================="
echo " TEST RUN SUMMARY "
echo "=================================================="
echo "Total tested: ${#PACKAGES_TO_TEST[@]}"
echo "Passed: ${#passed_packages[@]}"
echo "Failed: ${#failed_packages[@]}"
echo "=================================================="

if [ ${#failed_packages[@]} -gt 0 ]; then
echo ""
echo "!!! DETAILED LOGS FOR FAILED PACKAGES !!!"
for pkg in "${failed_packages[@]}"; do
safe_pkg_name=$(echo "$pkg" | tr '/' '_')
log_file="${LOG_DIR}/${safe_pkg_name}.log"
echo "--------------------------------------------------"
echo "LOGS FOR: ${pkg}"
echo "--------------------------------------------------"
[ -f "$log_file" ] && cat "$log_file"
echo ""
done
exit 1
fi

echo "All tests passed successfully!"
exit 0
2 changes: 2 additions & 0 deletions packages/django-google-spanner/setup.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# Copyright 2020 Google LLC
# Trigger parallel system test run

#
# Use of this source code is governed by a BSD-style
# license that can be found in the LICENSE file or at
Expand Down
2 changes: 2 additions & 0 deletions packages/google-api-core/setup.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# Copyright 2018 Google LLC
# Trigger parallel system test run

#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down
2 changes: 2 additions & 0 deletions packages/google-cloud-ndb/setup.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# Copyright 2018 Google LLC
# Trigger parallel system test run

#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down
2 changes: 2 additions & 0 deletions packages/sqlalchemy-bigquery/setup.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#!/usr/bin/env python
# Copyright (c) 2017 The sqlalchemy-bigquery Authors
# Trigger parallel system test run

#
# Permission is hereby granted, free of charge, to any person obtaining a copy of
# this software and associated documentation files (the "Software"), to deal in
Expand Down
Loading