perf: Faster integration tests for add_documents.rs #5574

martin-g · 2025-05-16T13:14:13Z

Pull Request

Related issue

#4840

What does this PR do?

Applies the improvements suggested at Make the meilisearch integration tests run faster #4840 to add_document.rs IT test

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

irevoire

Hey @martin-g thanks for your interest in this issue!

This file was one of the tricky one sorry, a lot of the tests at the beginning don't look like the usual Meilisearch test and were not really described in my issue 😬

I left a lot of comments here and there, hope that helps. I also noticed that a you skipped a lot of tests where we could have used a shared instance + unique index.
Try searching for the Server::new.

The only case where we can't use a shared server is when the test is modifying a global part of the instance, like a specific test on the task queue, the api keys or the experimental features.
Here I believe you should be all good!

Also, the the file is fairly big. That was not part of the issue but if you want feel free to split it into 3 smaller files (or more if you find good names for each files).
We're trying to help rust analyzer run on our tests by never having file with more than 1000 lines of code.

Thanks again for your contribution!

irevoire · 2025-05-19T09:20:12Z

crates/meilisearch/tests/documents/add_documents.rs

+    // this is what is expected and should work
+    let server = Server::new_shared();
    let app = server.init_web_app().await;

    // post


The shared server is nice but afterward we're hardcoding the name of the index to be dog.
This could be flaky in the future, instead we should create a new unique index.

Also, when we snapshoted the task, we can see it has the task id 0 which is definirely flaky.
We should just call snapshot!(response, ...); without the json_string to properly snapshot the tasks

crates/meilisearch/tests/documents/add_documents.rs

irevoire · 2025-05-19T09:30:35Z

crates/meilisearch/tests/documents/add_documents.rs

    // wait first batch of documents to finish
    futures::future::join_all(waiter).await;
-    index.wait_task(4).await;
+    index.wait_task(4).await.succeeded();


Here it would also be better to share the Server and get a unique index but it also means we cannot wait for the task 4 anymore.
We now have to store and retrieve the task id of the last task in the join and wait for it.

Hey, we still have this issue. We cannot wait for any task with an hardcoded number as they will change between run on a shared server 😔

irevoire · 2025-05-19T09:30:46Z

crates/meilisearch/tests/documents/add_documents.rs

@@ -2925,7 +2925,7 @@ async fn batch_several_documents_addition() {
    }
    // wait second batch of documents to finish
    futures::future::join_all(waiter).await;
-    index.wait_task(9).await;
+    index.wait_task(9).await.failed();


Same as above

martin-g · 2025-05-19T11:54:53Z

Thank you for the review, @irevoire !
Let me better understand the testing mini-framework by working on smaller tests first! I will come back to this one later!

martin-g · 2025-05-26T08:34:57Z

@irevoire This is ready for a new review!
I didn't split the file into several ones because it will make the review harder.

irevoire

Hey!

Same as your other PR, we're almost good and only one last test is flaky because we're waiting for a task with an hardcoded task id.

Then we're good to merge 🚀

irevoire · 2025-05-27T09:29:04Z

crates/meilisearch/tests/documents/add_documents.rs

    // wait first batch of documents to finish
    futures::future::join_all(waiter).await;
-    index.wait_task(4).await;
+    index.wait_task(4).await.succeeded();


Hey, we still have this issue. We cannot wait for any task with an hardcoded number as they will change between run on a shared server 😔

martin-g · 2025-05-27T13:52:52Z

@irevoire Could you please take a look at https://github.com/meilisearch/meilisearch/actions/runs/15276592157/job/42964417626?pr=5574 ?
I do not understand why it the response now says that there were 2 received and indexed documents.

The test name is add_documents_no_index_creation but the old code was:

let server = Server::new().await;
    let index = server.index("test");

This creates an index, no ?

The new code is:

let server = Server::new_shared();
    let index = server.unique_index();

irevoire

Hey, I found your bug and quickly looked if I could find more. I think we'll be good once you fix this one

crates/meilisearch/tests/documents/add_documents.rs

Use Shared server where possible Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Some of them succeed, others fail. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Co-authored-by: Tamo <irevoire@protonmail.ch>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

irevoire

Thank you it's really awesome that you solved the anonymous index name!

irevoire requested changes May 19, 2025

View reviewed changes

irevoire mentioned this pull request May 19, 2025

Make the meilisearch integration tests run faster #4840

Open

32 tasks

martin-g marked this pull request as draft May 19, 2025 11:54

martin-g marked this pull request as ready for review May 26, 2025 08:32

martin-g force-pushed the faster-add_documents-it-tests branch 2 times, most recently from 0c5f85a to 79e33d1 Compare May 26, 2025 08:36

irevoire requested changes May 27, 2025

View reviewed changes

irevoire requested changes May 28, 2025

View reviewed changes

crates/meilisearch/tests/documents/add_documents.rs Outdated Show resolved Hide resolved

crates/meilisearch/tests/documents/add_documents.rs Outdated Show resolved Hide resolved

martin-g and others added 10 commits May 28, 2025 14:18

Faster IT tests for add_documents.rs

38b1c57

Use Shared server where possible Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Call .succeeded() or .failed() on the waited task

cf5d261

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Use shared server and unique indices for add_documents IT tests

fc88b00

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Fix clippy and fmt

ce9c930

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Use a Regex in insta dynamic redaction to replace Uuids with [uuid]

f3d6916

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Wait for the batched tasks bu their real uid.

3e0de6c

Some of them succeed, others fail. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Add dynamic redactions for uid, batchUid and taskUid

ce65ad2

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Apply suggestions from code review

36f0a14

Co-authored-by: Tamo <irevoire@protonmail.ch>

uuid is a production dependency of meili-snap

c13efde

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Update the status code

02929e2

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

martin-g force-pushed the faster-add_documents-it-tests branch from ddb61ad to 02929e2 Compare May 28, 2025 11:37

format the code

43ec97f

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

irevoire added this to the v1.16.0 milestone May 28, 2025

irevoire added maintenance Issue about maintenance (CI, tests, refacto...) no db change The database didn't change labels May 28, 2025

irevoire approved these changes May 28, 2025

View reviewed changes

irevoire added this pull request to the merge queue May 28, 2025

Merged via the queue into meilisearch:main with commit c8e77b5 May 28, 2025
12 of 16 checks passed

martin-g deleted the faster-add_documents-it-tests branch May 29, 2025 05:07

BrewTestBot mentioned this pull request Aug 4, 2025

meilisearch 1.16.0 Homebrew/homebrew-core#232297

Merged

meili-bot added the v1.16.0 PRs/issues solved in v1.16.0 released on 2025-08-04 label Aug 5, 2025

perf: Faster integration tests for add_documents.rs #5574

perf: Faster integration tests for add_documents.rs #5574

Uh oh!

Conversation

martin-g commented May 16, 2025

Pull Request

Related issue

What does this PR do?

PR checklist

Uh oh!

irevoire left a comment

Choose a reason for hiding this comment

Uh oh!

irevoire May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

irevoire May 19, 2025

Choose a reason for hiding this comment

Uh oh!

irevoire May 27, 2025

Choose a reason for hiding this comment

Uh oh!

irevoire May 19, 2025

Choose a reason for hiding this comment

Uh oh!

martin-g commented May 19, 2025

Uh oh!

martin-g commented May 26, 2025

Uh oh!

irevoire left a comment

Choose a reason for hiding this comment

Uh oh!

irevoire May 27, 2025

Choose a reason for hiding this comment

Uh oh!

martin-g commented May 27, 2025

Uh oh!

irevoire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

irevoire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!