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

Sort user search results #1146

Merged
merged 1 commit into from
Jun 30, 2022
Merged

Sort user search results #1146

merged 1 commit into from
Jun 30, 2022

Conversation

mvandenburgh
Copy link
Member

#1124 removed the explicit ordering of user search results, which I believe is causing the flaky CI failures.

@jjnesbitt
Copy link
Member

Good find! Before we merge this though, I want to understand:

  1. Why this wasn't caught in CI when the PR was merged
  2. Why this error doesn't show up when run locally

If we can codify the reason for this in a test that'd be ideal.

@waxlamp
Copy link
Member

waxlamp commented Jun 30, 2022

Instead of removing the ordering from the search results, why can't we apply the same ordering to the ground truth in testing?

Keeping determinism in our search results seems like an important quality to maintain.

@jjnesbitt
Copy link
Member

Instead of removing the ordering from the search results, why can't we apply the same ordering to the ground truth in testing?

Keeping determinism in our search results seems like an important quality to maintain.

The problem is the PR that caused this removed the ordering from the results, so this PR adds that ordering back in. Looking at my two questions now, it seems obvious that both are caused by the non-deterministic ordering so I don't think any more tests are needed.

@waxlamp
Copy link
Member

waxlamp commented Jun 30, 2022

Instead of removing the ordering from the search results, why can't we apply the same ordering to the ground truth in testing?
Keeping determinism in our search results seems like an important quality to maintain.

The problem is the PR that caused this removed the ordering from the results, so this PR adds that ordering back in. Looking at my two questions now, it seems obvious that both are caused by the non-deterministic ordering so I don't think any more tests are needed.

Thanks, not sure how I missed that. The plus sign and the green background in your one-line diff should have been enough to clue me in 😝

@mvandenburgh mvandenburgh merged commit 921cccb into master Jun 30, 2022
@mvandenburgh mvandenburgh deleted the sort-user-search-results branch June 30, 2022 17:43
@dandibot
Copy link
Member

dandibot commented Jul 1, 2022

🚀 PR was released in v0.2.29 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants