[DRAFT] chore: add test sharding#17438
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces parallel execution for system tests and implements a sharding mechanism for CI jobs, including a new Python script to group packages and updates to the test runner script. Feedback on these changes focuses on improving reliability and safety: first, by using an EXIT trap in .kokoro/system.sh to guarantee cleanup of isolated gcloud configuration directories in case of test failures; and second, by avoiding global toggles of set -e in ci/run_conditional_tests.sh and instead capturing test exit codes using the || operator.
| # Isolate gcloud config for parallel execution | ||
| export CLOUDSDK_CONFIG=$(mktemp -d) |
There was a problem hiding this comment.
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.
| # 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 |
| # Clean up isolated gcloud config | ||
| rm -rf "${CLOUDSDK_CONFIG}" |
There was a problem hiding this comment.
With the EXIT trap handling the cleanup of CLOUDSDK_CONFIG automatically, this manual cleanup is redundant and can be safely removed.
| set +e | ||
| ${test_script} | ||
| ret=$? | ||
| set -e |
There was a problem hiding this comment.
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.
| set +e | |
| ${test_script} | |
| ret=$? | |
| set -e | |
| ret=0 | |
| ${test_script} || ret=$? |
WIP with test sharding