Conversation
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
chingor13
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to skip unit tests for specific modules during bulk builds by using a Maven profile bulkTests. The changes in .kokoro/build.sh activate this profile when a full build is performed, and the changes in the various pom.xml files define this profile to set <skipTests>true</skipTests>. The overall approach is sound. I have a suggestion to improve the robustness of the shell script by using an array for the optional Maven profile argument, which is a best practice in bash scripting to avoid potential issues with word splitting.
| EXTRA_PROFILE_OPT="" | ||
| else | ||
| EXTRA_PROFILE_OPT="-PbulkTests" |
There was a problem hiding this comment.
To handle optional arguments more robustly in bash, it's better to use an array. This avoids potential word-splitting issues. This converts EXTRA_PROFILE_OPT to an array EXTRA_PROFILE_OPTS; a corresponding change will be needed where it's used (see my other comment).
| EXTRA_PROFILE_OPT="" | |
| else | |
| EXTRA_PROFILE_OPT="-PbulkTests" | |
| EXTRA_PROFILE_OPTS=() | |
| else | |
| EXTRA_PROFILE_OPTS=("-PbulkTests") |
| -Danimal.sniffer.skip=true \ | ||
| -Dmaven.wagon.http.retryHandler.count=5 \ | ||
| -T 1C ${SUREFIRE_JVM_OPT} | ||
| -T 1C ${SUREFIRE_JVM_OPT} ${EXTRA_PROFILE_OPT} |
There was a problem hiding this comment.
To correctly expand the EXTRA_PROFILE_OPTS array, use "${EXTRA_PROFILE_OPTS[@]}". This syntax safely handles both empty and non-empty arrays, preventing issues with word splitting and ensuring arguments are passed correctly.
| -T 1C ${SUREFIRE_JVM_OPT} ${EXTRA_PROFILE_OPT} | |
| -T 1C ${SUREFIRE_JVM_OPT} "${EXTRA_PROFILE_OPTS[@]}" |
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| java: [11, 17, 21, 25] |
3 participants
Uses a GitHub actions matrix and the dorny/paths-filter action (already in use) to conditionally run split unit tests for a select number of modules.
When running the bulk unit test suite, we enable use the
bulkTestsprofile. This profile is configured to skip the tests for the ones that have split tests.