Skip to content

tests: Use Server::wait_task() instead of Index::wait_task() #5703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

tests: Use Server::wait_task() instead of Index::wait_task() #5703

wants to merge 2 commits into from

Conversation

martin-g
Copy link
Contributor

Pull Request

Related issue

#4840

What does this PR do?

  • The code is mostly duplicated. Server::wait_task() has better handling for errors and more retries.

@martin-g
tests: Use Server::wait_task() instead of Index::wait_task()
4de3c56 
The code is mostly duplicated. Server::wait_task() has better handling for errors and more retries.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g mentioned this pull request Jun 23, 2025
Open
32 tasks
@martin-g
Fix some search+replace issues. Make Server::wait_task() available fo…
983dd85 
…r Index:: methods

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
martin-g commented Jun 24, 2025
View reviewed changes
crates/meilisearch/tests/common/server.rs
Server::<Shared>::_wait_task(async |url| self.service.get(url).await, update_id).await
}

pub(super) async fn _wait_task<F>(request_fn: F, update_id: u64) -> Value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is needed because common::index needs to use it.
I guess this was the reason for the duplicated method in both files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't the common::Index contains a Server to make his request against?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Index looks like

meilisearch/crates/meilisearch/tests/common/index.rs

Lines 15 to 20 in bd2bd0f

pub struct Index<'a, State = Owned> {
pub uid: String,
pub service: &'a Service,
pub(super) encoder: Encoder,
pub(super) marker: PhantomData<State>,
}

There is no server around to be used...

irevoire
irevoire requested changes Jun 24, 2025
View reviewed changes
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I think we're good, I just have a question as I'm not sure I understood why we're still stuck with this method in the Index (even if the current version is still better as it removes duplicated code)

crates/meilisearch/tests/common/server.rs
Server::<Shared>::_wait_task(async |url| self.service.get(url).await, update_id).await
}

pub(super) async fn _wait_task<F>(request_fn: F, update_id: u64) -> Value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't the common::Index contains a Server to make his request against?

@martin-g
Copy link
Contributor Author

why we're still stuck with this method in the Index

Few Index methods need to wait for tasks.
For example

meilisearch/crates/meilisearch/tests/common/index.rs

Lines 36 to 50 in bd2bd0f

pub async fn load_test_set(&self) -> u64 {
let url = format!("/indexes/{}/documents", urlencode(self.uid.as_ref()));
let (response, code) = self
.service
.post_str(
url,
include_str!("../assets/test_set.json"),
vec![("content-type", "application/json")],
)
.await;
assert_eq!(code, 202);
let update_id = response["taskUid"].as_i64().unwrap();
self.wait_task(update_id as u64).await;
update_id as u64
}

waits for the test_set JSON[NL] to be fully indexed before returning.

Another option:
These methods are helpers, so we could just pass &server as an additional argument and use it (server.wait_task(...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irevoire irevoire irevoire requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martin-g @irevoire