-
Notifications
You must be signed in to change notification settings - Fork 100
fix(tests): adapt network tests for Meilisearch v1.30 HA changes #1184
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
base: main
Are you sure you want to change the base?
Conversation
Meilisearch v1.30.0 removed the sharding flag in favor of a new Leader/Follower High Availability model. This commit updates the test suite to align with these breaking changes.
WalkthroughThis change renames the test helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)tests/client/test_client_sharding.py (2)
tests/client/test_client_multi_search_meilisearch.py (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/client/test_client_multi_search_meilisearch.py (1)
85-102: Critical: Remove deprecatedshardingflag from request body.The test still sends
"sharding": Trueat line 88, which is incompatible with Meilisearch v1.30+. According to the PR objectives, v1.30 removed the sharding boolean from the/networkendpoint in favor of the leader/follower HA model.This will cause the test to fail against v1.30+ instances, defeating the purpose of this PR.
🔎 Apply this diff to remove the deprecated sharding flag:
resp = client.add_or_update_networks( { "self": REMOTE_MS_1, - "sharding": True, "remotes": { REMOTE_MS_1: { "url": "http://ms-1235.example.meilisearch.io", "searchApiKey": "xxxxxxxx", "writeApiKey": "xxxxxxxx", }, REMOTE_MS_2: { "url": "http://ms-1255.example.meilisearch.io", "searchApiKey": "xxxxxxxx", "writeApiKey": "xxxxxxxx", }, }, } )tests/client/test_client_sharding.py (1)
11-21: Critical: Remove deprecatedshardingflag from test options.The test still includes
"sharding": Trueat line 20, which is incompatible with Meilisearch v1.30+. The PR description explicitly states that sharding usage was removed fromtest_client_sharding.py, but this flag remains.This will cause the test to fail against v1.30+ instances.
🔎 Apply this diff to remove the deprecated sharding flag:
options = { "self": instance_name, "remotes": { instance_name: { "url": BASE_URL, "searchApiKey": "search-key-1", "writeApiKey": "write-key-1", } }, - "sharding": True, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/client/test_client_multi_search_meilisearch.py(2 hunks)tests/client/test_client_network.py(2 hunks)tests/client/test_client_sharding.py(2 hunks)tests/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/client/test_client_multi_search_meilisearch.py (2)
tests/test_utils.py (1)
reset_network_config(37-38)tests/conftest.py (1)
client(15-16)
tests/client/test_client_network.py (2)
tests/test_utils.py (1)
reset_network_config(37-38)tests/conftest.py (1)
client(15-16)
tests/client/test_client_sharding.py (2)
tests/test_utils.py (1)
reset_network_config(37-38)tests/conftest.py (1)
client(15-16)
🔇 Additional comments (7)
tests/test_utils.py (1)
37-38: LGTM! Clean migration to the new network configuration model.The function rename from
disable_shardingtoreset_network_configis appropriate, and the body correctly resets the network configuration using the new v1.30 HA model semantics (remotesandleaderfields).tests/client/test_client_multi_search_meilisearch.py (2)
5-5: LGTM! Import correctly updated.The import has been properly updated to use
reset_network_configinstead ofdisable_sharding.
115-115: LGTM! Cleanup correctly updated.The teardown call has been properly updated to use
reset_network_config(client).tests/client/test_client_network.py (2)
4-4: LGTM! Import correctly updated.The import has been properly updated to use
reset_network_configinstead ofdisable_sharding.
16-40: LGTM! Test correctly updated for v1.30 network model.The test body no longer includes the deprecated
shardingflag, and the assertions no longer check for it in the response. The teardown correctly usesreset_network_config(client).tests/client/test_client_sharding.py (2)
4-4: LGTM! Import correctly updated.The import has been properly updated to use
reset_network_configinstead ofdisable_sharding.
37-37: LGTM! Cleanup correctly updated.The teardown call has been properly updated to use
reset_network_config(client).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/client/test_client_sharding.py (1)
11-21: Critical: Remove deprecatedshardingfield from request body.The test still sends
"sharding": True(line 20) to the API in the options dict. According to the PR objectives and Meilisearch v1.30 changes, this field is no longer supported and should be removed from the request body, not just from assertions.🔎 Apply this diff to remove the deprecated field:
options = { "self": instance_name, "remotes": { instance_name: { "url": BASE_URL, "searchApiKey": "search-key-1", "writeApiKey": "write-key-1", } }, - "sharding": True, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/client/test_client_sharding.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/client/test_client_sharding.py (2)
tests/test_utils.py (1)
reset_network_config(37-38)tests/conftest.py (1)
client(15-16)
🔇 Additional comments (2)
tests/client/test_client_sharding.py (2)
4-4: LGTM: Import updated correctly.The import statement correctly reflects the renamed utility function
reset_network_config.
36-36: LGTM: Cleanup call updated correctly.The function call correctly uses the renamed
reset_network_configutility, which properly resets the network configuration using the new v1.30 semantics (remotes: {}andleader: None).
Meilisearch v1.30.0 removed the sharding flag in favor of a new Leader/Follower High Availability model. This commit updates the test suite to align with these breaking changes.
Pull Request
Related issue
Fixes #1183
What does this PR do?
This PR updates the SDK test suite to support the breaking networking changes introduced in Meilisearch v1.30.0.
Specifically, v1.30 removed the sharding boolean flag from the /network endpoint in favor of the new Leader/Follower High Availability model. The existing tests were failing because they explicitly sent sharding: true in payloads and asserted its presence in responses.
PR checklist
Please check if your PR fulfills the following requirements:
test_client_network.py andtest_client_sharding.py`disable_shardingtoreset_network_configand updated its implementation to clear the remotes and leader configuration (sending remotes: {} and leader: None) instead of attempting to toggle sharding off.These changes ensure the SDK tests pass successfully when run against a Meilisearch v1.30+ instance.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.