-
Notifications
You must be signed in to change notification settings - Fork 2.1k
perf: Faster index::update_index IT tests #5579
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.
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
perf: Faster index::update_index IT tests #5579
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for tackling all these issues!
let index = server.unique_index(); | ||
let (create_task, code) = index.create(Some("id")).await; | ||
|
||
assert_eq!(code, 202); | ||
index.wait_task(create_task.uid()).await.succeeded(); | ||
|
||
let documents = json!([ | ||
{ | ||
"id": "11", | ||
"content": "foobar" | ||
} | ||
]); | ||
index.add_documents(documents, None).await; | ||
let (add_docs_task, add_docs_status_code) = index.add_documents(documents, None).await; | ||
assert_eq!(add_docs_status_code, 202); | ||
index.wait_task(add_docs_task.uid()).await.succeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case instead of creating a now index and pushing documents I think we can use one of the already existing indexes like the one with movies (SHARED_DOCUMENTS) crates/meilisearch/tests/common/mod.rs:228
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async fn error_update_existing_primary_key() {
let index = shared_index_with_documents().await;
let (update_task, code) = index.update(Some("primary")).await;
index.update() is missing for &Index<Shared>
. It is available only for Index<Owned>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I see you're right.
Do you think you could create a new update_fail
method similar to this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't work with:
pub async fn update_index_fail(&self) -> (Value, StatusCode) {
let (mut task, code) = self._update_settings(Value(serde_json::Value::Object(Default::default()))).await;
if code.is_success() {
task = self.wait_task(task.uid()).await;
if task.is_success() {
panic!(
"`update_index_fail` succeeded: {}",
serde_json::to_string_pretty(&task).unwrap()
);
}
}
(task, code)
}
I will continue on it soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, but it's because you tried to update the settings here:
let (mut task, code) = self._update_settings(Value(serde_json::Value::Object(Default::default()))).await;
You should create your own _update
around here that contains the code of the function currently specialized for the Owned index.
Then you can create the update_index_fail
function the right way and update the original update function to make it call your _update
wrapper.
Once this is done I think you'll be the only other person that entirely knows how to update the tests in all cases 😅
let server = Server::new_shared(); | ||
let index = server.unique_index(); | ||
let (task, code) = index.update(None).await; | ||
|
||
assert_eq!(code, 202); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we could use the special nonexistent index that already exists instead of making a unique index
Use a shared server where possible. Assert succeeded/failed task waits. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
384aa59
to
b4ca0a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just perfect thank you! 🙏
Pull Request
Related issue
#4840
What does this PR do?