-
Notifications
You must be signed in to change notification settings - Fork 100
tests: Fix tests failures when testing against 1.15.1 #673
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
E.g. https://github.com/meilisearch/meilisearch-rust/actions/runs/15610369450/job/43969605803?pr=625 Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
WalkthroughThe changes update a test comment and assertion in the error handling module, and add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyManager
Client->>KeyManager: Request access for action (e.g., ChatCompletions)
KeyManager->>KeyManager: Match action to enum variant (ChatCompletions)
KeyManager-->>Client: Grant or deny access based on action
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (1)
src/key.rs (1)
659-661
: Addition ofChatCompletions
variant looks good – add ecosystem touch-ups.The serde rename and docstring are correct and match the server spelling from 1.15.1. Two minor follow-ups worth considering:
- Update any public docs/tutorials that enumerate
Action
values so this new variant is visible to users.- Add an enum-round-trip test (
serde_json::to_string
⇄from_str
) to ensure the casing stays locked against future renames.No code change required.
// there will never be a `meilisearch.gouv.fr` addr since these domain names are controlled by the state of france | ||
reqwest::Url::parse("https://meilisearch.gouv.fr").unwrap(), | ||
)) | ||
.await | ||
.unwrap_err(), | ||
); | ||
assert_eq!( | ||
error.to_string(), | ||
"HTTP request failed: error sending request for url (https://meilisearch.gouv.fr/)" | ||
"HTTP request failed: error sending request" | ||
); |
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.
💡 Verification agent
❓ Verification inconclusive
Assertion on exact reqwest error string is brittle – prefer substring or prefix match.
reqwest
’s error wording ("error sending request"
) can differ between patch versions, features (native-tls/rustls), or OS.
Relying on full equality risks false failures the next time reqwest
bumps.
-assert_eq!(
- error.to_string(),
- "HTTP request failed: error sending request"
-);
+assert!(
+ error.to_string().starts_with("HTTP request failed:"),
+ "Unexpected reqwest error format: {}",
+ error
+);
This keeps the intent (we got an HTTP error) while de-flaking the test.
Prefer substring match over exact reqwest error string
Matching the full reqwest
error message can break tests when minor wording or feature differences occur. It’s safer to assert on a static prefix and include the actual error in the failure message.
• File: src/errors.rs
Lines: 398–407
- assert_eq!(
- error.to_string(),
- "HTTP request failed: error sending request"
- );
+ assert!(
+ error.to_string().starts_with("HTTP request failed:"),
+ "Unexpected reqwest error format: {}",
+ error
+ );
🤖 Prompt for AI Agents
In src/errors.rs around lines 398 to 407, the test asserts exact equality on the
reqwest error string, which is brittle due to possible variations in error
wording across versions and platforms. Modify the assertion to check that the
error string starts with or contains a stable substring like "HTTP request
failed" instead of exact equality. This ensures the test verifies the error type
without breaking on minor message changes.
E.g. https://github.com/meilisearch/meilisearch-rust/actions/runs/15610369450/job/43969605803?pr=625
Pull Request
Related issue
#625 IT tests fail
What does this PR do?
Summary by CodeRabbit
New Features
/chatCompletions
endpoint, enabling new actions related to chat completions.Style