fix: time units are not normalized (issue #122)#318
Conversation
ktrz
changed the title
* extract addBenchmarkEntry function
* normalize new entry values to match last entry units
* normalize ops per time unit
* handle <time_unit>/iter unit * add tests for write function
ktrz
force-pushed
the
fix/122-time-units-not-normalized
branch
from
3168d8d to
9bb96a6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #318 +/- ##
==========================================
+ Coverage 88.12% 89.13% +1.01%
==========================================
Files 10 16 +6
Lines 884 948 +64
Branches 187 198 +11
==========================================
+ Hits 779 845 +66
- Misses 101 103 +2
+ Partials 4 0 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NachoVazquez
left a comment
NachoVazquez
left a comment
There was a problem hiding this comment.
I'm not sure if you want to act on my comments. The PR looks great in general, and I was able to quickly grasp the context without knowing the tool too well. My comments are more style than behavior-oriented, so take it or leave it.
Good to go in any case. 🚀
* use spread operator consistently for cloning the array * rename normalizeUnit to normalizeValueByUnit * removed Wallaby debugging statements
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds unit canonicalization and conversion utilities, parses range prefixes, normalizes benchmark results against prior runs, appends normalized entries to suites with max-items truncation, and updates write flow and tests to use normalized results for graphs/alerts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Action as GitHub Action
participant Write as write.ts
participant Add as addBenchmarkEntry.ts
participant Norm as normalizeBenchmark.ts
participant ResultNorm as normalizeBenchmarkResult.ts
participant Utils as canonicalize/normalizeValue/extractRangeInfo
participant Store as data.json
Action->>Write: writeBenchmark(config, currentBench, entries, maxItems)
Write->>Add: addBenchmarkEntry(benchName, currentBench, entries, maxItems)
Add->>Store: read entries[benchName]
alt suite exists
Add->>Add: find prevBench (last different commit)
else suite missing
Add->>Store: create new suite
end
Add->>Norm: normalizeBenchmark(prevBench, currentBench)
Norm->>ResultNorm: normalizeBenchmarkResult(prevResult, currResult) [per bench]
ResultNorm->>Utils: canonicalize / normalizeValueByUnit / extractRangeInfo
Utils-->>ResultNorm: normalized value/unit/range
ResultNorm-->>Norm: normalized bench results
Norm-->>Add: normalizedCurrentBench
Add->>Store: append normalizedCurrentBench
opt maxItems enforced
Add->>Store: truncate oldest items
end
Add-->>Write: { prevBench, normalizedCurrentBench }
Write->>Write: prepare comments/summaries/alerts using normalizedCurrentBench
Write-->>Action: results and side-effects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks, @NachoVazquez, for the feedback. I applied most of it! Shipping in a moment 🚀 |
![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
♻️ Duplicate comments (1)
src/addBenchmarkEntry.ts (1)
20-27: Use a more concise previous-bench lookup.Equivalent and more readable; previously suggested in past reviews.
- // Get the last suite which has different commit ID for alert comment - for (const e of [...suites].reverse()) { - if (e.commit.id !== benchEntry.commit.id) { - prevBench = e; - break; - } - } + // Get the last suite which has different commit ID for alert comment + prevBench = suites.slice().reverse().find((e) => e.commit.id !== benchEntry.commit.id) ?? null;
🧹 Nitpick comments (11)
test/extractRangeInfo.spec.ts (1)
24-30: Add coverage for single-digit, decimals, exponent, and surrounding whitespace.These cases ensure robustness and prevent regressions after tightening the regex.
it('should NOT extract range with invalid number', () => { expect(extractRangeInfo('± boo')).toBeUndefined(); expect(extractRangeInfo('+- boo')).toBeUndefined(); expect(extractRangeInfo('± 1boo')).toBeUndefined(); expect(extractRangeInfo('+- 1boo')).toBeUndefined(); }); + it('should extract single-digit integer', () => { + expect(extractRangeInfo('±2')).toEqual({ value: 2, prefix: '±' }); + }); + it('should extract decimal and preserve prefix space', () => { + expect(extractRangeInfo('± 0.5')).toEqual({ value: 0.5, prefix: '± ' }); + }); + it('should extract scientific notation', () => { + expect(extractRangeInfo('+-1e-3')).toEqual({ value: 1e-3, prefix: '+-' }); + }); + it('should tolerate surrounding whitespace', () => { + expect(extractRangeInfo(' ±20 ')).toEqual({ value: 20, prefix: '±' }); + });src/normalizeValueByUnit.ts (1)
1-14: Support microsecond symbol variants and stray whitespace/case in unit strings.Many toolchains output µs/μs. Canonicalize inputs before indexing so normalization doesn’t silently no-op.
-export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { +export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { for (const units of SUPPORTED_UNITS) { - const prevUnitIndex = units.indexOf(prevUnit); - const currentUnitIndex = units.indexOf(currentUnit); + const prev = canonicalizeUnit(prevUnit); + const curr = canonicalizeUnit(currentUnit); + const prevUnitIndex = units.indexOf(prev); + const currentUnitIndex = units.indexOf(curr); if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { const unitDiff = prevUnitIndex - currentUnitIndex; return value * UNIT_CONVERSION_MULTIPLIER ** unitDiff; } } return value; }Add this helper in the module:
function canonicalizeUnit(u: string): string { // normalize case/space and micro symbol in any context (e.g., "µs/iter", "ops/µs") return u.trim().toLowerCase().replace(/µs|μs/g, 'us'); }Please confirm whether your inputs can include "µs/μs" or mixed-case (e.g., "MS"). If yes, this change and accompanying tests are warranted.
test/normalizeValueByUnit.spec.ts (1)
40-42: Add tests for microsecond symbol aliases and whitespace/case tolerance.Validates the canonicalization change and real-world inputs.
it('NOT normalize when new unit is not supported', () => { expect(normalizeValueByUnit('unknown1', 'unknown2', 12)).toBe(12); }); + it('handles microsecond symbol variants', () => { + expect(normalizeValueByUnit('ms', 'µs', 12)).toBe(0.012); + expect(normalizeValueByUnit('us', 'μs', 12)).toBe(12); + expect(normalizeValueByUnit('ops/µs', 'ops/s', 12_000_000)).toBe(12); + }); + it('tolerates surrounding whitespace and case', () => { + expect(normalizeValueByUnit(' MS ', ' S ', 12)).toBe(0.012); + });src/normalizeBenchmark.ts (1)
9-19: Avoid O(n²) lookup by pre-indexing previous benches.Small win, but cheap and clearer.
- return { - ...currentBench, - benches: currentBench.benches - .map((currentBenchResult) => ({ - currentBenchResult, - prevBenchResult: prevBench.benches.find((result) => result.name === currentBenchResult.name), - })) - .map(({ currentBenchResult, prevBenchResult }) => - normalizeBenchmarkResult(prevBenchResult, currentBenchResult), - ), - }; + const prevByName = new Map(prevBench.benches.map((b) => [b.name, b])); + return { + ...currentBench, + benches: currentBench.benches.map((curr) => + normalizeBenchmarkResult(prevByName.get(curr.name), curr), + ), + };test/normalizeBenchmarkResult.spec.ts (2)
120-127: Keep the range marker consistent within the "+-" section.This block is under "with range (+-)" but uses "±" in inputs/expected. It still passes (parser accepts both), but for consistency/readability, consider using "+-" here as well.
- { name: 'Bench', value: 900, unit: 'unknown1', range: '± 20' }, - { name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' }, + { name: 'Bench', value: 900, unit: 'unknown1', range: '+- 20' }, + { name: 'Bench', value: 1.01, unit: 'unknown2', range: '+- 0.1' },
3-4: Add tests for ops/ normalization (e.g., ops/ms ↔ ops/s).PR claims support for ops/, but no assertions cover it. Please add cases proving values scale correctly when previous/current units are ops/ns|us|ms|s.
Example to add:
it('normalizes ops/<time> to previous unit', () => { // prev uses ops/ms, current uses ops/s → current should scale by 1/1000 to ops/ms expect( normalizeBenchmarkResult( { name: 'Bench', value: 500, unit: 'ops/ms', range: '± 50' }, { name: 'Bench', value: 600000, unit: 'ops/s', range: '± 60000' }, ), ).toEqual({ name: 'Bench', value: 600, unit: 'ops/ms', range: '± 60' }); });Also applies to: 83-84, 130-131
src/normalizeBenchmarkResult.ts (3)
7-8: Typo in parameter name: currenBenchResult → currentBenchResult.Purely a readability/consistency fix; avoids confusion in future diffs and reviews.
-export function normalizeBenchmarkResult( - prevBenchResult: BenchmarkResult | undefined | null, - currenBenchResult: BenchmarkResult, -): BenchmarkResult { +export function normalizeBenchmarkResult( + prevBenchResult: BenchmarkResult | undefined | null, + currentBenchResult: BenchmarkResult, +): BenchmarkResult { - if (!prevBenchResult) { - return currenBenchResult; + if (!prevBenchResult) { + return currentBenchResult; } - const currentUnit = currenBenchResult.unit; - const currentRange = currenBenchResult.range; + const currentUnit = currentBenchResult.unit; + const currentRange = currentBenchResult.range; const currentRangeInfo = extractRangeInfo(currentRange); - const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); - const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value); + const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit; return { - ...currenBenchResult, + ...currentBenchResult, value: normalizedValue, unit: normalizedUnit, range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, }; }
20-25: Consider formatting normalized range values (and possibly values) to stable precision.Stringifying raw floats can produce long decimals (e.g., 0.30000000000000004). A small formatter (e.g., trim to max 6 significant digits) would make outputs cleaner and comments more readable.
Also applies to: 31-32
18-25: Guard: normalize range only when value was normalized.If unit conversion isn’t applicable (unsupported units) but extractRangeInfo succeeds, we still rebuild range. Harmless, but you can skip rebuilding when normalizedUnit === currentUnit for slightly simpler behavior.
- const normalizedRangeInfo = currentRangeInfo + const normalizedRangeInfo = currentRangeInfo && normalizedUnit !== currentUnit ? { prefix: currentRangeInfo.prefix, value: normalizeValueByUnit(prevUnit, currentUnit, currentRangeInfo.value), } : undefined;src/addBenchmarkEntry.ts (1)
15-19: Minor: also normalize when creating the first suite (no behavior change).Calling normalizeBenchmark with null prev keeps behavior identical but unifies code paths (always push normalizedCurrentBench).
- if (entries[benchName] === undefined) { - entries[benchName] = [benchEntry]; + if (entries[benchName] === undefined) { + normalizedCurrentBench = normalizeBenchmark(null, benchEntry); + entries[benchName] = [normalizedCurrentBench]; core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);test/write.spec.ts (1)
420-466: Add ops/ normalization scenarios.Please add cases where previous is ops/ms (or ops/us) and current is ops/s (and vice versa) to validate conversion direction and range scaling in alerts and failure paths.
Also applies to: 1291-1313
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/addBenchmarkEntry.ts(1 hunks)src/extractRangeInfo.ts(1 hunks)src/normalizeBenchmark.ts(1 hunks)src/normalizeBenchmarkResult.ts(1 hunks)src/normalizeValueByUnit.ts(1 hunks)src/write.ts(8 hunks)test/extractRangeInfo.spec.ts(1 hunks)test/normalizeBenchmarkResult.spec.ts(1 hunks)test/normalizeValueByUnit.spec.ts(1 hunks)test/write.spec.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/normalizeBenchmark.ts (1)
src/normalizeBenchmarkResult.ts (1)
normalizeBenchmarkResult(5-33)
test/extractRangeInfo.spec.ts (1)
src/extractRangeInfo.ts (1)
extractRangeInfo(1-24)
src/addBenchmarkEntry.ts (2)
src/write.ts (1)
BenchmarkSuites(14-14)src/normalizeBenchmark.ts (1)
normalizeBenchmark(4-20)
src/normalizeBenchmarkResult.ts (3)
src/extract.ts (1)
BenchmarkResult(6-12)src/extractRangeInfo.ts (1)
extractRangeInfo(1-24)src/normalizeValueByUnit.ts (1)
normalizeValueByUnit(1-14)
test/normalizeValueByUnit.spec.ts (1)
src/normalizeValueByUnit.ts (1)
normalizeValueByUnit(1-14)
src/write.ts (3)
src/extract.ts (1)
Benchmark(38-43)src/addBenchmarkEntry.ts (1)
addBenchmarkEntry(6-41)src/config.ts (1)
Config(7-28)
test/normalizeBenchmarkResult.spec.ts (1)
src/normalizeBenchmarkResult.ts (1)
normalizeBenchmarkResult(5-33)
test/write.spec.ts (2)
src/extract.ts (1)
Benchmark(38-43)src/write.ts (1)
writeBenchmark(513-528)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Criterion.rs benchmark example
- GitHub Check: Run Criterion.rs benchmark example - github.com/benchmark-action/github-action-benchmark-results
🔇 Additional comments (11)
src/extractRangeInfo.ts (1)
1-4: LGTM overall.Straightforward parsing with clear return type; undefined paths handled cleanly.
Also applies to: 20-23
src/normalizeValueByUnit.ts (1)
16-20: Constants read well and cover the required families.The reversed ordering for ops/ is correct for dimensionality.
src/normalizeBenchmark.ts (1)
4-7: LGTM on control flow.Null guard and immutability of the returned structure look good.
test/normalizeBenchmarkResult.spec.ts (1)
3-176: Great coverage of unit normalization across s/ms/us/ns and with/without ranges.Scenarios are clear and assert the exact expected values/units. Nice work.
src/addBenchmarkEntry.ts (1)
12-31: Helper extraction looks good; normalization happens before append.Flow is clear and returns both prevBench and normalizedCurrentBench for downstream consumers.
src/write.ts (3)
320-335: Good: addBenchmarkToDataJson now returns prev and normalized current; write path updated accordingly.This centralizes normalization and keeps write* functions slim.
152-159: Comment/alert rendering remains readable with normalized units.strVal and table generation work well with normalizedCurrentBench; messages in tests align with expected output.
Also applies to: 168-201, 204-241
402-406: Handlers use normalizedCurrentBench handleComment, handleSummary, and handleAlert already receive normalizedCurrentBench as the “current” suite; no changes needed.test/write.spec.ts (3)
209-276: Nice addition of expectedAdded to assert normalized append results.This makes normalization effects explicit and guards against regressions.
343-379: Great: multi-benchmark suite case covers mixed-unit normalization.Catches cross-bench consistency issues.
828-906: Test harness refactor reads better (destructuring + expectedAdded fallback).Concise and easier to follow.
* fix handling of single digit range
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/canonicalizeUnit.ts (1)
1-3: Nit: simplify the microseconds regexUse a character class; it’s shorter and equivalent.
-export function canonicalizeUnit(u: string): string { - return u.trim().toLowerCase().replace(/µs|μs/g, 'us'); -} +export function canonicalizeUnit(u: string): string { + return u.trim().toLowerCase().replace(/[µμ]s/g, 'us'); +}src/normalizeValueByUnit.ts (1)
3-15: Minor perf/readability: canonicalize once, outside the loopNo behavior change; avoids redundant work per group.
-export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { - for (const units of SUPPORTED_UNITS) { - const prev = canonicalizeUnit(prevUnit); - const current = canonicalizeUnit(currentUnit); - const prevUnitIndex = units.indexOf(prev); - const currentUnitIndex = units.indexOf(current); +export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { + const prev = canonicalizeUnit(prevUnit); + const current = canonicalizeUnit(currentUnit); + for (const units of SUPPORTED_UNITS) { + const prevUnitIndex = units.indexOf(prev); + const currentUnitIndex = units.indexOf(current);src/normalizeBenchmarkResult.ts (1)
5-11: Typo: rename currenBenchResult → currentBenchResultPurely cosmetic; improves readability.
export function normalizeBenchmarkResult( prevBenchResult: BenchmarkResult | undefined | null, - currenBenchResult: BenchmarkResult, + currentBenchResult: BenchmarkResult, ): BenchmarkResult { if (!prevBenchResult) { - return currenBenchResult; + return currentBenchResult; } const prevUnit = prevBenchResult.unit; - const currentUnit = currenBenchResult.unit; - const currentRange = currenBenchResult.range; + const currentUnit = currentBenchResult.unit; + const currentRange = currentBenchResult.range; @@ - const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); - const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value); + const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit; @@ return { - ...currenBenchResult, + ...currentBenchResult, value: normalizedValue, unit: normalizedUnit, range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, };Also applies to: 13-15, 18-19, 27-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/canonicalizeUnit.ts(1 hunks)src/normalizeBenchmarkResult.ts(1 hunks)src/normalizeValueByUnit.ts(1 hunks)test/normalizeValueByUnit.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/normalizeValueByUnit.spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/normalizeBenchmarkResult.ts (3)
src/extract.ts (1)
BenchmarkResult(6-12)src/extractRangeInfo.ts (1)
extractRangeInfo(1-24)src/normalizeValueByUnit.ts (1)
normalizeValueByUnit(3-18)
src/normalizeValueByUnit.ts (1)
src/canonicalizeUnit.ts (1)
canonicalizeUnit(1-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Criterion.rs benchmark example - github.com/benchmark-action/github-action-benchmark-results
- GitHub Check: Run JMH Java Benchmark Framework example
- GitHub Check: Run Julia benchmark example
- GitHub Check: Run Criterion.rs benchmark example
🔇 Additional comments (1)
src/normalizeValueByUnit.ts (1)
10-14: LGTM: conversion math and unit group ordering are correctThe exponent sign and reversed ops/time ordering produce correct scaling across s↔ms↔us↔ns and ops/s↔ops/ns.
src/normalizeBenchmarkResult.ts
Outdated
| const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); | ||
| const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; | ||
| const normalizedRangeInfo = currentRangeInfo |
There was a problem hiding this comment.
Bug: unit may not be normalized when value is 0
When currentUnit ≠ prevUnit but value is 0, normalizedValue equals the original (0), so unit isn’t switched to prevUnit. This breaks the “match the last unit” guarantee.
Apply:
- const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value);
- const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit;
+ const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value);
+ const adoptPrevUnit =
+ prevUnit !== currentUnit && normalizeValueByUnit(prevUnit, currentUnit, 1) !== 1;
+ const normalizedUnit = adoptPrevUnit ? prevUnit : currentUnit;I can also add a unit test covering 0 with differing units if you’d like.
📝 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 normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); | |
| const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; | |
| const normalizedRangeInfo = currentRangeInfo | |
| const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); | |
| const adoptPrevUnit = | |
| prevUnit !== currentUnit && normalizeValueByUnit(prevUnit, currentUnit, 1) !== 1; | |
| const normalizedUnit = adoptPrevUnit ? prevUnit : currentUnit; | |
| const normalizedRangeInfo = currentRangeInfo |
🤖 Prompt for AI Agents
In src/normalizeBenchmarkResult.ts around lines 18 to 20, the code sets
normalizedUnit by comparing the original value to the normalizedValue, which
fails when value === 0 because normalization returns 0 and the unit won’t switch
to prevUnit; change the unit-selection logic to explicitly prefer prevUnit when
units differ and the original value is zero (e.g., if currentUnit !== prevUnit
&& currenBenchResult.value === 0 then set normalizedUnit = prevUnit), otherwise
keep the existing comparison (set prevUnit when the normalizedValue changed,
else currentUnit).
2 participants
This PR fixes an issue when the new benchmark results are using a different time unit than the previous one. This happens specifically when the benchmark values are very close to the boundary between 2 units, ex.
990 ns/iterand1.1 us/iter. In that case, we need to normalize the newly coming results to the previously used unit so that the values are comparable.<time>,<time>/iterandops/<time>unitsfixes #122