Conversation
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. License Issues.github/workflows/performance-tests.yml
OpenSSF Scorecard
Scanned Files
|
SummaryThe following content is AI-generated and provides a summary of the pull request: Add Performance Testing InfrastructureNew Features✨ Introduces a comprehensive performance testing suite for Changes
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a well-structured performance testing framework, but has several substantive issues: an O(n²) bug in the B2 benchmark harness (which contaminates the measurements the framework is designed to produce), a process.exit(0) that always signals success even on drift violations, and malformed Markdown table rows in the report generator. Please address those before merging.
PR Bot Information
Version: 1.19.9 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
16e478c0-2c25-11f1-899a-d218300bc287 - Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
| const sensitiveRecords = records.map((r: any) => ({ | ||
| ...r, | ||
| email: `user_${r.id}@example.com`, | ||
| phone: `+1-555-${String(records.indexOf(r)).padStart(4, "0")}`, |
There was a problem hiding this comment.
Performance Bug: records.indexOf(r) inside map is O(n²)
Array.prototype.indexOf performs a linear scan, so calling it inside records.map(...) makes this O(n²) in the number of records. At size=1000 this computes 1000 linear scans. This poisons the B2 benchmark by adding quadratic overhead that belongs to the harness, not the code under test.
Instead, use the map index parameter directly:
| const sensitiveRecords = records.map((r: any) => ({ | |
| ...r, | |
| email: `user_${r.id}@example.com`, | |
| phone: `+1-555-${String(records.indexOf(r)).padStart(4, "0")}`, | |
| const sensitiveRecords = records.map((r: any, idx: number) => ({ | |
| ...r, | |
| email: `user_${r.id}@example.com`, | |
| phone: `+1-555-${String(idx).padStart(4, "0")}`, | |
| name: `User ${r.id}`, | |
| })); | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| let baseline: BaselineData | undefined; | ||
| if (fs.existsSync(BASELINE_PATH)) { | ||
| baseline = JSON.parse(fs.readFileSync(BASELINE_PATH, "utf8")) as BaselineData; |
There was a problem hiding this comment.
Logic Error: Baseline check skipped when UPDATE_BASELINE=false but no baseline exists
When the baseline file is missing and UPDATE_BASELINE is false, this.skip() is correctly called in the first before hook. However, the second before hook (which pre-generates synthetic data) runs unconditionally before the first one's this.skip() takes effect for subsequent hooks. In Mocha, this.skip() inside a before hook does abort subsequent hooks and tests, so the ordering matters: the data-generation before hook should come first (or rely on skip propagating). This is technically safe in Mocha but the logical ordering (check prerequisites first) is already correct. No code change needed — this is an observation only.
More critically: the after hook writes reports even when the test was skipped via if (!report) return; which is correct. However, a baseline file that is present but whose JSON is malformed will throw at line 346 and crash the test with an unhandled exception rather than a graceful failure. Consider wrapping the read in a try/catch:
| let baseline: BaselineData | undefined; | |
| if (fs.existsSync(BASELINE_PATH)) { | |
| baseline = JSON.parse(fs.readFileSync(BASELINE_PATH, "utf8")) as BaselineData; | |
| let baseline: BaselineData | undefined; | |
| if (fs.existsSync(BASELINE_PATH)) { | |
| try { | |
| baseline = JSON.parse(fs.readFileSync(BASELINE_PATH, "utf8")) as BaselineData; | |
| report.baseline = baseline; | |
| } catch (e) { | |
| console.warn(` ⚠️ WARNING: Could not parse baseline file at ${BASELINE_PATH}: ${e}`); | |
| } | |
| } | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| } else { | ||
| console.log("\n[PASS] No significant drift detected."); | ||
| } | ||
| process.exit(0); |
There was a problem hiding this comment.
Bug: hasViolation is true but process.exit(0) is always called
When drift violations are detected, hasViolation is set and the [WARN] message is printed, but then process.exit(0) is unconditionally called on line 267 — which signals success to the calling process/CI. The strategy doc states this check is a "FAIL" condition. Should exit with a non-zero code when hasViolation is true, otherwise npm run test:performance:check-drift will never signal a problem to CI.
| process.exit(0); | |
| process.exit(hasViolation ? 1 : 0); | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| const perItemMs = timingsMs.map((time, index) => time / sizes[index]); | ||
| const slopes = computeSlopes(timingsMs, sizes); | ||
| const slopeRatio = slopes.length >= 2 ? slopes[slopes.length - 1] / slopes[0] : 1; |
There was a problem hiding this comment.
Logic Error: slopeRatio is 1 when there is only one slope segment
When sizes has exactly 2 entries, slopes has length 1, so slopes.length >= 2 is false and slopeRatio defaults to 1. This silently hides any scaling signal. The ratio requires at least 2 segments (3 size points) to be meaningful; with only 1 segment the ratio is mathematically undefined, not 1. The default of 1 could mask a real problem. Consider returning NaN or documenting the invariant, and guard the same expression in benchmarkAsync (line 245).
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| lines.push( | ||
| [ | ||
| result.name, | ||
| formatList(result.timingsMs, 2), | ||
| formatNumber(avgCV, 1) + "% " + cvEmoji(avgCV), | ||
| formatList(result.perItemMs, 7), | ||
| formatList(result.memoryDeltaMB, 2), | ||
| formatNumber(result.slopeRatio, 4) + " " + slopeRatioEmoji(result.slopeRatio), | ||
| formatNumber(result.r2, 4) + " " + r2Emoji(result.r2), | ||
| baselinePerItem, | ||
| baselineSlope, | ||
| baselineR2, | ||
| ].join(" | ") | ||
| ); |
There was a problem hiding this comment.
Bug: Markdown table rows use join(" | ") without leading/trailing pipes
The header row (line 95) uses proper Markdown table format with leading and trailing |, but each data row is assembled with join(" | ") which produces col1 | col2 | ... — missing the leading | and trailing |. This makes the rows invalid Markdown table syntax; most renderers will not display them as table rows.
| lines.push( | |
| [ | |
| result.name, | |
| formatList(result.timingsMs, 2), | |
| formatNumber(avgCV, 1) + "% " + cvEmoji(avgCV), | |
| formatList(result.perItemMs, 7), | |
| formatList(result.memoryDeltaMB, 2), | |
| formatNumber(result.slopeRatio, 4) + " " + slopeRatioEmoji(result.slopeRatio), | |
| formatNumber(result.r2, 4) + " " + r2Emoji(result.r2), | |
| baselinePerItem, | |
| baselineSlope, | |
| baselineR2, | |
| ].join(" | ") | |
| ); | |
| lines.push( | |
| "| " + | |
| [ | |
| result.name, | |
| formatList(result.timingsMs, 2), | |
| formatNumber(avgCV, 1) + "% " + cvEmoji(avgCV), | |
| formatList(result.perItemMs, 7), | |
| formatList(result.memoryDeltaMB, 2), | |
| formatNumber(result.slopeRatio, 4) + " " + slopeRatioEmoji(result.slopeRatio), | |
| formatNumber(result.r2, 4) + " " + r2Emoji(result.r2), | |
| baselinePerItem, | |
| baselineSlope, | |
| baselineR2, | |
| ].join(" | ") + | |
| " |" | |
| ); | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Have you...