Do not throw an error when browsertime provides null timestamps incorrectly #4399
Conversation
Codecov ReportBase: 88.55% // Head: 88.56% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4399 +/- ##
=======================================
Coverage 88.55% 88.56%
=======================================
Files 282 282
Lines 25476 25484 +8
Branches 6854 6860 +6
=======================================
+ Hits 22561 22569 +8
Misses 2708 2708
Partials 207 207
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| // Flow doesn't like null because it thinks that the timestamp can't be null. | ||
| // But this is a bug on browsertime. |
There was a problem hiding this comment.
If this can happen in the real life, I think we should actually change the type to account for this, so that our code doesn't break in unexpected ways. What do you think?
There was a problem hiding this comment.
Hm yeah. I didn't want to change the type initially because it's not the real data we expect but it's better to change it if the one we have right now can be nullable.
This change required a bunch of changes in the view progress graph though. Tested and it's not breaking the UI but it's still broken (the graph is 100% all the time without any data point). We should ideally not add these tracks if we don't have any timestamp but I prefer to do these in another PR.
| @@ -679,7 +685,12 @@ describe('visualMetrics processing', function () { | |||
| const metricProgressMarker = thread.markers.name.find( | |||
| (name) => name === metricProgressMarkerIndex | |||
| ); | |||
| expect(metricProgressMarker).toBeTruthy(); | |||
|
|
|||
| if (changeMarkerLength > 0) { | |||
There was a problem hiding this comment.
This seems to be a hack: you're using a property that's about change markers to change the behavior for progress markers.
Instead you should probably introduce a progressMarkerLength and use the same mechanism as for the change markers (filtering + use the toHaveLength matcher); or a simpler boolean hasProgressMarker possibly.
There was a problem hiding this comment.
Well I initially didn't want to do a lot of changes in the original function since it's just a test but added a hasProgressMarker now.
src/profile-logic/process-profile.js
Outdated
| @@ -1798,6 +1804,11 @@ export function processVisualMetrics( | |||
|
|
|||
| const changeMarkerName = `${metricName} Change`; | |||
| for (const { timestamp, percent } of metric) { | |||
| if (timestamp === null) { | |||
There was a problem hiding this comment.
Should we have the same check for the Progress markers above?
optional alternate change: put the check directly in addMetricMarker (and rename it possiblyAddMetricMarker) and remove from the loops.
Then I'd also remove the first condition if (metric.every((m) => m.timestamp === null)), indeed this will possibly loop over all metrics again in the case of the bug.
There was a problem hiding this comment.
I wasn't sure about adding the null checks to every addition at first because usually when the bug happens every timestamp becomes null, so I thought adding this check should fix it. But then couldn't help myself and added this just to be safe too :) But you're right, moving this to addMetricMarker is probably a safer option. Changed it.
canova
force-pushed
the
browsertime-null-timestamps
branch
from
062497a to
cccff0b
Compare
| @@ -102,7 +102,7 @@ class TrackVisualProgressCanvas extends React.PureComponent<CanvasProps> { | |||
| // Create a path for the top of the chart. This is the line that will have | |||
| // a stroke applied to it. | |||
| x = | |||
| (deviceWidth * (progressGraphData[i].timestamp - rangeStart)) / | |||
| (deviceWidth * ((progressGraphData[i].timestamp ?? 0) - rangeStart)) / | |||
There was a problem hiding this comment.
probably the right way would be to keep the previous value and use it, instead of 0. (and of course use 0 as the initial previous value).
Or maybe we could preprocess it, so that we know that in this component everything is non-null.
But this is very theorical for the case where one timestamp is missing. We don't need this at the moment, so I believe this is good enough for now.
canova
force-pushed
the
browsertime-null-timestamps
branch
from
cccff0b to
7fb305d
Compare
It looks like browsertime sometimes provides null timestamps for the visual metrics due to sitespeedio/browsertime#1746. We should be more careful and do not throw any errors in case of this. This PR fixes that.
Example profile