e2e: Use gotestsum for CI e2e runs, change e2e ARGS handling #2904
Conversation
zmerlynn
added
feature-freeze-do-not-merge
zmerlynn
requested review from
markmandel and
gongmax
and removed request for
cyriltovena and
pooneh-m
|
Build Failed 😱 Build Id: f0bb7851-0edd-4b5a-955b-20b7a6e6dc5c To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 👏 Build Id: 1abfd5b1-7c2f-4955-b6ab-d3cf76c8f1a2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
build/Makefile
Outdated
| else | ||
| gotestsum_format=--format=standard-quiet | ||
| endif | ||
| gotestsum_cmd=gotestsum $(gotestsum_format) --jsonfile=$(gotestsum_json) --hide-summary=skipped --rerun-fails=2 $(gotestsum_post_run_command) $(GOTESTSUM_ARGS) |
There was a problem hiding this comment.
Is gotestsum_post_run_command set anywhere? Is it supposed to be the same thing as the --post-run-command in line 174?
There was a problem hiding this comment.
Is GOTESTSUM_ARGS needed? You mentioned "use GOTESTSUM_ARGS to pass args to gotestsum (e.g. --rerun-fails=5)", but looks like those fields (i.e. jsonfile, hide-summary, rerun-failes) are set explicitly here.
There was a problem hiding this comment.
gotestsum_post_run_command was a vestigal of a previous factoring - missed that, thanks. Removed.
The idea behind GOTESTSUM_ARGS was just as an entry point to allow the user to specify more args specifically for gotestsum. We default to the ones you mention, but I intentionally but it at the end so if you want to override any of those default things (e.g. --rerun-fails=5), you can do that.
| --pullsecret=$(IMAGE_PULL_SECRET) | ||
| echo "Starting e2e controller failure test!" | ||
| ifdef E2E_USE_GOTESTSUM | ||
| $(GOTESTSUM) --packages=$(agones_package)/test/e2e/controller -- $(go_test_args) $(ARGS) -parallel=1 -args \ |
There was a problem hiding this comment.
&(ARGS) has the -parallel=16 set but here -parallel is explicitly set to 1. Should the $(ARGS) be removed?
There was a problem hiding this comment.
I think you're referring to the ARGS coming from e2e.sh, but two things:
- in my mind,
ARGSis generically the args we pass togo test- it just happens thate2e.shexplicitly overrides it, I think because sometime back someone decided to force the parallelism possibly higher than the default. But you can alsomake test-e2e, so this should remain generic to that as well - that said, for this test specifically we want to run it serially regardless (here I am just reflecting what we do in the
go testcase). Why we chose to do this via-parallel=1vs just not removing thet.Parallel(), I'm not clear, but I chose not to change the behavior here.
|
Build Succeeded 👏 Build Id: edc33727-25c1-41ee-8011-8e2679a2ffe8 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
BTW, formatting changes can be previewed in the logs here. |
Kalaiselvi84
removed
the
feature-freeze-do-not-merge
The main goal of this PR is to add gotestsum (https://pkg.go.dev/gotest.tools/gotestsum#section-readme) as an optional way to run e2es. gotestsum wraps `go test -json` and, aside from some fancy formatting options, allows us to re-run flaky tests. As we start adding more permutations to the test matrix, this will become important. (Also important will become actually monitoring flakes, but that's the next step.) We do this by: * Installing `gotestsum` in the build image * Adding a new E2E_USE_GOTESTSUM option to the Makefile that defaults to 2x reruns and super-terse output. * Also added a GOTESTSUM_TESTNAME_AND_VERBOSE option meant for CI that shows the status of all of the tests then dumps the full verbose output for diagnostics. One thing that changes slightly after this PR is the arguments involved in invoking e2es via `make`. Previously, both `ARGS` and `GO_E2E_TEST_ARGS` were passed to `go test`. In order to hack arguments for re-runs, `gotestsum` requires a delineation between the arguments to `go test` and the arguments to the test binary. However, so that usage is consistent when E2E_USE_GOTESTSUM is present or not, I changed both to do the equivalent of: ``` go test ${ARGS} -args ${GO_E2E_TEST_ARGS} ``` In other words, for the e2e targets: * Use `ARGS` to pass args to `go test` (e.g. `-parallel 16`) * Use `GO_E2E_TEST_ARGS` to pass args to the test binary (e.g. `--kubeconfig`) * If `E2E_USE_GOTESTSUM` is on, use `GOTESTSUM_ARGS` to pass args to `gotestsum` (e.g. `--rerun-fails=5`)
|
Build Failed 😱 Build Id: 941e10fb-4cbe-4031-be6c-8c0b018fa692 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gongmax, markmandel, roberthbailey, zmerlynn 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 |
|
In |
|
The last 4 builds in CI have all failed with that same error, which looks like a CI infrastructure bug than anything triggered by this PR. |
|
Build Succeeded 👏 Build Id: a8cdc9f8-f686-4bd8-aded-3ec9735f6077 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
markmandel
added
kind/feature
The main goal of this PR is to add
gotestsumas an optional way to run e2es.gotestsumwrapsgo test -jsonand, aside from some fancy formatting options, allows us to re-run flaky tests. As we start adding more permutations to the test matrix, this will become important. (Also important will become actually monitoring flakes, but that's the next step.)We do this by:
gotestsumin the build imageE2E_USE_GOTESTSUMoption to the Makefile that defaults to 2x reruns and super-terse output.GOTESTSUM_TESTNAME_AND_VERBOSEoption meant for CI that shows the status of all of the tests then dumps the full verbose output for diagnostics.One thing that changes slightly after this PR is the arguments involved in invoking e2es via
make. Previously, bothARGSandGO_E2E_TEST_ARGSwere passed togo test. In order to hack arguments for re-runs,gotestsumrequires a delineation between the arguments togo testand the arguments to the test binary. However, so that usage is consistent when E2E_USE_GOTESTSUM is present or not, I changed both to do the equivalent of:In other words, for the e2e targets:
ARGSto pass args togo test(e.g.-parallel 16)GO_E2E_TEST_ARGSto pass args to the test binary (e.g.--kubeconfig)E2E_USE_GOTESTSUMis on, useGOTESTSUM_ARGSto pass args togotestsum(e.g.--rerun-fails=5)/kind cleanup