fix(tests/e2e): fix flaky E2E tests with node readiness checks and extended timeout - #351
fix(tests/e2e): fix flaky E2E tests with node readiness checks and extended timeout#351g0spel wants to merge 1 commit into
Conversation
…ks and increasing tx confirmation timeout Root cause: Chain stalls/crashes during cold starts or under CI resource pressure cause transactions to broadcast with code=0, height=0, but the 1-minute tx confirmation timeout in defaultExecValidation is insufficient for node recovery. With Docker RestartPolicy=no, a dead container never recovers, leading to silent "Condition never satisfied" failures with no diagnostics. Changes: - Added waitForNodeReady() helper that polls the REST API endpoint before entering the tx confirmation loop, giving the node up to 2 minutes to become responsive during cold starts or after stalls. - Increased tx confirmation timeout from 1 to 3 minutes in both defaultExecValidation and expectErrExecValidation, providing sufficient window for tx inclusion under resource pressure. - Improved queryKiichainTx() error messages to include tx hash and endpoint URL in every error path, enabling faster diagnosis of CI failures. - Added nil-safety for tx_response type assertion in queryKiichainTx() to prevent panics on unexpected response formats. Fixes: KiiChain#176
WalkthroughThis change updates the e2e test suite to reduce flaky transaction confirmation failures. Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Validation as defaultExecValidation
participant Ready as waitForNodeReady
participant Query as queryKiichainTx
participant API as Validator REST API
Validation->>Ready: check node reachable
Ready->>API: GET tx.height=0
API-->>Ready: HTTP 200
Ready-->>Validation: node ready
loop poll up to 3 minutes
Validation->>Query: queryKiichainTx(txHash, endpoint)
Query->>API: GET tx by hash
API-->>Query: tx_response or error
Query-->>Validation: confirmation result or error
end
Related issues: Suggested labels: tests, e2e, bugfix Suggested reviewers: kiichain-e2e-maintainers 🐰 A node once slept while tests did wait, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/query.go`:
- Around line 41-47: The tx response handling in the polling helper still
assumes txResp["code"] is always present, which can panic when it is omitted on
successful transactions. Update the logic in the tx response check to safely
read and type-assert the code field in the same area that inspects tx_response,
and only compare it when the field exists; keep the existing error path for
non-zero codes and let missing code be treated as success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f81fb4d-5b16-451b-a626-193b83cec6ac
📒 Files selected for processing (2)
tests/e2e/e2e_exec_test.gotests/e2e/query.go
| txResp, ok := result["tx_response"].(map[string]interface{}) | ||
| if !ok { | ||
| return fmt.Errorf("tx %s response missing tx_response field", txHash) | ||
| } | ||
| if v := txResp["code"]; v.(float64) != 0 { | ||
| return fmt.Errorf("tx %s failed with status code %v", txHash, v) | ||
| return fmt.Errorf("tx %s failed with code %v", txHash, v) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tests/e2e/query.go ---\n'
nl -ba tests/e2e/query.go | sed -n '1,120p'
printf '\n--- search for TxResponse definition and code tag ---\n'
rg -n 'type TxResponse|json:"code,omitempty"|tx_response|code' -S .Repository: KiiChain/kiichain
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/e2e/query.go ---'
nl -ba tests/e2e/query.go | sed -n '1,120p'
echo
echo '--- TxResponse definition and code tag ---'
rg -n 'type TxResponse|json:"code,omitempty"|json:"code"' -S .Repository: KiiChain/kiichain
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/e2e/query.go ---'
cat -n tests/e2e/query.go | sed -n '1,120p'
echo
echo '--- TxResponse definition ---'
rg -n 'type TxResponse|json:"code,omitempty"|json:"code"' -S .Repository: KiiChain/kiichain
Length of output: 4759
🌐 Web query:
Cosmos SDK TxResponse json:"code,omitempty" tx_response code omitted on success /cosmos/tx/v1beta1/txs
💡 Result:
In the Cosmos SDK, the code field in the TxResponse object is typically omitted from JSON output on success because it is defined with the omitempty JSON tag [1][2][3]. Since a successful transaction in the Cosmos SDK (specifically at the ABCI level) has a return code of 0 [4], the standard Go json marshaler treats this as a zero value and omits it from the serialized output when the json:"code,omitempty" tag is present. The TxResponse struct (found in cosmos-sdk/types/result.go) is used across various endpoints, including those under /cosmos/tx/v1beta1/txs [1][5][3]. While the underlying protobuf definition and internal Go struct include the code field, its visibility in JSON responses is governed by the serialization rules of the SDK's encoding layer, which adheres to the omitempty convention for this field [1][6]. If you are developing a client (such as a frontend or a service consuming these APIs) and need to verify the success of a transaction, you should not rely solely on the presence or absence of the code field. Instead, standard practice is to: 1. Check if the code field exists and is equal to 0 (if present). 2. Alternatively, treat the absence of the code field as implicitly successful (i.e., code == 0), given that the transaction was successfully processed by the node and returned a TxResponse [4]. For robust client-side logic, it is often recommended to use higher-level libraries (like CosmJS) which handle these serialization nuances and provide helper functions to distinguish between successful and failed transactions [4].
Citations:
- 1: https://github.com/cosmos/cosmos-sdk/blob/v0.53.6/types/result.go
- 2: https://github.com/cosmos/cosmos-sdk/blob/master/types/result.go
- 3: https://github.com/cosmos/cosmos-sdk/blob/main/types/result.go
- 4: add code property on the BroadcastTxSuccess interface. cosmos/cosmjs#878
- 5: https://cosmos-docs.mintlify.app/sdk/v0.53/learn/advanced/transactions
- 6: https://docs.cosmos.network/sdk/latest/learn/concepts/encoding
Unguarded code assertion can still panic on successful txs. txResp["code"] is omitted on success, so v.(float64) can hit nil and crash this polling path.
🐛 Proposed fix
- if v := txResp["code"]; v.(float64) != 0 {
- return fmt.Errorf("tx %s failed with code %v", txHash, v)
+ code, ok := txResp["code"].(float64)
+ if ok && code != 0 {
+ return fmt.Errorf("tx %s failed with code %v", txHash, code)
}📝 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.
| txResp, ok := result["tx_response"].(map[string]interface{}) | |
| if !ok { | |
| return fmt.Errorf("tx %s response missing tx_response field", txHash) | |
| } | |
| if v := txResp["code"]; v.(float64) != 0 { | |
| return fmt.Errorf("tx %s failed with status code %v", txHash, v) | |
| return fmt.Errorf("tx %s failed with code %v", txHash, v) | |
| } | |
| txResp, ok := result["tx_response"].(map[string]interface{}) | |
| if !ok { | |
| return fmt.Errorf("tx %s response missing tx_response field", txHash) | |
| } | |
| code, ok := txResp["code"].(float64) | |
| if ok && code != 0 { | |
| return fmt.Errorf("tx %s failed with code %v", txHash, code) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/query.go` around lines 41 - 47, The tx response handling in the
polling helper still assumes txResp["code"] is always present, which can panic
when it is omitted on successful transactions. Update the logic in the tx
response check to safely read and type-assert the code field in the same area
that inspects tx_response, and only compare it when the field exists; keep the
existing error path for non-zero codes and let missing code be treated as
success.
1 participant
Summary
Fixes #176 — the long-standing flaky E2E test issue where transactions broadcast with
code: 0, height: 0but are never confirmed, resulting inCondition never satisfiedfailures.Root Cause
When the chain stalls or restarts during E2E execution (cold CI start, resource pressure, Docker container crash), transactions are broadcast to mempool (
code: 0, height: 0) but never committed. The existing 1-minute tx confirmation timeout indefaultExecValidationis insufficient for node recovery.The Docker containers use
RestartPolicy: "no", so a crashed container stays dead — no amount of polling will recover it.Changes
waitForNodeReady()— New helper that polls the REST API endpoint before entering the tx confirmation loop, giving the node up to 2 minutes to become responsive during cold starts or after stalls.Increased timeout — Tx confirmation timeout raised from 1 to 3 minutes in both
defaultExecValidationandexpectErrExecValidation.Better diagnostics —
queryKiichainTx()error messages now include the tx hash, endpoint URL, and HTTP status in every error path, enabling faster debugging of CI failures.Nil-safety — Added type assertion guard in
queryKiichainTx()to prevent panics on unexpected API response formats.Verification
The approach was discussed in the original issue: @ramagumilar correctly identified it as "infrastructure/synchronization-related, not module-specific" and suggested waiting on explicit block height instead of fixed polling timeouts. PR #225 attempted timeout-only but was closed as "doesn't accomplish anything" by @Thaleszh — this PR adds the node-readiness pre-check that the timeout-only approach was missing.