Skip to content
This repository has been archived by the owner on Dec 17, 2018. It is now read-only.

Do not rely on order of items returned without sorting. #35

Merged
merged 2 commits into from Jan 7, 2017

Conversation

jciolek
Copy link
Contributor

@jciolek jciolek commented Jan 4, 2017

Summary

The following two tests for paginated results:

  • returns paginated object, paginates by default and shows total
  • paginates max and skips

were relying on the order of items returned from the storage without sorting applied. While this can work sometimes, many databases claim that the order of items returned without sorting applied is non-deterministic. This definitely holds true for Elasticsearch.

This PR simply adds sorting by name to the tests mentioned above.

There are no issues opened, which relate to this PR.
This PR is not dependent on any PRs in other repos.

@daffl
Copy link
Member

daffl commented Jan 4, 2017

Is the problem just the pagination tests? When a database doesn't guarantee the order of the items inserted I usually added a counter (or default sorting) to the db tests itself (see https://github.com/feathersjs/feathers-rethinkdb/blob/master/test/index.test.js#L8).

@jciolek
Copy link
Contributor Author

jciolek commented Jan 4, 2017

Correct, the problem is just the pagination test. The results are returned from ES in a non-deterministic fashion with constant_score, and so the tests were failing intermittently. After adding the sorting they were passing every time.

I believe changing the test could be good for clarity and consistency. In my view, this way there would not be any "hidden" parameters passed to the service under the test. Also, by focusing on just one particular test one could tell what query is being issued. I imagine, this way all the adapters would be tested in the same way, too.

That's just my two pennies' worth. Ultimately, the decision is yours :)

@daffl daffl merged commit 93c5449 into feathersjs-ecosystem:master Jan 7, 2017
@daffl
Copy link
Member

daffl commented Jan 7, 2017

Makes sense. Released as v0.9.3

@jciolek
Copy link
Contributor Author

jciolek commented Jan 8, 2017

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants