Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
|
The CLA check failed. Please ensure you have:
After fixing these issues, comment 'recheck' to trigger the workflow again. |
|
The CLA check failed. Please ensure you have:
After fixing these issues, comment 'recheck' to trigger the workflow again. |
|
Kilo Code Review could not run — your account is out of credits. Add credits at app.kilo.ai to enable reviews on this change. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts replay status handling so that startup failures (notably in Docker Compose mode) preserve an APP_HALTED/APP_FAULT classification instead of being overwritten as INTERNAL_ERR, and so that a single failing test set doesn’t necessarily abort the entire Docker Compose replay run.
Changes:
- Added helper functions to map application/run-time errors to
TestSetStatus, and to decide when a replay run should abort. - Updated replay flow to avoid aborting the full run on
APP_HALTED/APP_FAULTwhen running in Docker Compose mode. - Added logic to resolve/override status in several error paths (consumed mocks, mock params updates, test case results retrieval, and final status resolution).
Comments suppressed due to low confidence (1)
pkg/service/replay/replay.go:450
- In Docker Compose mode, APP_HALTED/APP_FAULT no longer aborts the full run, but the flaky-check attempt loop will still continue rerunning the same failing test set (since abortTestRun stays false). That can waste time and may restart the same broken compose stack multiple times before moving on. Consider breaking out of the per-testset attempt loop immediately for terminal statuses (APP_HALTED/APP_FAULT/INTERNAL_ERR), while still allowing the outer loop to continue to the next test set in Docker Compose mode.
switch testSetStatus {
case models.TestSetStatusAppHalted:
testSetResult = false
abortTestRun = shouldAbortTestRun(testSetStatus, cmdType)
case models.TestSetStatusInternalErr:
testSetResult = false
abortTestRun = shouldAbortTestRun(testSetStatus, cmdType)
case models.TestSetStatusFaultUserApp:
testSetResult = false
abortTestRun = shouldAbortTestRun(testSetStatus, cmdType)
case models.TestSetStatusUserAbort:
return nil
case models.TestSetStatusFailed:
testSetResult = false
case models.TestSetStatusPassed:
testSetResult = true
case models.TestSetStatusIgnored:
testSetResult = false
case models.TestSetStatusNoTestsToRun:
testSetResult = false
}
if testSetStatus != models.TestSetStatusIgnored {
testRunResult = testRunResult && testSetResult
if abortTestRun {
break
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…keploy into fix/replay-after-startup-failure
|
The CLA check failed. Please ensure you have:
After fixing these issues, comment 'recheck' to trigger the workflow again. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| strings.Contains(errMsg, "connection reset by peer") || | ||
| strings.Contains(errMsg, "broken pipe") || | ||
| strings.Contains(errMsg, "server closed") || | ||
| strings.Contains(errMsg, "eof") || |
There was a problem hiding this comment.
WARNING: Overly broad string match
The check for "eof" is very short and could inadvertently match error messages that happen to contain "eof" as a substring (e.g., "malloc of size...", or any error mentioning "EOF" in a different context). Consider using a more specific match like "eof" at word boundaries, or checking for the exact string "eof" (as the full error message) or "unexpected eof".
| default: | ||
| testSetStatusByErrChan = models.TestSetStatusAppHalted | ||
| } | ||
| utils.LogError(r.logger, err, "application failed to run") |
There was a problem hiding this comment.
SUGGESTION: Error log lacks actionable next step
Per coding guidelines, error logs should provide a logical next step for the user, not just state what failed. Consider enhancing this message to guide users on troubleshooting, e.g., "application failed to run; check the application logs for details or verify the app command is correct".
| default: | ||
| testSetStatusByErrChan = models.TestSetStatusAppHalted | ||
| } | ||
| utils.LogError(r.logger, err, "application failed to run") |
There was a problem hiding this comment.
SUGGESTION: Error log lacks actionable next step
Same as line 855 - this error log should provide guidance to the user. Consider: "application failed to run; check the application logs for details or verify the app command is correct".
Code Review SummaryStatus: 3 Issues Found (unchanged from previous review) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Incremental Review NotesNew commits reviewed since
Previous issues remain: The 3 issues from the initial review are still present in unchanged code and should still be addressed. Other Observations (not in diff)Other error logs in unchanged code also lack actionable next steps (lines 1182, 1309, 1347, 1638). Per custom instructions, error logs should provide a logical next step for the user, not just state what failed. Consider a follow-up to enhance these messages. Positive Notes
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.5-opus-20251124 · 447,999 tokens |
|
The CLA check failed. Please ensure you have:
After fixing these issues, comment 'recheck' to trigger the workflow again. |
🚀 Keploy Performance Test ResultsMulti-Run Validation: Tests run 3 times, pipeline fails only if 2+ runs show regression.
Thresholds: P50 < 5ms, P90 < 15ms, P99 < 70ms, RPS >= 100 (±1% tolerance), Error Rate < 1% ✅ Result: PASSED - Only 1 out of 3 runs failed (threshold: 2) P50, P90, and P99 percentiles naturally filter out outliers |
Describe the changes that are made
APP_HALTEDinstead of being downgraded toINTERNAL_ERR.What type of PR is this? (check all applicable)
Added e2e test pipeline?
Added comments for hard-to-understand areas?
Added to documentation?
Are there any sample code or steps to test the changes?
Self Review done?
Any relevant screenshots, recordings or logs?
APP_HALTEDand the subsequent test sets continue to run.🧠 Semantics for PR Title & Branch Name
PR Title:
fix: continue replay after docker compose startup failureBranch Name:
fix/replay-after-startup-failure