mozilla / perfcompare

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

sorted confidence level in decending order #775

Open mercybassey opened 5 days ago

mercybassey commented 5 days ago

This PR displays results in descending order based on confidence levels, from high to medium and then to low.

Issue: #766

netlify[bot] commented 5 days ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit e9ad6a947aa1d1e9655dae745c3ee448ec323678
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/670933aff9dae80008b4fb6e
Deploy Preview https://deploy-preview-775--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.

hibaa03 commented 5 days ago

Hi, check the README doc to check and run the validators and tests if you're facing issues with it

mercybassey commented 5 days ago

Hi, check the README doc to check and run the validators and tests if you're facing issues with it

Thank you @hibaa03. I am currently working on it.

mercybassey commented 5 days ago

@Carla-Moz I have successfully made the change and below is the result locally.

perf

However my tests are failing locally. I think there is type mismatch, not sure. This might be because of my change, specifically adding filtering logic to the confidence column. Is there anything I could be doing wrong?

I'd really appreciate your guidance.

mercybassey commented 4 days ago

@Carla-Moz my tests are still failing, I'd really appreciate your help.

julienw commented 3 days ago

I believe that the test issues come from the fact that now that the "high" confidence value is selected by default, this filters out the results. If you look at the test data we're using in our tests (src/tests/utils/fixtures.ts), you'll see that some of the results are using confidence: 'High', however in some tests we use only the first result (because it makes the test faster) that has confidence: 'Low'. Without looking closer, I think this is the issue.

I think the easiest fix would be to change the first result in testCompareData to have confidence: 'High' so that it would be displayed in these tests. Another fix would be to manually select the other options in the filter, in each of these tests, but this seems more work.

mercybassey commented 3 days ago

The initial issue incorrectly mentioned to sort the values, but we really just want to filter them. Sorting is too complex with our current structure. Thanks for your understanding. Can you please make these changes?

Sure

mercybassey commented 3 days ago

@julienw it seems just changing the first result in testCompareData to have confidence: 'High' didn't resolve the issue.

mercybassey commented 3 days ago

@julienw it seems just changing the first result in testCompareData to have confidence: 'High' didn't resolve the issue.

This is my result locally. perf