mozilla-frontend-infra / firefox-health-dashboard

https://firefox-health-dashboard.netlify.com
Mozilla Public License 2.0
26 stars 68 forks source link

Update speedometer chart to show many browsers #668

Closed ajinkabeer closed 4 years ago

ajinkabeer commented 4 years ago

This PR addresses the issue #652. i.e, All charts should be comparing browsers, not platforms.

I updated the code responsible for the Speedometer chart by adding three browsers in the series object keeping the platformsame. I am a bit confused if it's the right way to approach this problem. I just wanted to get the feedback instead of going the wrong path. I would like to know what you think please.

@kimberlythegeek @klahnakoski

kimberlythegeek commented 4 years ago

@ajinkabeer I'll take a look! Added Work in Progress to the title to signify to others that it is not ready for an official review 🙂

ajinkabeer commented 4 years ago

@ajinkabeer I'll take a look! Added Work in Progress to the title to signify to others that it is not ready for an official review 🙂

oh, thank you so much!

klahnakoski commented 4 years ago

@airimovici Please verify we want to show regular Speedometer or Power Speedometer: I am guessing we want just the regular Speedometer power test because we want to show performance, not power, usage.

klahnakoski commented 4 years ago

@kimberlythegeek please do a visual comparison on the preview to ensure it works and shows the "same" numbers.

klahnakoski commented 4 years ago

@ajinkabeer Looking at the preview: https://deploy-preview-668--firefox-health-dashboard.netlify.com/android I see the chart is reporting "mAh" (milliamp hours), which is not the same as the score.

Maybe Speedometer is run on only one browser?

@gmierz is that true?

ajinkabeer commented 4 years ago

@ajinkabeer Looking at the preview: https://deploy-preview-668--firefox-health-dashboard.netlify.com/android I see the chart is reporting "mAh" (milliamp hours), which is not the same as the score.

Maybe Speedometer is run on only one browser?

@gmierz is that true?

Oh, yes that was because I used the power version in the suite. I changed it to non-power version and now it's score. I have pushed the changes.

speedometer

klahnakoski commented 4 years ago

@ajinkabeer Please update the title so it is clear which platform is shown. If you also make a second chart or the other platform, it would be appreciated.

ajinkabeer commented 4 years ago

@klahnakoski

Yes, you mean Speedometer to another title? For exampleMoto G5(arm 7) Yes, I would definitely make a second chart for the other platform.

kimberlythegeek commented 4 years ago

@klahnakoski what I see so far looks good to me 👌

klahnakoski commented 4 years ago

@ajinkabeer Include both the "Speedometer" and platform name "moto G5" in the title

klahnakoski commented 4 years ago

@ajinkabeer and feel free to push changes for intermediate reviews: You can make a number of local branches and --force push them to one branch in origin if you want to move back-and-forth between experiments

Also, please run yarn test -u to update the tests (and then they will pass)

ajinkabeer commented 4 years ago

@klahnakoski Yes. Thanks for the tip.

I think the tests are failing because of the titleprop.

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@bdbc435). Click here to learn what that means. The diff coverage is 47.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #668   +/-   ##
=========================================
  Coverage          ?   49.55%           
=========================================
  Files             ?       76           
  Lines             ?     3824           
  Branches          ?      727           
=========================================
  Hits              ?     1895           
  Misses            ?     1643           
  Partials          ?      286
Impacted Files Coverage Δ
src/android/config.js 50% <37.5%> (ø)
src/android/index.jsx 88.88% <80%> (ø)

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 bdbc435...1011a59. Read the comment docs.

ajinkabeer commented 4 years ago

I managed to generate the series instead of hard-coding it following your advice. Would be cool if you could give a feedback on this approach.

@klahnakoski @kimberlythegeek

airimovici commented 4 years ago

@airimovici Please verify we want to show regular Speedometer or Power Speedometer: I am guessing we want just the regular Speedometer power test because we want to show performance, not power, usage.

From what I see on the current production (https://health.graphics/android) we have Speedometer, Speedometer CPU power usage and Unity webGL.

After the changes in this PR I would expect to have:

Am I wrong with this?

ajinkabeer commented 4 years ago

@airimovici Thanks for your review. This adds more clarity to the problem and also on what I need to achieve.

kimberlythegeek commented 4 years ago

Thanks for all the help with this @airimovici @klahnakoski !

klahnakoski commented 4 years ago

@airimovici thank you for you time reviewing these changes. Your decisions all look good so far.

ajinkabeer commented 4 years ago

@airimovici Thanks so much for your reviews. I really appreciate it. I have added the title prop as you have referenced. Thanks again.

ajinkabeer commented 4 years ago

Hello @klahnakoski @kimberlythegeek,

It would be cool if you could take a look again at the latest changes.

ajinkabeer commented 4 years ago

Oh, thanks @kimberlythegeek for the quick response. I will wait until they review it again.

kimberlythegeek commented 4 years ago

I'm not sure why it says some checks haven't completed. If I follow the Details link to the Netlify build, it appears to have completed successfully

ajinkabeer commented 4 years ago

Yes, I too have noticed it. Maybe some sort of glitch. I think it will be fixed if I push the changes again next time. :) @kimberlythegeek

kimberlythegeek commented 4 years ago

thanks for your contribution @ajinkabeer !

ajinkabeer commented 4 years ago

Thanks to you guys @kimberlythegeek @klahnakoski. I've learned a lot about the health dashboard while working on this issue. Feels good to know that it's finally merged :)