Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support the case where some visual metrics properties are missing #3967

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Mar 31, 2022

Fixes #3966

production
deploy preview

The data itself in this profile seems to have an error (all "timestamp" properties are undefined) and that's why the resulting graph looks wrong.

I didn't add new tests, because I thought that the type changes are good enough in this case. Tell me what you think :-)

@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #3967 (9b3b206) into main (5dccf3c) will increase coverage by 0.00%.
The diff coverage is 78.57%.

❗ Current head 9b3b206 differs from pull request most recent head b5face8. Consider uploading reports for the commit b5face8 to get more accurate results

@@           Coverage Diff           @@
##             main    #3967   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files         279      279           
  Lines       23934    23926    -8     
  Branches     6325     6322    -3     
=======================================
- Hits        20885    20879    -6     
+ Misses       2809     2807    -2     
  Partials      240      240           
Impacted Files Coverage Δ
src/components/timeline/GlobalTrack.js 72.26% <0.00%> (ø)
src/profile-logic/tracks.js 83.53% <100.00%> (+0.20%) ⬆️
src/selectors/profile.js 96.90% <100.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dccf3c...b5face8. Read the comment docs.

@julienw julienw force-pushed the 3966-add-visual-progress-tracks-if-we-have-data branch 2 times, most recently from e619edf to 9b3b206 Compare March 31, 2022 13:56
@julienw julienw requested a review from canova March 31, 2022 14:25
@julienw julienw force-pushed the 3966-add-visual-progress-tracks-if-we-have-data branch from 9b3b206 to 227beda Compare March 31, 2022 14:28
@julienw julienw force-pushed the 3966-add-visual-progress-tracks-if-we-have-data branch from 227beda to ab64434 Compare March 31, 2022 14:30
Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!
It's indeed interesting that the timestamps are null in the profile.

@@ -685,30 +685,30 @@ export type ProgressGraphData = {|
* along with their visual progression. These properties find their way into the gecko profile through running
* browsertime, which is a tool that lets you collect performance metrics of your website.
* Browsertime provides the option of generating a gecko profile, which then embeds these visual metrics
* into the geckoprofile. More information about browsertime can be found through https://github.com/mozilla/browsertime.
* into the geckoprofile. More information about browsertime can be found through https://github.com/sitespeedio/browsertime.
* These values are generated only when browsertime is run with --visualMetrics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for additional information!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geckoprofiler trace with Visual metrics enabled from Browsertime gets an error
2 participants