fix: include package name for duplicate bench names [#264]#330
Conversation
📝 WalkthroughWalkthroughEnhanced Go benchmark result extraction to handle multiple packages. The extractGoResult function now parses benchmark output by sections marked with "pkg:" identifiers, conditionally appending package suffixes to benchmark names when multiple packages are detected. Extensive test coverage validates single-package backward compatibility and multi-package disambiguation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #330 +/- ##
==========================================
- Coverage 89.14% 89.13% -0.02%
==========================================
Files 16 16
Lines 949 948 -1
Branches 200 201 +1
==========================================
- Hits 846 845 -1
+ Misses 103 99 -4
- Partials 0 4 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In `@src/extract.ts`:
- Around line 399-400: The chunkPairs function currently drops the last token
when given an odd-length arr; add a validation at the top of chunkPairs to check
arr.length % 2 === 0 and throw a descriptive error (including arr length or the
offending token) when odd so malformed Go output surfaces immediately; keep the
rest of the logic intact (Array.from(...)) and ensure callers can catch this
exception if necessary.
🧹 Nitpick comments (1)
src/extract.ts (1)
346-388: Normalize and dedupe package names before deciding to suffix.
hasMultiplePackagescurrently counts sections rather than unique package names, andpkgisn’t trimmed. If output concatenates multiple runs of the same package (or includes trailing whitespace), names will be suffixed even though there’s only one unique package, changing the backward‑compat behavior.🛠️ Suggested tweak (trim + unique package counting)
- const sections = output.split(/^pkg:\s+/m).map((section, index) => { + const sections = output.split(/^pkg:\s+/m).map((section, index) => { if (index === 0) return { pkg: '', lines: section.split(/\r?\n/g) }; const [pkg, ...rest] = section.split(/\r?\n/g); - return { pkg, lines: rest }; + return { pkg: pkg.trim(), lines: rest }; }); - const hasMultiplePackages = sections.filter((s) => s.pkg).length > 1; + const packageNames = new Set(sections.map((s) => s.pkg).filter(Boolean)); + const hasMultiplePackages = packageNames.size > 1;
| function chunkPairs(arr: string[]): Array<[string, string]> { | ||
| return Array.from({ length: Math.floor(arr.length / 2) }, (_, i) => [arr[i * 2], arr[i * 2 + 1]]); |
There was a problem hiding this comment.
Guard against odd-length value/unit sequences.
chunkPairs silently drops the last token when arr.length is odd, which can yield partial or misleading metrics on malformed input. Consider throwing to surface invalid Go output early.
🛡️ Suggested validation
function chunkPairs(arr: string[]): Array<[string, string]> {
+ if (arr.length % 2 !== 0) {
+ throw new Error(
+ `Malformed Go benchmark metrics (expected value/unit pairs, got ${arr.length} fields): ${arr.join(' ')}`
+ );
+ }
return Array.from({ length: Math.floor(arr.length / 2) }, (_, i) => [arr[i * 2], arr[i * 2 + 1]]);
}📝 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.
| function chunkPairs(arr: string[]): Array<[string, string]> { | |
| return Array.from({ length: Math.floor(arr.length / 2) }, (_, i) => [arr[i * 2], arr[i * 2 + 1]]); | |
| function chunkPairs(arr: string[]): Array<[string, string]> { | |
| if (arr.length % 2 !== 0) { | |
| throw new Error( | |
| `Malformed Go benchmark metrics (expected value/unit pairs, got ${arr.length} fields): ${arr.join(' ')}` | |
| ); | |
| } | |
| return Array.from({ length: Math.floor(arr.length / 2) }, (_, i) => [arr[i * 2], arr[i * 2 + 1]]); | |
| } |
🤖 Prompt for AI Agents
In `@src/extract.ts` around lines 399 - 400, The chunkPairs function currently
drops the last token when given an odd-length arr; add a validation at the top
of chunkPairs to check arr.length % 2 === 0 and throw a descriptive error
(including arr length or the offending token) when odd so malformed Go output
surfaces immediately; keep the rest of the logic intact (Array.from(...)) and
ensure callers can catch this exception if necessary.
NachoVazquez
left a comment
NachoVazquez
left a comment
There was a problem hiding this comment.
I just have a question, but it looks good otherwise!
* add test output
* add test file with a snapshot * add test output
* add test for backwards compatibility * add test output
* initial implementation * add test output
* add more granular tests
Replace index arithmetic (i * 2, i * 2 + 1) with explicit chunking using a purpose-built chunkPairs function. This makes the [value, unit] pair structure obvious through destructuring and isolates the index math.
ktrz
force-pushed
the
ktrz/264/golang-package-name-in-bench-name
branch
from
3d0659f to
c68fc65
Compare
2 participants
Problem Solved
When running Go benchmarks across multiple packages that have benchmarks with the same name (e.g.,
BenchmarkAppendMsgitemin bothmiddleware/cacheandmiddleware/csrf), the results would collide since they shared identical names. This made it impossible to track them separately over time.Solution
Benchmark names are now disambiguated by appending the package name in parentheses when multiple packages are detected in the output:
BenchmarkFoo→BenchmarkFoo (github.com/example/package1)BenchmarkFoo→BenchmarkFoo (github.com/example/package2)Backward compatible: When only a single package exists (or no
pkg:lines), names remain unchanged.Code Changes
src/extract.ts(+33/-28 lines)extractGoResult()to detect multiple packages by parsingpkg:lineschunkPairs()helper function for cleaner[value, unit]pair extractionflatMapapproachextractGoResultfor direct unit testingNew Test Coverage
test/extractGoResult.spec.ts(254 lines)Comprehensive unit tests covering:
Test fixtures added
go_fiber_duplicate_names_output.txt- Real-world example with duplicate names acrosscacheandcsrfpackagesgo_single_package_output.txt- Single package output for backward compatibility testingSummary by CodeRabbit
New Features
Tests
Chores