Configure k6 quote CNC test for 500 RPS#534
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded performance test documentation and refactored the k6 test script to use scenario-based load profiling with constant-arrival-rate configuration. Updated URL composition, API base URL handling, HTTP thresholds, and exported function naming from default to named export. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes The changes are primarily straightforward refactoring and configuration updates. While the k6 script has multiple modifications, they follow consistent patterns: URL parameter substitution, scenario config migration, and export signature changes. No complex new logic or intricate interdependencies are introduced. The new README is trivial documentation. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #534 (comment) The following files were modified: * `tests/perf/k6-quote-cnc.js`
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/perf/k6-quote-cnc.js (1)
37-75: Add error handling for resource creation failures.If material or module creation fails during setup,
materialIdormoduleIdwill beundefined, causing the entire performance test to fail unclearly. Consider adding explicit error handling:check(materialRes, { 'material created': (r) => r.status === 200 }); const materialId = materialRes.json('id'); + if (!materialId) { + throw new Error('Failed to create material'); + } // ... check(moduleRes, { 'module created': (r) => r.status === 200 }); const moduleId = moduleRes.json('id'); + if (!moduleId) { + throw new Error('Failed to create module'); + }This provides clearer failure messages and prevents the test from running with invalid data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/perf/README.md(1 hunks)tests/perf/k6-quote-cnc.js(5 hunks)
🔇 Additional comments (5)
tests/perf/README.md (1)
1-41: Documentation looks comprehensive and accurate.The README provides clear guidance on running the performance test. The documented defaults, environment variables, and SLO thresholds align correctly with the script implementation. The example command is well-structured and shows proper usage.
Since the PR objectives note that "the performance script configuration change has not been run," ensure this documentation is validated against actual test runs in staging before merging.
tests/perf/k6-quote-cnc.js (4)
4-20: Verify VU sizing for the target environment.The constant-arrival-rate configuration correctly targets 500 RPS with a VU pool of 150-600. However, each iteration includes a 0.5s sleep (line 186), plus the time for 2 API requests and optional frontend checks. At 500 RPS, this could require 300-400+ VUs depending on actual response times.
The default
maxVUs: 600should be sufficient for the documented latency SLO (p95 < 300ms), but if staging/production exhibits higher latency or increased request duration, k6 may fail to maintain the rate and emit warnings like "not enough VUs."Run an initial test in the target environment and monitor k6's output for warnings about VU exhaustion. If warnings appear, increase
MAX_VUSvia the environment variable.
23-23: Confirm the 2x increase in latency SLO is intentional.The threshold has been relaxed from
p(95)<150top(95)<300ms. This doubles the acceptable 95th percentile latency.Ensure this change aligns with:
- Baseline performance measurements at 500 RPS
- Business requirements for acceptable user experience
- Any architectural changes that would justify higher latency
If baseline measurements aren't available, consider running an initial test to establish actual p95 latency before finalizing this threshold.
27-35: URL construction logic is correct.The implementation properly handles both explicit base URLs and component-based construction with sensible defaults. The conditional check for
FRONTEND_HOST(line 33) correctly prevents constructing invalid URLs when frontend variables are not provided.
120-120: Function export structure is correct.The scenario function is properly exported both as a named export (line 120) and as the default export (line 189), and correctly referenced in the scenario configuration's
execproperty (line 18).Also applies to: 189-189
| 'Content-Type': 'application/json', | ||
| Authorization: 'Bearer ' + (__ENV.API_WRITE_TOKEN || 'test-write-token'), | ||
| }; | ||
| const moduleId = data?.moduleId || createMaterialAndModule().moduleId; |
There was a problem hiding this comment.
Fallback resource creation could overwhelm the system.
The fallback createMaterialAndModule() on line 125 would execute for every iteration if setup() fails or returns invalid data. At 500 RPS over 5 minutes, this could create 150,000 materials and modules, overwhelming the database and invalidating test results.
Consider removing the fallback or making setup failures explicit:
- const moduleId = data?.moduleId || createMaterialAndModule().moduleId;
+ if (!data?.moduleId) {
+ throw new Error('Setup failed - moduleId not available');
+ }
+ const moduleId = data.moduleId;This ensures the test fails fast with a clear error message rather than attempting resource creation in the hot path.
📝 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.
| const moduleId = data?.moduleId || createMaterialAndModule().moduleId; | |
| if (!data?.moduleId) { | |
| throw new Error('Setup failed - moduleId not available'); | |
| } | |
| const moduleId = data.moduleId; |
🤖 Prompt for AI Agents
In tests/perf/k6-quote-cnc.js around line 125, the inline fallback
createMaterialAndModule() is invoked in the hot path (const moduleId =
data?.moduleId || createMaterialAndModule().moduleId), which would create
resources on every iteration if setup() failed; remove the fallback call and
instead make setup failures explicit by throwing or returning a clear error when
data is missing, so the test fails fast; update the code to validate that
data.moduleId (and other required setup values) exist before the load loop and
abort with a descriptive error/log if they do not, rather than creating
resources during iterations.
) Docstrings generation was requested by @shayancoin. * #534 (comment) The following files were modified: * `tests/perf/k6-quote-cnc.js` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f8089fe9d08330853b68bccc3327ec
Summary by CodeRabbit
Documentation
Tests