chore: optimize zonal tests cloning and fix paths#16581
chore: optimize zonal tests cloning and fix paths#16581chandra-siri wants to merge 2 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the zonal test execution environment to use a sparse checkout of the google-cloud-python monorepo instead of the standalone python-storage repository. It also adjusts the Cloud Build configuration to point to the new script location. Review feedback suggests improving script reliability by adding set -e, fetching the specific commit SHA directly for better robustness, and reconsidering the removal of zonal snippet tests to maintain test coverage.
| # Clone the repository and checkout the specific commit from the build trigger. | ||
| git clone https://github.com/googleapis/python-storage.git | ||
| cd python-storage | ||
| git clone --no-checkout --depth 1 --sparse --filter=blob:none https://github.com/googleapis/google-cloud-python.git |
There was a problem hiding this comment.
Adding set -e ensures the script terminates immediately if any command fails, which is a best practice for CI scripts to avoid running in an inconsistent state.
| git clone --no-checkout --depth 1 --sparse --filter=blob:none https://github.com/googleapis/google-cloud-python.git | |
| set -e | |
| git clone --no-checkout --depth 1 --sparse --filter=blob:none https://github.com/googleapis/google-cloud-python.git |
| git fetch origin "refs/pull/${_PR_NUMBER}/head" | ||
| git checkout ${COMMIT_SHA} |
There was a problem hiding this comment.
Fetching the specific COMMIT_SHA directly is more robust than fetching the PR head. It ensures the exact commit that triggered the build is used, and it works correctly even for non-PR triggers or when the PR has been updated.
| git fetch origin "refs/pull/${_PR_NUMBER}/head" | |
| git checkout ${COMMIT_SHA} | |
| git fetch --depth 1 origin ${COMMIT_SHA} | |
| git checkout FETCH_HEAD |
| CURRENT_ULIMIT=$(ulimit -n) | ||
| echo '--- Running Zonal tests on VM with ulimit set to ---' $CURRENT_ULIMIT | ||
| pytest -vv -s --log-format='%(asctime)s %(levelname)s %(message)s' --log-date-format='%H:%M:%S' tests/system/test_zonal.py | ||
| pytest -vv -s --log-format='%(asctime)s %(levelname)s %(message)s' --log-date-format='%H:%M:%S' samples/snippets/zonal_buckets/zonal_snippets_test.py |
There was a problem hiding this comment.
The removal of the zonal snippets test reduces test coverage. If these tests are still relevant for zonal buckets, they should be retained and their paths updated if necessary (e.g., to samples/snippets/zonal_buckets/zonal_snippets_test.py relative to the package directory).
chalmerlowe
left a comment
There was a problem hiding this comment.
LGTM.
With the following caveat. I am not an expert in cloudbuild and how it should work. Having said that, I don't see anything here that causes me great concern.
This PR contains changes to optimize zonal tests cloning and fix paths.