Create integrations tests for all MeiliSearch http routes #54

ppamorim · 2020-10-04T20:43:58Z

Summary

Reasons why the tests are failing

~~On the test testGetDistinctAttribute and testUpdateDistinctAttribute:~~

~~- The attribute that returns from the server contains a double "", causing the error: XCTAssertEqual failed: ("product_id") is not equal to (""product_id"")~~

~~On the test testGetSettings and testUpdateSettings:~~

~~- Probably due to the acceptNewFields, this has been removed.~~

ppamorim · 2020-10-05T01:36:05Z

@curquiza Please review the tests, I am not sure what to do with the failed tests. Please bear that this PR is still a draft.

ppamorim · 2020-10-05T12:23:28Z

@curquiza Waiting for the merge of #51 This will sort the settings test failure. I will rebase everything before merging this PR.

curquiza · 2020-10-05T13:55:50Z

Hello @ppamorim! Thanks for this awesome PR!
The #51 is removed!

You can now rebase your branch 🙂

ppamorim · 2020-10-05T13:56:35Z

@curquiza Just did this.

curquiza · 2020-10-05T14:00:28Z

awesome @ppamorim. If your PR is ready for review, can you remove the draft status of this PR? 🙂
We will review it asap!

ppamorim · 2020-10-05T20:31:28Z

Waiting for PR #30 to be rebased and merged. This is needed to correct some failing tests in the search route.

ppamorim · 2020-10-06T12:19:43Z

@curquiza Sorry I had to rebase it since Github informed that it was out of sync with the master.

curquiza · 2020-10-06T14:15:24Z

@curquiza Sorry I had to rebase it since Github informed that it was out of sync with the master.

Yes, GitHub will inform you, but Bors will do the rebase at your place before merging 🙂 (except if you have merge conflicts. In this case you would have to rebase yourself)
So no worries about this GitHub warning.

ppamorim · 2020-10-06T16:00:10Z

@curquiza @bidoubiwa Just waiting for the review now. :)

curquiza

Hello @ppamorim!
Huge work!

This is the review part I 😁 I will review the test files tomorrow!

~~One change asked 😇~~

[ ] If I'm not wrong, there is no test about the facetsDistribution of SearchResult. I mean, doing a test with facetsDistribution as a search parameter and check that the facetsDistribution in the serach *result are accessible. See the issue requirement (#58).

curquiza · 2020-10-06T16:40:39Z

Sources/MeiliSearch/Model/SearchParameters.swift

+    public func encode(to encoder: Encoder) throws {
+        var container = encoder.container(keyedBy: CodingKeys.self)
+        try container.encode(query, forKey: .query)
+        if limit != 20 {


Why should we need to do a condition with the default value?

This is a filter to default parameters, so far I know these values are fixed in the server and if they are not informed in the HTTP request the server will apply the default ones, am I correct?

The API is based in this documentation: https://docs.meilisearch.com/references/search.html#search-in-an-index-with-post-route

Note how the first example only contains the query item as mandatory, all other options are optional. This script remove the need of informing the default values.

But what is the point of doing this?

if they are not informed in the HTTP request the server will apply the default ones,

Yes, exactly, so no need to check the default value. Just send to MeiliSearch what the users fill, we don't care if they fill default values. MeiliSearch will manage that.

This big if conditions are really complex for a job that MeiliSearch already does.

Or maybe I missed something!

Tests/MeiliSearchIntegrationTests/XCTestManifests.swift

ppamorim · 2020-10-06T21:05:54Z

Hello @ppamorim!
Huge work!

This is the review part I 😁 I will review the test files tomorrow!

One change asked 😇

If I'm not wrong, there is no test about the facetsDistribution of SearchResult. I mean, doing a test with facetsDistribution as a search parameter and check that the facetsDistribution in the serach *result are accessible. See the issue requirement (facetsDistribution is missing in SearchResult model #58).

There is a test related to this, it's named testSearchFacetsDistribution. It checks for the genres of books and compares the dictionary that returns from Meilisearch.

curquiza · 2020-10-07T11:53:42Z

There is a test related to this, it's named testSearchFacetsDistribution. It checks for the genres of books and compares the dictionary that returns from Meilisearch.

My bad! I've seen it!

Sources/MeiliSearch/Model/Stat.swift

curquiza

Review part II

You did a so HUGE work on the tests!!!! We all know here how it's a painful job to write tests and you created a lot! Thanks for that 🙂

Some remarks:

still the questioning here (#54 (comment)) from my previous review
I remark, for the Settings tests , for the get methods, you use the update method. For me, it's a little bit redundant with the update test right after: you can only check the default value of the settings.
But, as it's not really mandatory and just a detail, I'm going to open a PR to your branch to suggest changes so that it avoids you another painful work, just let me the day 😉

Sources/MeiliSearch/Settings.swift

Tests/MeiliSearchIntegrationTests/UpdatesTests.swift

Sources/MeiliSearch/Model/SearchResult.swift

Co-authored-by: Clémentine Urquizar <[email protected]>

…isearch-swift into feature/integrationTest

* Add forgotten tests * Simplify GET tests for Settings * Add attribtuesForFaceting in tests * Simplify UPDATE tests for Settings * Rename wrong variables

curquiza

This PR is finally ready for a merge :)

Two points could be improved from my POV:

the behemot (see #54 (comment)): I'm going to open an issue
the if condition (see #54 (comment)). I'm going to open a PR with an improvement suggestion in the next few days.

Thanks again @ppamorim!

curquiza · 2020-10-08T15:28:44Z

bors try

bors · 2020-10-08T15:31:15Z

try

Build succeeded:

swift-test

curquiza · 2020-10-08T15:31:29Z

bors merge

bors · 2020-10-08T15:34:06Z

Build succeeded:

swift-test

ppamorim added enhancement New feature or request Hacktoberfest labels Oct 4, 2020

ppamorim self-assigned this Oct 4, 2020

ppamorim linked an issue Oct 4, 2020 that may be closed by this pull request

Create integrations tests for all MeiliSearch http routes #49

Closed

24 tasks

ppamorim linked an issue Oct 5, 2020 that may be closed by this pull request

getDistinctAttribute returning value with double comma #56

Closed

ppamorim requested a review from curquiza October 5, 2020 12:22

ppamorim mentioned this pull request Oct 5, 2020

Remove acceptNewFields routes #51

Merged

ppamorim force-pushed the feature/integrationTest branch 3 times, most recently from d831059 to f5b7391 Compare October 5, 2020 13:54

ppamorim force-pushed the feature/integrationTest branch from f5b7391 to ec7254e Compare October 5, 2020 13:56

curquiza requested a review from bidoubiwa October 5, 2020 14:00

ppamorim linked an issue Oct 5, 2020 that may be closed by this pull request

Distinct attribute missing in settings #57

Closed

ppamorim mentioned this pull request Oct 5, 2020

Distinct attribute missing in settings #57

Closed

ppamorim marked this pull request as ready for review October 5, 2020 17:30

ppamorim force-pushed the feature/integrationTest branch 2 times, most recently from 1830852 to d5afa00 Compare October 5, 2020 17:33

ppamorim mentioned this pull request Oct 5, 2020

Add support to Encodable types for addDocuments function #53

Merged

2 tasks

ppamorim linked an issue Oct 5, 2020 that may be closed by this pull request

facetsDistribution is missing in SearchResult model #58

Closed

2 tasks

This was referenced Oct 5, 2020

facetsDistribution is missing in SearchResult model #58

Closed

getDistinctAttribute returning value with double comma #56

Closed

Create a better error handler #40

Closed

ppamorim linked an issue Oct 5, 2020 that may be closed by this pull request

Create a better error handler #40

Closed

ppamorim force-pushed the feature/integrationTest branch from 440f5f0 to c799343 Compare October 5, 2020 21:11

ppamorim force-pushed the feature/integrationTest branch from 70a6f6b to 9661172 Compare October 6, 2020 12:19

curquiza requested changes Oct 6, 2020

View reviewed changes

curquiza removed a link to an issue Oct 6, 2020

Create a better error handler #40

Closed

Add missing instrumented tests for settings, update and search routes

c06f6fd

ppamorim force-pushed the feature/integrationTest branch from c199a7c to c06f6fd Compare October 6, 2020 21:38

curquiza reviewed Oct 7, 2020

View reviewed changes

Sources/MeiliSearch/Model/Stat.swift Outdated Show resolved Hide resolved

Remove unecessary CodingKeys

76a1b33

ppamorim requested a review from curquiza October 7, 2020 19:53

ppamorim mentioned this pull request Oct 7, 2020

Feature/cocoapods #62

Closed

curquiza reviewed Oct 8, 2020

View reviewed changes

Sources/MeiliSearch/Settings.swift Outdated Show resolved Hide resolved

Tests/MeiliSearchIntegrationTests/UpdatesTests.swift Outdated Show resolved Hide resolved

curquiza reviewed Oct 8, 2020

View reviewed changes

Sources/MeiliSearch/Model/SearchResult.swift Show resolved Hide resolved

ppamorim and others added 4 commits October 8, 2020 10:24

Remove CodingKeys for DistinctAttributePayload

666e9a8

Co-authored-by: Clémentine Urquizar <[email protected]>

Reduce complexity of status tests

b9ee70d

Merge branch 'feature/integrationTest' of github.com:meilisearch/meil…

0bcbdb4

…isearch-swift into feature/integrationTest

Make the tests easier to read and maintain (#64)

7f3c1a4

* Add forgotten tests * Simplify GET tests for Settings * Add attribtuesForFaceting in tests * Simplify UPDATE tests for Settings * Rename wrong variables

curquiza approved these changes Oct 8, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Oct 8, 2020

Try #54:

c770bde

bors bot merged commit 06c8527 into master Oct 8, 2020

bors bot deleted the feature/integrationTest branch October 8, 2020 15:34

curquiza added skip-changelog The PR will not appear in the release changelogs and removed enhancement New feature or request labels Oct 8, 2020

Create integrations tests for all MeiliSearch http routes #54

Create integrations tests for all MeiliSearch http routes #54

Uh oh!

Conversation

ppamorim commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reasons why the tests are failing

Uh oh!

ppamorim commented Oct 5, 2020

Uh oh!

ppamorim commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curquiza commented Oct 5, 2020

Uh oh!

ppamorim commented Oct 5, 2020

Uh oh!

curquiza commented Oct 5, 2020

Uh oh!

ppamorim commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppamorim commented Oct 6, 2020

Uh oh!

curquiza commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppamorim commented Oct 6, 2020

Uh oh!

curquiza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curquiza Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

ppamorim Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

ppamorim Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

curquiza Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ppamorim commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curquiza commented Oct 7, 2020

Uh oh!

Uh oh!

curquiza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

curquiza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curquiza commented Oct 8, 2020

Uh oh!

bors bot commented Oct 8, 2020

try

Uh oh!

curquiza commented Oct 8, 2020

Uh oh!

bors bot commented Oct 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ppamorim commented Oct 4, 2020 •

edited

Loading

ppamorim commented Oct 5, 2020 •

edited

Loading

ppamorim commented Oct 5, 2020 •

edited

Loading

curquiza commented Oct 6, 2020 •

edited

Loading

curquiza left a comment •

edited

Loading

curquiza Oct 7, 2020 •

edited

Loading

ppamorim commented Oct 6, 2020 •

edited

Loading

curquiza left a comment •

edited

Loading

curquiza left a comment •

edited

Loading