mozilla / perfcompare

Improved Performance Comparison Tool
Mozilla Public License 2.0
39 stars 93 forks source link

Use replicate values when available for graphs. #710

Closed gmierz closed 1 month ago

gmierz commented 2 months ago

This patch changes the graphs to use the replicates when they are available. There are certain situations where this doesn't occur (such as for summary-level values). Only try runs, and mozilla-central speedometer3 test runs have these replicates available to them.

It depends on the changes in this patch for the Treeherder API: https://github.com/mozilla/treeherder/pull/8165

netlify[bot] commented 2 months ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit 42452eb4b7c841602e338e35a3a79eb14dc05166
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/66c634252910830008855c4e
Deploy Preview https://deploy-preview-710--mozilla-perfcompare.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

gmierz commented 2 months ago

@julienw not entirely sure how this should be implemented, but it works for me when I have tests with, and without replicates.

gmierz commented 2 months ago

To test locally, you'll need to setup a local DB with a couple try runs with performance numbers ingested, and apply the patch from here https://github.com/mozilla/treeherder/pull/8165. Then change the API URL in treeherder.ts to point to the localhost:PORT running Treeherder.

gmierz commented 2 months ago

@julienw I think I'm going to need to change the tests to include the new replicates fields in either case, unless you see something different that we could do?

julienw commented 2 months ago

@julienw I think I'm going to need to change the tests to include the new replicates fields in either case, unless you see something different that we could do?

Yes that's it!

That's why I suggested yesterday that we could make the parameter optional so that it would be absent when there's no replicates. But if it's not optional then we need to add it in every test data.

I see that the properties are missing from the type CompareResultItem in src/types/state.ts currently, I believe it was lost during our git manipulations yesterday. Then typing npm run tsc should make it obvious where it's missing in tests. It will probably be in src/__tests__/utils/fixtures.ts

It would be good to add a test covering replicates too. This could happen in the ResultsView.test.tsx test by copying part of Should display Base, New and Common graphs with tooltips, amending the data in testCompareDataWithMultipleRuns to have one with replicates and opening the matching row. But I can help on this one once the other work is done!

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.68%. Comparing base (fdaf55d) to head (42452eb). Report is 1 commits behind head on beta.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## beta #710 +/- ## ========================================== + Coverage 85.63% 85.68% +0.05% ========================================== Files 83 83 Lines 2165 2173 +8 Branches 396 404 +8 ========================================== + Hits 1854 1862 +8 Misses 274 274 Partials 37 37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gmierz commented 1 month ago

@julienw I fixed the issue, and I also found another place that needed a change for this. I added a couple tests for these cases as well.

gmierz commented 1 month ago

@julienw the replicates patch is live on the treeherder backend now! See an example of this patch in action here: https://deploy-preview-710--mozilla-perfcompare.netlify.app/compare-results?baseRev=d0d28f60528be531682bba8e03025f47a6b87647&baseRepo=try&newRev=2d683f4c1f0a93888eab3a651eb319d3818695e6&newRepo=try&framework=13

julienw commented 1 month ago

Nice, thanks! I feel like we need to have a hint on the main row that there are more results inside, but I think that's good enough for now! Merging