Keep random order stable for 24 hours in search API #13592
Conversation
diox
requested review from
a team and
bobsilverberg
and removed request for
a team
There was a problem hiding this comment.
This looks good, thanks for taking care of this @diox. I just had one suggestion about the tests.
r+wc
| def test_sort_random_featured(self): | ||
| qs = self._filter(data={'featured': 'true', 'sort': 'random'}) | ||
| # Note: this test does not call AddonFeaturedQueryParam so it won't | ||
| # apply the featured filtering. That's tested below in | ||
| # TestCombinedFilter.test_filter_featured_sort_random | ||
| assert qs['sort'] == ['_score'] | ||
| assert qs['query']['function_score']['functions'] == [ | ||
| {'random_score': {}} | ||
| {'random_score': {'seed': 737481}} |
There was a problem hiding this comment.
This seems to leave room for human error. Would it be better to declare a const for the date, and then use that date both as input to @freeze_time(), and also do our own calculation of .toordinal() on the const? Then we're only specifying the value once, and not relying on a manual calculation in the test.
There was a problem hiding this comment.
I altered the value in one of the tests in e95d109 to test 2 different dates.
I kinda prefer hardcoding the value: that way we don't rely on a potentially unknown computation, we know exactly what we're going to send. With the second test having a different value I think it should be a little more reliable.
There was a problem hiding this comment.
I'd argue it's not unknown at all, as you know how it's being calculated in the code under test, but this is just a nit and I'm fine with it landing as it now stands.
Fixes mozilla/addons#7339