test: use gotestsum to retry only the failing test #1990
Conversation
Router image scan passed✅ No security vulnerabilities found in image: |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
WalkthroughThe integration test workflow was refactored to use a matrix over new Makefile targets, consolidating test execution into a single step per target. The Makefile was updated to add dedicated targets for integration and flaky tests, with specific parameters for retries, timeouts, and test selection, and to generalize the test command. Changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/router-ci.yaml (1)
222-225: Quote the matrix variable to survive special-char targetsAlthough unlikely today, quoting is a good habit and costs nothing:
- run: make ${{ matrix.make_target }} + run: make "${{ matrix.make_target }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/router-ci.yaml(2 hunks)router-tests/Makefile(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/router-ci.yaml
[error] 142-142: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
router-tests/Makefile (1)
51-53: Concurrency flags are contradictory – confirm intent
-p 1runs one package at a time, while--parallel 10lets tests within
that single package spawn up to 10 goroutines.
If the goal is maximum throughput on CI, consider either:
- Remove
-p 1to let Go test run multiple packages concurrently; or- Drop
--parallel 10and rely on default intra-package parallelism.Please double-check which level of parallelism is required for the flaky
tests (some of them are flaky precisely because of parallel execution).
| integration-tests: | ||
| $(MAKE) test test_params="-run '^Test[^(Flaky)]' --timeout=10m" test_target="./..." | ||
|
|
There was a problem hiding this comment.
Regex does not exclude “Flaky” tests – it matches only one extra rune
^Test[^(Flaky)] is a character class, i.e. it allows exactly one
character that is not “(”, “F”, “l”, “a”, “k”, or “y”.
It will not exclude TestFlaky*; instead it runs almost nothing.
Go test (RE2) lacks negative look-ahead, so a common workaround is:
- $(MAKE) test test_params="-run '^Test[^(Flaky)]' --timeout=10m" test_target="./..."
+ # run all tests whose name does **not** start with TestFlaky
+ $(MAKE) test test_params="-run '^Test([^F]|F[^l]|Fl[^a]|Fla[^k]|Flak[^y])' --timeout=10m" test_target="./..."Alternatively, move flaky tests into a separate package or use build tags for
clean separation.
Failing to fix this will leave the non-flaky suite virtually empty.
🤖 Prompt for AI Agents
In router-tests/Makefile around lines 48 to 50, the regex `^Test[^(Flaky)]`
incorrectly uses a character class and does not exclude tests with "Flaky" in
their names, causing almost no tests to run. Replace this regex with a pattern
that matches test names starting with "Test" but excludes those containing
"Flaky" by using a workaround such as `^Test[^F].*` or another suitable pattern
that avoids matching "Flaky" tests, or alternatively separate flaky tests into a
different package or use build tags for exclusion.
| gotestsum $(gotestsum_params) --packages=$(test_target) -f $(FORMAT) -- -ldflags=-extldflags=-Wl,-ld_classic $(test_params) -race $(test_target) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid passing the package list twice & quote optional params
gotestsum already receives the package list through --packages=$(test_target).
The trailing $(test_target) positional arg is redundant and can break when the
variable is empty/over-ridden. In addition, any of the parameter variables
containing spaces should be quoted to prevent word-splitting.
- gotestsum $(gotestsum_params) --packages=$(test_target) -f $(FORMAT) -- -ldflags=-extldflags=-Wl,-ld_classic $(test_params) -race $(test_target)
+ gotestsum "$(gotestsum_params)" --packages="$(test_target)" -f "$(FORMAT)" -- -ldflags=-extldflags=-Wl,-ld_classic $(test_params) -race🤖 Prompt for AI Agents
In router-tests/Makefile around lines 14 to 15, remove the redundant positional
package argument at the end of the gotestsum command since the packages are
already specified with --packages=$(test_target). Also, quote the parameter
variables like "$(gotestsum_params)" and "$(test_params)" to prevent
word-splitting issues when they contain spaces.
| make_target: | ||
| - integration-tests | ||
| - integration-tests-flaky | ||
| services: |
There was a problem hiding this comment.
Trailing spaces break YAML-lint & block merges
YAML-lint is already flagging these lines:
Remove the blanks to keep the pipeline green.
- make_target:
- - integration-tests
- - integration-tests-flaky
+ make_target:
+ - integration-tests
+ - integration-tests-flaky📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| make_target: | |
| - integration-tests | |
| - integration-tests-flaky | |
| services: | |
| make_target: | |
| - integration-tests | |
| - integration-tests-flaky | |
| services: |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 142-142: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/router-ci.yaml around lines 142 to 145, there are trailing
spaces after the list items under make_target which cause YAML-lint errors and
block merges. Remove any trailing spaces at the end of these lines to ensure the
YAML syntax is clean and the pipeline passes linting checks.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Motivation and Context
We are retrying the flaking tests with a github action, that after a failure of a single test is going to retry the whole testsuite. With this PR we are using gotestsum retry instead, that is going to retry only the specific test that is failing.
Checklist
Summary by CodeRabbit