ci: increase test-system timeout #22386
Conversation
📝 WalkthroughWalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
akhilkumarpilli
requested review from
auricom,
julienrbrt and
tac0turtle
as code owners
akhilkumarpilli
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/Makefile (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/Makefile (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (1)
tests/systemtests/Makefile (1)
8-8: LGTM! The timeout addition looks reasonable.The 15-minute timeout is a good default for system tests, providing enough time for comprehensive testing while preventing indefinite hangs.
Let's verify the historical test execution times to confirm this timeout is appropriate:
✅ Verification successful
Based on the search results, I can now verify the timeout change:
15-minute timeout is appropriate for system tests
The timeout value of 15 minutes is reasonable because:
- System tests are integration-level tests that exercise full end-to-end functionality
- The codebase shows other long-running tests with similar timeouts:
- Integration tests use 30m timeout (
tests/Makefile)- Snapshot tests use 1m timeout per operation
- Many test files contain long-running operations with multiple block commits
The change aligns with testing best practices by:
- Providing enough time for comprehensive system-level testing
- Setting an upper bound to catch potential infinite loops/hangs
- Using a round number that's easy to adjust if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check historical test execution times in CI logs # Note: This helps validate if 15m is sufficient based on past runs gh run list --workflow=".github/workflows/test.yml" --json conclusion,durationMS --jq '.[] | select(.conclusion=="success") | .durationMS' | awk '{printf "%.2f minutes\n", $1/60000}' | sort -nLength of output: 428
Script:
#!/bin/bash # Let's try a different approach to check test execution patterns # Look for any existing timeout configurations or test duration patterns in the codebase # Search for timeout related configurations in test files and makefiles rg -g '**/*test*' -g 'Makefile' -i 'timeout|duration' -A 2 -B 2 # Also check for any system test specific configurations fd Makefile --exec grep -i 'system.*test' {} \;Length of output: 230254
| @@ -5,7 +5,7 @@ WAIT_TIME ?= 45s | |||
| all: test format | |||
|
|
|||
| test: | |||
| go test -mod=readonly -failfast -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) | |||
| go test -mod=readonly -failfast -timeout=15m -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the timeout configurable.
For flexibility across different environments and test scenarios, consider making the timeout configurable via an environment variable, similar to how WAIT_TIME is handled.
- go test -mod=readonly -failfast -timeout=15m -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2)
+TEST_TIMEOUT ?= 15m
+ go test -mod=readonly -failfast -timeout=$(TEST_TIMEOUT) -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2)📝 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.
| go test -mod=readonly -failfast -timeout=15m -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) | |
| TEST_TIMEOUT ?= 15m | |
| go test -mod=readonly -failfast -timeout=$(TEST_TIMEOUT) -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose $(if $(findstring v2,$(COSMOS_BUILD_OPTIONS)),--binary=simdv2) |
(cherry picked from commit 94919dc)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit