mozilla / perfcompare

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

Populate the search form in the home page from the URL #730

Closed julienw closed 1 month ago

julienw commented 1 month ago

The goal for this PR is to provide the functionality for the button "Compare against another revision" in treeherder (example 1 / example 2)

I've been one step further as the original implementation to be able to provide the framework as well. I also decided that it makes sense to prefill the base repository with the same repository too.

I went with using a loader like in other views because we had to fetch information to the server about this revision. Because it's a one-time thing this wasn't strictly necessary and I could have do the fetch from the react component, but then I would have to deal with promise resolution and all sorts of things, and I thought this was simpler this way.

Please look at the 4 commits separately, I believe this will be simpler to review:

For the tab button, the related code in treeherder is https://github.com/mozilla/treeherder/blob/6df095030feeccfc93536050926f2e5b3627000a/ui/job-view/details/tabs/PerformanceTab.jsx#L238-L249. We can access the framework information from perfJobDetails[0].perfDocs.framework (if I remember properly) where we have the framework name, not the id (that's why the loader uses a frameworkName parameter -- actually I feel like we should have used the frameworkName in our URL everywhere too, but that ship has sailed :-) ).

For the job menu, the code is in https://github.com/mozilla/treeherder/blob/6df095030feeccfc93536050926f2e5b3627000a/ui/job-view/pushes/PushActionMenu.jsx#L183. Here I'm not sure we have access to the framework information, but I guess that's OK we can ignore it here.

Without any parameters (like before) With only the revision/repo parameters With the revision/repo parameters as well as the framework parameter With bogus value for revision With bogus value for repository With bogus value for framework

netlify[bot] commented 1 month ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit d606df49ef6d38e78c3e1af8f9484204376f1680
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/66cf1711fe00000008886612
Deploy Preview https://deploy-preview-730--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.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.58%. Comparing base (81e13e5) to head (d606df4). Report is 1 commits behind head on beta.

Files Patch % Lines
src/components/Search/loader.ts 96.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## beta #730 +/- ## ========================================== + Coverage 91.48% 91.58% +0.09% ========================================== Files 83 84 +1 Lines 2162 2199 +37 Branches 397 406 +9 ========================================== + Hits 1978 2014 +36 - Misses 161 162 +1 Partials 23 23 ```

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

beatrice-acasandrei commented 1 month ago

Looks good! Are we planing to improve this feature by allowing URL parameters for multiple new revisions / base revision / time range or is this the final work? In my opinion we might want to allow multiple new revisions since it's one of the new features.

julienw commented 1 month ago

Looks good! Are we planing to improve this feature by allowing URL parameters for multiple new revisions / base revision / time range or is this the final work? In my opinion we might want to allow multiple new revisions since it's one of the new features.

Good question! Here I focused on making it work for the use case of the button in the performance tab. I think supporting that we can input several revisions could be possible, and we could also probably share some code with the compare results loader in this process. But should we do that now? What would the end to end scenario look like?

Carla-Moz commented 1 month ago

Looks good! Are we planing to improve this feature by allowing URL parameters for multiple new revisions / base revision / time range or is this the final work? In my opinion we might want to allow multiple new revisions since it's one of the new features.

Good question! Here I focused on making it work for the use case of the button in the performance tab. I think supporting that we can input several revisions could be possible, and we could also probably share some code with the compare results loader in this process. But should we do that now? What would the end to end scenario look like?

@julienw @beatrice-acasandrei

Wouldn't accommodating multiple new revisions mean this feature would also be explicit in the related treeherder code's performance tabs? Because right now, in treeherder, we can only compare with "one" revision with the Compare against another revision or Compare Performance buttons, right? Just trying to understand better.

And if that's the case, I think for now we can keep the links with one revision, repo, and framework because users can pick the base, add multiple revisions, or pick the timeRange once they reach PerfCompare, right? If we want to add multiple revisions or timeRange in the home page url etc, we'd have to change this behavior in the treeherder Performance tabs which I think is beyond the scope of this work; users can already do so in PerfCompare.

Now, another scenario would be if a user knew which base, new revisions, framework,or timeRange they wanted in the home page and could populate the url that way. This is different from the treeherder tab Performance work, right? I think this would merit a new ticket altogether since this is a separate work flow. So I think this PR is done until we want to do another one for a new workflow.

What do you think? Let me know if this needs clarifying or if I've misunderstood something.

julienw commented 1 month ago

@Carla-Moz Fully agree with your comment :-)

thanks for the review!