test: add TracingProcessor tests for getLogNormalDistribution and getRiskToResponsiveness #806
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulirish
force-pushed
the
testlognormal
branch
from
798234c
to
9548f87
Compare
paulirish
changed the title
addyosmani
approved these changes
Oct 21, 2016
paulirish
force-pushed
the
testlognormal
branch
3 times, most recently
from
da80c98
to
32118da
Compare
brendankenny
requested changes
Oct 24, 2016
| TracingProcessor._riskPercentiles = oldFn; | ||
| }); | ||
|
|
||
| it('gets durations of top-level tasks', () => { |
There was a problem hiding this comment.
for this test, I think it would be better to just pull out a "get top level slices from main thread" function in tracing-processor.js and test that
| const distribution = TracingProcessor.getLogNormalDistribution(median, pODM); | ||
|
|
||
| function getPct(distribution, value) { | ||
| return distribution.computeComplementaryPercentile(value).toFixed(2); |
There was a problem hiding this comment.
these string comparisons are somewhat horrifying :) Somewhat just jumping through hoops, but maybe return Number(distribution.computeComplementaryPercentile(value).toFixed(2))? and use numbers below?
paulirish
force-pushed
the
testlognormal
branch
2 times, most recently
from
e7e3dac
to
113e6c7
Compare
brendankenny
approved these changes
Oct 28, 2016
| * Provides durations of all main thread top-level events | ||
| * @param {!traceviewer.Model} model | ||
| * @param {{traceEvents: !Array<!Object>}} trace | ||
| * @param {number=} startTime Optional start time (in ms) of range of interest. Defaults to trace start. |
There was a problem hiding this comment.
just @param {number} at this point
| * @param {!traceviewer.Model} model | ||
| * @param {{traceEvents: !Array<!Object>}} trace | ||
| * @param {number=} startTime Optional start time (in ms) of range of interest. Defaults to trace start. | ||
| * @param {number=} endTime Optional end time (in ms) of range of interest. Defaults to trace end. |
| * @param {{traceEvents: !Array<!Object>}} trace | ||
| * @param {number=} startTime Optional start time (in ms) of range of interest. Defaults to trace start. | ||
| * @param {number=} endTime Optional end time (in ms) of range of interest. Defaults to trace end. | ||
| * @return {!Object>} |
There was a problem hiding this comment.
@return {{durations: !Array<number>, clippedLength: number}} FWIW :)
|
otherwise LGTM |
paulirish
force-pushed
the
testlognormal
branch
from
113e6c7
to
70a2d94
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We didn't have test coverage for this method. The assertions test the shape of the curve and that our percentiles are correct.
https://www.desmos.com/calculator/0izses21rf