Migrate unit tests to use Go workspace #20872
Conversation
|
Skipping CI for Draft Pull Request. |
ivanvc
force-pushed
the
migrate-test-unit-to-use-workspace
branch
6 times, most recently
from
6fabb4a to
0f51f15
Compare
|
/test all |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 20 files with indirect coverage changes @@ Coverage Diff @@
## main #20872 +/- ##
==========================================
- Coverage 69.20% 69.11% -0.10%
==========================================
Files 422 422
Lines 34831 34831
==========================================
- Hits 24104 24072 -32
- Misses 9328 9354 +26
- Partials 1399 1405 +6 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Introduce the new functions: run_go_tests and run_go_test_expanding_packages, which implement (without Shellcheck exceptions) the old behavior from go_test, simplifying the arguments received. Replaced the three possible values mode with: 1. keep_going: By using the KEEP_GOING_TESTS environment variable. This mode expanded the relative modules; it can be run by using run_go_tests_expanding_packages. 2. parallel: It worked without expanding the relative modules with the absolute modules. Therefore, this mode can be run directly using run_go_tests. 3. fail_fast: By adding an argument "-failfast", and not setting the KEEP_GOING_TESTS environment variable. The argument flags_for_package_func can be replaced by regular arguments passed to the function. These two changes simplify the complexity of the go test wrapper function and allow further customization in the callers. The JUnit XML generation works the same way, by reading the JUNIT_REPORT_DIR or ARTIFACTS environment variable. It generates the same reports. Temporarily introduce the get_junit_filename_prefix function, as a replacement for junitFilenamePrefix. The latter will be removed once all test targets are using the new functions. Finally, update the unit tests function to make use of the new functions, and allow running the tests by using the workspace (i.e., there's no need to change directories anymore). Signed-off-by: Ivan Valdes <[email protected]>
ivanvc
force-pushed
the
migrate-test-unit-to-use-workspace
branch
from
0f51f15 to
cef8274
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivanvc, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| # run_go_tests_expanding_packages [arguments to pass to go test] | ||
| # Expands the packages in the list of arguments, i.e. ./... into a list of | ||
| # packages for that given module. Then, it calls run_go_tests with the expanded | ||
| # packages. | ||
| function run_go_tests_expanding_packages { | ||
| local packages=() | ||
| local args=() | ||
| for arg in "$@"; do | ||
| if [[ "${arg}" =~ ./ ]]; then | ||
| packages+=("${arg}") | ||
| else | ||
| args+=("${arg}") | ||
| fi | ||
| done | ||
|
|
||
| # Expanding patterns (like ./...) into list of packages | ||
| local unpacked_packages=() | ||
| while IFS='' read -r line; do unpacked_packages+=("$line"); done < <( | ||
| go list "${packages[@]}" | ||
| ) | ||
|
|
||
| run_go_tests "${unpacked_packages[@]}" "${args[@]}" | ||
| } |
There was a problem hiding this comment.
This function isn't used at all. Why add it?
There was a problem hiding this comment.
It's added for compatibility with the old go_tests function. It's going to be used by integration tests.
|
|
||
| function unit_pass { | ||
| run_for_modules run_unit_tests "$@" | ||
| run_for_all_workspace_modules \ |
There was a problem hiding this comment.
I see there is also a function run_for_workspace_modules, which seems confusing (please at least ensure adding sufficient comment to clarify the difference, which can be addressed separately). What's the difference between the them? Why use this one, not the other one?
There was a problem hiding this comment.
Once we finish migrating all the Makefile targets, we can clean up the old functions. Right now, as I'm doing a staged migration, I can't do a cleanup yet.
| local pkgs="${1:-./...}" | ||
| shift 1 | ||
| # shellcheck disable=SC2068 #For context see - https://github.com/etcd-io/etcd/pull/16433#issuecomment-1684312755 | ||
| go_test "${pkgs}" "parallel" : -short -timeout="${TIMEOUT:-3m}" ${COMMON_TEST_FLAGS[@]:-} ${RUN_ARG[@]:-} "$@" |
There was a problem hiding this comment.
Eventually you will remove the function go_test separately?
4 participants
Introduce the new functions:
run_go_testsandrun_go_test_expanding_packages, which implement (without Shellcheck exceptions) the old behavior fromgo_test, simplifying the arguments received.Replaced the three possible values mode with:
KEEP_GOING_TESTSenvironment variable. This mode expanded the relative modules; it can be run by usingrun_go_tests_expanding_packages.run_go_tests.KEEP_GOING_TESTSenvironment variable.The argument
flags_for_package_funccan be replaced by regular arguments passed to the function.These two changes simplify the complexity of the go test wrapper function and allow further customization in the callers.
The JUnit XML generation works the same way, by reading the
JUNIT_REPORT_DIRorARTIFACTSenvironment variable. It generates the same reports.Temporarily introduce the get_junit_filename_prefix function, as a replacement for
junitFilenamePrefix. The latter will be removed once all test targets are using the new functions.Finally, update the unit tests function to make use of the new functions, and allow running the tests by using the workspace (i.e., there's no need to change directories anymore).
Part of #18409.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.