-
Notifications
You must be signed in to change notification settings - Fork 126
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
EVG-14306, EVG-19287: Apply mutli sort logic to test resolvers and set HasCedarResults field in the API task model #6402
Conversation
} | ||
} | ||
} | ||
var sort []testresult.SortBy | ||
if sortBy != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping single sort logic here since we are going to remove this resolver in favor of the Task.tests
resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were changes to go.mod and go.sum intentional?
Yep! I had to update the timber library. |
evergreen refresh |
if opts.SortOrderDSC { | ||
return results[i].Status > results[j].Status | ||
sort.SliceStable(results, func(i, j int) bool { | ||
for _, sortBy := range opts.Sort { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this equivalent to sort.SliceStable
for each of the sorts one after the other?
like
for _, sortBy := range opts.Sort {
sort.SliceStable(results, func(i, j int) bool {sortBy.lessThanFunc()}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because even though it's a stable sort, it will not respect the priority of the sort criteria. For example:
[("julian", 5), ("jonathan", 10), ("john", 1), ("april", 9)]
# sort by (name, int)
# after the first iteration:
[("april", 9), ("john", 1), ("jonathan", 10), ("julian", 5)]
# and after the second:
[("john", 1), ("julian", 5), ("april", 9), ("jonathan", 10)]
Basically, if we do it one after the other it completely ignores the previous sort criteria. You only want to sort collisions from the previous criteria.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I understand.
Is it equivalent to doing the SliceStable
s in reverse, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, this attempts to sort by the highest precedent criteria (returning if it's either less than or greater than depending on the sort order). If the keys are equal, instead of returning false
, it continues to the subsequent sort criteria until it can return true
or, in the case that all keys are equal, returns false
(on L328).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, first try sorting by key X, but if the values of key X are equal then try sorting by key Y, and so on until there the values of one of the keys specified in the sort criteria are not equal or until all the keys are exhausted, in which case return false
.
evergreen refresh |
EVG-14306, EVG-19287
Description
HasCedarResults
field to the API task model to fix the bug in the Task.tests resolver where older tasks using said field would not return tasks.Testing
Added unit tests and tested that single sort logic works in staging. I confirmed using the graphql playground that the Task.tests resolver returns tests for older tasks using the
HasCedarResults
field.Documentation
N/A