feat(ci): improve test performance, fix failing tests #9021
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,25 +31,50 @@ fi | |
| test_script="${PROJECT_ROOT}/ci/run_single_test.sh" | ||
|
|
||
|
|
||
| for arg in "$@"; do | ||
| case "${arg}" in | ||
| --strict) | ||
| STRICT=true | ||
| ;; | ||
| esac | ||
| done | ||
|
|
||
| if [[ "${STRICT}" == "true" || "${STRICT}" == "1" ]]; then | ||
| if [ -z "${GIT_DIFF_ARG}" ]; then | ||
| echo "Error: STRICT mode requires GIT_DIFF_ARG to be set." >&2 | ||
| exit 1 | ||
| fi | ||
| set +e | ||
| git diff --quiet ${GIT_DIFF_ARG} | ||
| diff_status=$? | ||
| set -e | ||
| if [[ ${diff_status} -ne 0 && ${diff_status} -ne 1 ]]; then | ||
| echo "Error: STRICT mode git diff ${GIT_DIFF_ARG} failed with exit code ${diff_status}." >&2 | ||
| exit 1 | ||
| fi | ||
| else | ||
| if [ -z "${GIT_DIFF_ARG}" ]; then | ||
| if [ "${BUILD_TYPE}" == "presubmit" ]; then | ||
| # For presubmit build, we want to know the difference from the | ||
| # common commit in origin/main. | ||
| GIT_DIFF_ARG="origin/main..." | ||
|
|
||
| # Then fetch enough history for finding the common commit. | ||
| git fetch origin main --deepen=300 | ||
|
|
||
| elif [ "${BUILD_TYPE}" == "continuous" ]; then | ||
| # For continuous build, we want to know the difference in the last | ||
| # commit. This assumes we use squash commit when merging PRs. | ||
| GIT_DIFF_ARG="HEAD~.." | ||
|
|
||
| # Then fetch one last commit for getting the diff. | ||
| git fetch origin main --deepen=1 | ||
|
|
||
| else | ||
| # Run everything. | ||
| GIT_DIFF_ARG="" | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # Then detect changes in the test scripts. | ||
|
|
@@ -61,11 +86,11 @@ set -e | |
| if [[ "${changed}" -eq 0 ]]; then | ||
| echo "no change detected in ci" | ||
| else | ||
| # echo "skipping trigger of tests for now: tracking in #7540" | ||
| echo "change detected in ci, we should test everything" | ||
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | ||
| git diff ${GIT_DIFF_ARG} ci | ||
| GIT_DIFF_ARG="" | ||
|
Comment on lines
+89
to
+93
Contributor
Author
There was a problem hiding this comment. As tests now take ~30 minutes (down from 2 hours), we can reenable this and close #7540. Once we add PNPM workspaces and turbo caching, this number goes down to ~15!! |
||
| fi | ||
|
|
||
| # Now we have a fixed list, but we can change it to autodetect if | ||
|
|
@@ -82,18 +107,21 @@ subdirs=( | |
| ) | ||
|
|
||
| RETVAL=0 | ||
| # These following APIs need an explicit credential file to run properly (or oAuth2, which we don't support in this repo). | ||
| # When we hit these packages, we will run the "samples with credentials" trigger, which contains the credentials as an env variable | ||
|
|
||
| tests_with_credentials="core/packages/google-auth-library-nodejs/ packages/google-analytics-admin/ packages/google-area120-tables/ packages/google-analytics-data/ packages/google-iam-credentials/ packages/google-apps-meet/ packages/google-chat/ packages/google-streetview-publish/ packages/google-cloud-developerconnect/" | ||
|
|
||
| # Some packages are only used by our bots and automation. These packages do not need to run on Windows and | ||
| # often employ platform specific code like file system interaction. Some packages may also fail | ||
| # on Windows due to incompatible npm scripts. | ||
| # | ||
| # Until these packages can be updated to be OS agnostic, we will skip them on Windows. | ||
| windows_exempt_tests="core/ core/packages/ core/dev-packages/ .github/scripts/fixtures/ .github/scripts/tests/ core/packages/gapic-node-processing/ core/packages/typeless-sample-bot/ handwritten/cloud-profiler/" | ||
|
|
||
| # Gather all test directories into an array | ||
| test_dirs=() | ||
|
|
||
| for subdir in ${subdirs[@]}; do | ||
| for d in `ls -d ${subdir}/*/`; do | ||
| if [ -s "ignore.json" ] && jq -e ".ignored[] | select(. == \"$d\")" ignore.json > /dev/null 2>&1; then | ||
|
|
@@ -116,7 +144,7 @@ for subdir in ${subdirs[@]}; do | |
| # System tests for packages are broken and blocking PRs. | ||
| # See https://github.com/googleapis/google-cloud-node/issues/7976. | ||
| # | ||
| # Per https://github.com/googleapis/google-cloud-node/issues/7921, | ||
| # we are likely to permanently remove these tests in the near future. | ||
| if [[ "${subdir}" == "packages" && "${TEST_TYPE}" == "system" ]]; then | ||
| echo "Skipping ${TEST_TYPE} test for packages: ${d}" | ||
|
|
@@ -126,7 +154,7 @@ for subdir in ${subdirs[@]}; do | |
| # Sample tests for packages are broken/flaky and blocking PRs. | ||
| # See https://github.com/googleapis/google-cloud-node/issues/7976#issuecomment-4210458096. | ||
| # | ||
| # Per https://github.com/googleapis/google-cloud-node/issues/7921, | ||
| # we are likely to permanently remove these tests in the near future. | ||
| if [[ "${subdir}" == "packages" && "${TEST_TYPE}" == "samples" ]]; then | ||
| echo "Skipping ${TEST_TYPE} test for packages: ${d}" | ||
|
|
@@ -204,21 +232,51 @@ for subdir in ${subdirs[@]}; do | |
| fi | ||
| fi | ||
| if [ "${should_test}" = true ]; then | ||
| test_dirs+=("${d}") | ||
| fi | ||
| done | ||
| done | ||
| # If DRY_RUN_SHARDS is set, output dynamic matrix values to GitHub Actions and exit | ||
| if [[ "${DRY_RUN_SHARDS}" == "true" ]]; then | ||
| count=${#test_dirs[@]} | ||
| if [[ $count -gt 15 ]]; then | ||
| matrix="[0, 1, 2, 3, 4]" | ||
| total="5" | ||
| else | ||
| matrix="[0]" | ||
| total="1" | ||
| fi | ||
| if [[ -n "${GITHUB_OUTPUT}" ]]; then | ||
| echo "shard_matrix=${matrix}" >> "${GITHUB_OUTPUT}" | ||
| echo "shard_total=${total}" >> "${GITHUB_OUTPUT}" | ||
| else | ||
| echo "shard_matrix=${matrix}" | ||
| echo "shard_total=${total}" | ||
| fi | ||
| exit 0 | ||
| fi | ||
|
|
||
| # If SHARD_TOTAL and SHARD_INDEX are provided, we will only run a subset of the tests. | ||
| for i in "${!test_dirs[@]}"; do | ||
| d="${test_dirs[$i]}" | ||
|
|
||
| if [[ -n "${SHARD_TOTAL}" && -n "${SHARD_INDEX}" ]]; then | ||
| if (( SHARD_TOTAL > 0 && i % SHARD_TOTAL != SHARD_INDEX )); then | ||
| continue | ||
|
Contributor
There was a problem hiding this comment. We should just keep an eye on whether there are differences between the shard durations (in case multiple "heavy" targets land in a single shard consistently.)
Contributor
Author
There was a problem hiding this comment. Yes, I've already seen this happen that Shard 0 is a little heavier, probably because of the
Contributor
There was a problem hiding this comment.
This it what I'd do.
+1 |
||
| fi | ||
| fi | ||
|
|
||
| echo "running test in ${d}" | ||
| pushd "${d}" >/dev/null | ||
| # Temporarily allow failure. | ||
| set +e | ||
| "${test_script}" | ||
| ret=$? | ||
| set -e | ||
| if [ ${ret} -ne 0 ]; then | ||
| exit ${ret} | ||
| fi | ||
| popd >/dev/null | ||
| done | ||
|
|
||
| exit 0 | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
can we reuse test_dirs from setup?
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.
I went with this way because:
test_dirswithgit difftakes ~1 second. Passing it via GitHub Actions adds ~5-10 seconds of network transfer and disk I/O per shard.ci/run_conditional_tests.shto be executed standalone