Add feature tests #102

mmachatschek · 2021-02-14T10:52:03Z

WIP

This adds feature tests to the repo and solves #90 and #81

~~TODOS:~~

~~Add documentation how to run feature tests locally~~
Add more tests (based on https://github.com/meilisearch/meilisearch-symfony/)
Update the cleanup method and add a SCOUT_PREFIX config with a test key and only remove indexes with that prefix to avoid deleting documents from a local meilisearch instance

~~I added a matrix for the meilisearch containers to also test the library against older versions. If this should be removed, just tell me @shokme or @curquiza.~~

test workflow test workflow test Add docker tag in workflow description

lint

curquiza · 2021-02-14T17:18:15Z

Hello @mmachatschek!
Thanks for this awesome PR. Feel free to remove the "Draft PR" status when you are ready for a review!

I added a matrix for the meilisearch containers to also test the library against older versions. If this should be removed, just tell me @shokme or @curquiza.

No need to test the previous version of MeiliSearch for the moment. Since MeiliSearch is not stable, it does not worth it 🙂

Thanks again!

Remove duplicate underscore

mmachatschek · 2021-02-17T12:55:17Z

@curquiza this is ready for review.

For the actual search queries to MeiliSearch I had to wrap it with a retry function because of a race condition, where meilisearch's index wasn't yet ready for all entries. How was this solved in symfony or other implementations?

curquiza · 2021-02-17T16:50:25Z

Hello @mmachatschek!

For the actual search queries to MeiliSearch I had to wrap it with a retry function because of a race condition, where meilisearch's index wasn't yet ready for all entries. How was this solved in symfony or other implementations?

If I understand well you need to be sure the push of documents is ready when you want to search. We provide a function in the meilisearch-php client, which is waitForPendingUpdate.

A usage example: https://github.com/meilisearch/meilisearch-php/blob/6cb37bae30904b0364fc5dc30b71cc41fbc6cac4/tests/Endpoints/SearchTest.php#L17-L19

The implementation of the method if you are curious: https://github.com/meilisearch/meilisearch-php/blob/6cb37bae30904b0364fc5dc30b71cc41fbc6cac4/src/Endpoints/Indexes.php#L116-L137

Sorry it's not documented yet!

curquiza

This is the first review, I have not checked the code, I'm waiting for your update with the waitForPendingUpdate method 👍

.github/workflows/tests.yml

CONTRIBUTING.md

curquiza · 2021-02-18T15:26:34Z

Also @mmachatschek you seem really involved in this repo and all the MeiliSearch team wants to thank you! That's why I've just given the write access to you on the repo 🙂 No need to work on your forked repository anymore, and your reviews will be valuable (= take into account as a real reviewer by GitHub).
Hope you are ok with this! 😁

mmachatschek · 2021-02-19T09:16:40Z

If I understand well you need to be sure the push of documents is ready when you want to search. We provide a function in the meilisearch-php client, which is waitForPendingUpdate.

I looked into this. It would definitely fix the race condition in the tests. However it would require to mock the feature tests again and hook into the Indexes class as far as I can tell. Which we generally wanted to avoid in the feature tests.

Also @mmachatschek you seem really involved in this repo and all the MeiliSearch team wants to thank you! That's why I've just given the write access to you on the repo 🙂 No need to work on your forked repository anymore, and your reviews will be valuable (= take into account as a real reviewer by GitHub).
Hope you are ok with this! 😁

Thanks @curquiza for that I'll gladly contribute to this and maybe other repos of MeiliSearch 😊

curquiza · 2021-02-19T15:13:53Z

I looked into this. It would definitely fix the race condition in the tests. However it would require to mock the feature tests again and hook into the Indexes class as far as I can tell. Which we generally wanted to avoid in the feature tests.

Hum I don't understand why we would mock this part 🤔 In all our other repositories we do tests with the waitForPendingUpdate method without any mocking test. In general, we don't have any mocking tests.

mmachatschek · 2021-02-22T12:05:45Z

Hum I don't understand why we would mock this part thinking In all our other repositories we do tests with the waitForPendingUpdate method without any mocking test. In general, we don't have any mocking tests.

I can't think of a possibility to get the promiseId of an update/create event returned by the meilisearch-php client. via the laravel-scout library without changing any method signature or mocking/extending anything for the test. Maybe I'm missunderstanding something here 😅

@shokme maybe you have an Idea how to use either waitForPendingUpdate or hook into laravel-scout etc.

curquiza · 2021-02-23T13:18:04Z

Hum, I see you have a waitForIndexToUpdate which is a good start.
In this method, a way to know if all the actions are processed would be to call the route "Get all updates status" (https://docs.meilisearch.com/reference/api/updates.html#get-all-update-status). It's getAllUpdateStatus with the PHP SDK. You can gather all the status -> if there is one "enqueued", this is not over.
It's not performant, I agree, but this is for tests purpose :)

mmachatschek · 2021-02-23T14:17:55Z

@curquiza I updated the code now. Now we use your suggested method with getAllUpdateStatus. I iterate over the updateId and we will wait for all updates to finish

curquiza

I have some questions, sorry if there are newbie questions, I have a limited experience with Laravel 🙂

phpunit.xml.dist

curquiza · 2021-02-25T17:20:40Z

tests/Feature/FeatureTestCase.php

+    {
+        collect(resolve(Client::class)->getAllIndexes())->each(function (Indexes $index) {
+            // Starts with prefix
+            if (substr($index->getUid(), 0, strlen($this->getPrefix())) === $this->getPrefix()) {


What is the purpose of this condition?
I think we should delete all the indexes without any condition.

I just wanted to make sure that there is no data lost if someone runs the tests with an existing instance. So this just removes the indexes from meilisearch that start with the prefix set in the tests.

https://github.com/meilisearch/meilisearch-laravel-scout/pull/102/files#diff-abf1fa77a19f720052b75bc802df0260157b9064d8c3c12a80794f01e9acc065R37-R38

tests/Feature/MeilisearchConsoleCommandTest.php

tests/Fixtures/database/migrations/0000_00_00_000000_create_searchable_models_table.php

tests/Feature/MeilisearchTest.php

curquiza

Awesome!! Thank you so much @mmachatschek!!

bors merge

bors · 2021-03-02T19:37:36Z

Build succeeded:

mmachatschek added 5 commits February 14, 2021 09:22

Add new Unit/Feature test structure

a3429d4

Use appropriate composer v2 commands in test runner

b631ebb

Add meilisearch service in workflow

cde0036

test workflow test workflow test Add docker tag in workflow description

Use meilisearch key as env variable

f723963

Add basic tests

0975f08

lint

mmachatschek added 3 commits February 17, 2021 09:19

Only test against latest version of meilisearch

aba9ab5

Add contributing guide how to start a dev machine of meilisearch

dda17fd

Add scout prefix to prevent deleting indexes of existing instances

6076b4e

mmachatschek marked this pull request as ready for review February 17, 2021 12:49

Update tests

abfeb3a

Remove duplicate underscore

curquiza suggested changes Feb 18, 2021

View reviewed changes

.github/workflows/tests.yml Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

Fix workflow issue

b1f1456

Remove duplicate docker part from readme

9e18d51

Use meilisearch implementation to wait for pending updates

d7631b8

mmachatschek requested a review from curquiza February 23, 2021 14:21

curquiza reviewed Feb 25, 2021

View reviewed changes

mmachatschek added 2 commits February 26, 2021 08:13

Remove migration and inline the schema

386874b

Rename indexName to indexUid

42dc48d

mmachatschek requested a review from curquiza February 26, 2021 07:14

curquiza approved these changes Mar 2, 2021

View reviewed changes

curquiza added the skip-changelog label Mar 2, 2021

bors bot merged commit 8652b62 into meilisearch:main Mar 2, 2021

mmachatschek deleted the add_feature_tests branch March 3, 2021 07:15

mmachatschek mentioned this pull request Mar 16, 2021

Add tests for search() method #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature tests #102

Add feature tests #102

mmachatschek commented Feb 14, 2021 •

edited

curquiza commented Feb 14, 2021

mmachatschek commented Feb 17, 2021

curquiza commented Feb 17, 2021 •

edited

curquiza left a comment

curquiza commented Feb 18, 2021 •

edited

mmachatschek commented Feb 19, 2021 •

edited

curquiza commented Feb 19, 2021

mmachatschek commented Feb 22, 2021

curquiza commented Feb 23, 2021 •

edited

mmachatschek commented Feb 23, 2021 •

edited

curquiza left a comment

curquiza Feb 25, 2021

mmachatschek Feb 26, 2021

curquiza left a comment

bors bot commented Mar 2, 2021

Add feature tests #102

Add feature tests #102

Conversation

mmachatschek commented Feb 14, 2021 • edited

curquiza commented Feb 14, 2021

mmachatschek commented Feb 17, 2021

curquiza commented Feb 17, 2021 • edited

curquiza left a comment

Choose a reason for hiding this comment

curquiza commented Feb 18, 2021 • edited

mmachatschek commented Feb 19, 2021 • edited

curquiza commented Feb 19, 2021

mmachatschek commented Feb 22, 2021

curquiza commented Feb 23, 2021 • edited

mmachatschek commented Feb 23, 2021 • edited

curquiza left a comment

Choose a reason for hiding this comment

curquiza Feb 25, 2021

Choose a reason for hiding this comment

mmachatschek Feb 26, 2021

Choose a reason for hiding this comment

curquiza left a comment

Choose a reason for hiding this comment

bors bot commented Mar 2, 2021

mmachatschek commented Feb 14, 2021 •

edited

curquiza commented Feb 17, 2021 •

edited

curquiza commented Feb 18, 2021 •

edited

mmachatschek commented Feb 19, 2021 •

edited

curquiza commented Feb 23, 2021 •

edited

mmachatschek commented Feb 23, 2021 •

edited