feat: rewrite load test suite#4099
feat: rewrite load test suite#4099MasterPtato wants to merge 1 commit into02-02-chore_guard_centralize_cors_configfrom
Conversation
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4099
|
🚅 Deployed to the rivet-pr-4099 environment in rivet-frontend
|
Code Review - PR #4099: Rewrite Load Test SuiteThis PR migrates the load test infrastructure from a custom TypeScript implementation to k6, consolidating testing under ✅ StrengthsArchitecture & Design
Developer Experience
🔍 Critical Issues1. Incorrect log message in SIGINT handler (test-runner/src/index.ts:127) 2. Hardcoded localhost URL (test-runner/src/index.ts:138) 3. Empty function body formatting (test-runner/src/index.ts:187) 🐛 Code Quality Issues4. Missing radix parameter (run.ts:102, 110) 5. WebSocket error handling in chatty mode (actor-lifecycle.js:583-605) 6. K6 script uses JavaScript (actor-lifecycle.js) 7. Memory leak potential (test-runner/src/index.ts:30) 8. No comment on blocking sleep (actor-lifecycle.js:258) 9. Health check timeouts (run.ts:314-368) 10. Variable naming clarity (run.ts:123) 11. Counter race condition (actor-lifecycle.js:123, 392) 12. Success check needs comment (actor-lifecycle.js:232-241) 📝 Documentation Issues13. Inconsistent script names (run.ts:209) 14. Missing variation explanation (QUICKSTART.md) 15. Incorrect path (QUICKSTART.md:38) 💡 Suggestions16. No input validation: Consider validating Future Improvements
📋 Required Changes
✨ AssessmentHigh-quality refactor that significantly improves load testing. K6 migration is well-executed, developer experience is excellent, code is mostly clean. Issues are minor and easily addressable. Recommendation: Approve with requested changes. Great work on comprehensive help text, validation, and documentation! 🚀 |
MasterPtato
force-pushed
the
02-02-chore_guard_centralize_cors_config
branch
from
1e902d5 to
cb28ad8
Compare
MasterPtato
force-pushed
the
02-02-feat_rewrite_load_test_suite
branch
from
e6b10fe to
07288c0
Compare
Pull Request Review: Load Test Suite RewriteThis PR rewrites the load testing infrastructure with a unified k6-based solution. Strengths
Critical Issues1. SIGTERM Handler Bug (test-runner/src/index.ts:53) 2. Runner Variable Scope (test-runner/src/index.ts:23) 3. File Naming Inconsistency 4. Missing Dependency (scripts/tests/package.json) 5. Wrong Token Variable (run.ts:322) 6. Race Condition Risk (actor-lifecycle.js:1842-1896) RecommendationsHigh Priority:
Medium Priority:
Overall: Well-designed rewrite that improves testing. Address the bugs above before merging. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| process.exit(0); | ||
| }); | ||
| process.on("SIGINT", async () => { | ||
| getLogger().debug("received SIGTERM, force exiting in 3s"); |
There was a problem hiding this comment.
Copy-paste error in SIGINT handler log message. It says "received SIGTERM" but should say "received SIGINT".
getLogger().debug("received SIGINT, force exiting in 3s");| getLogger().debug("received SIGTERM, force exiting in 3s"); | |
| getLogger().debug("received SIGINT, force exiting in 3s"); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| process.on("SIGTERM", async () => { | ||
| getLogger().debug("received SIGTERM, force exiting in 3s"); | ||
|
|
||
| await new Promise(res => setTimeout(res, 3000)); | ||
|
|
||
| process.exit(0); | ||
| }); | ||
| process.on("SIGINT", async () => { | ||
| getLogger().debug("received SIGTERM, force exiting in 3s"); | ||
|
|
||
| await new Promise(res => setTimeout(res, 3000)); | ||
|
|
||
| process.exit(0); | ||
| }); |
There was a problem hiding this comment.
These signal handlers contain unhandled promises. Since Node.js event handlers don't wait for async functions to complete, these create 'floating promises'. Fix by either removing the async keyword and using setTimeout with a callback, or add .catch() handlers to properly handle errors.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| runnerStarted.resolve(runner); | ||
| }, | ||
| onDisconnected: () => {}, | ||
| onDisconnected: () => { }, |
There was a problem hiding this comment.
Inconsistent formatting with extra space after the empty function body. This should be formatted as 'onDisconnected: () => {}' to match the code style used elsewhere.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
MasterPtato
force-pushed
the
02-02-feat_rewrite_load_test_suite
branch
from
07288c0 to
1728003
Compare
Pull Request Review: Load Test Suite RewriteSummaryThis PR replaces the previous TypeScript-based load testing suite with a new k6-based implementation. The change moves from custom test code to industry-standard load testing tooling, providing better metrics, configuration options, and test variations. ✅ Strengths
|
MasterPtato
force-pushed
the
02-02-chore_guard_centralize_cors_config
branch
from
cb28ad8 to
d3bae78
Compare
MasterPtato
force-pushed
the
02-02-feat_rewrite_load_test_suite
branch
from
1728003 to
349575f
Compare
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4099
Merge activity
|
# Description Please include a summary of the changes and the related issue. Please also include relevant motivation and context. ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes
Pull Request Review: Rewrite Load Test SuiteThis PR rewrites the load test suite from engine/tests/load/ to scripts/tests/load-test/ with k6-based testing. Key Strengths
Critical Issues
High Priority
Medium Priority
RecommendationsMust fix: Issues 1-3 SummarySolid rewrite improving load testing infrastructure. Main concerns are error handling edge cases, input validation, and resource leaks. Recommend approve with requested changes for critical and high priority issues. The code demonstrates good engineering practices but needs production-readiness improvements before merging. |
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: