Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

sorted confidence level in decending order #775

Closed
wants to merge 5 commits into from

Conversation

mercybassey
Copy link

@mercybassey mercybassey commented Oct 9, 2024

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

Issue: #766

Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for mozilla-perfcompare ready!

Name Link
🔨 Latest commit e9ad6a9
🔍 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
Copy link

hibaa03 commented Oct 9, 2024

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

@mercybassey
Copy link
Author

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 mercybassey marked this pull request as draft October 9, 2024 16:56
@mercybassey mercybassey marked this pull request as ready for review October 9, 2024 17:48
@mercybassey
Copy link
Author

mercybassey commented Oct 9, 2024

@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
Copy link
Author

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

@julienw
Copy link
Contributor

julienw commented Oct 11, 2024

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.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

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?

@mercybassey
Copy link
Author

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
Copy link
Author

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

@mercybassey
Copy link
Author

@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

@julienw
Copy link
Contributor

julienw commented Nov 7, 2024

Hey @mercybassey, I'm gonna close this PR now, because some recent changes in our codebase made this change less useful and we'd like to gather feedback before doing it. But thanks again for your work!

@julienw julienw closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants