fix: time units are not normalized (issue #122) #318
Conversation
* 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
3168d8d to
9bb96a6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #318 +/- ##
==========================================
+ Coverage 88.12% 89.07% +0.95%
==========================================
Files 10 15 +5
Lines 884 943 +59
Branches 187 199 +12
==========================================
+ Hits 779 840 +61
+ Misses 101 99 -2
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } else { | ||
| const suites = entries[benchName]; | ||
| // Get last suite which has different commit ID for alert comment | ||
| for (const e of suites.slice().reverse()) { |
There was a problem hiding this comment.
nit: alternatively you could do something like:
prevBench = suits.slice().reverse().find(e => e.commit.id !== benchEntry.commit.id)it is exactly the same but less verbose
| const currentRange = currenBenchResult.range; | ||
| const currentRangeInfo = extractRangeInfo(currentRange); | ||
|
|
||
| const normalizedValue = normalizeUnit(prevUnit, currentUnit, currenBenchResult.value); |
There was a problem hiding this comment.
suggestion: the naming here could be better, you are normalizing the value by calling a function named normalizeUnit and one line below you are creating a normalizedUnit variable.
Maybe a better name for the function is normalizeValueByUnit or something similar.
| const UNIT_CONVERSION_MULTIPLIER = 1000; | ||
| const TIME_UNITS = ['s', 'ms', 'us', 'ns']; | ||
| const ITER_UNITS = TIME_UNITS.map((unit) => `${unit}/iter`); | ||
| const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`); |
There was a problem hiding this comment.
giga-nitpick: I noticed you have used two different cloning techniques on the same PR, maybe is better for consistency to pick one.
- [...previousValue]
- previousValue.slice()
| added.benches; // ? | ||
| expectedAdded.benches; // ? |
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