fix(tests): Correct unreachable assertion in truncation test #23425
Conversation
In the test_bigger_truncation_size test, the 'err' variable from the pytest.raises context manager was being incorrectly reassigned by the API call. Because the `await client.post(...)` line raises an exception as expected, the `assert` statement on the following line was unreachable and never executed. This meant the test was only verifying the exception type, not the content of the error message. This commit refactors the test to correctly handle the exception. The API call is now made without assignment, and the assertion is moved outside the `with` block, checking `err.value` to inspect the actual exception instance. This ensures the test is robust and properly verifies the error message content. Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
AzizCode92
requested review from
DarkLight1337,
aarnphm,
robertgshaw2-redhat and
simon-mo
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an unreachable assertion in the test_bigger_truncation_size test. The change ensures that the test now properly verifies the exception raised for invalid truncation sizes. I have one suggestion to improve the robustness of the assertion by checking the structured error data instead of performing a brittle string comparison on the exception's text representation. This will make the test more resilient to future formatting changes.
| assert str(err.value) == f"""openai.BadRequestError: | ||
| Error code: 400 - {{'object': 'error', | ||
| 'message': 'truncate_prompt_tokens value | ||
| ({truncation_size}) | ||
| is greater than max_model_len ({max_model_len}). | ||
| Please, select a smaller truncation size.', | ||
| 'type': 'BadRequestError', | ||
| 'param': None, 'code': 400}}""" |
There was a problem hiding this comment.
While this change correctly fixes the test's logic, asserting against the exact string representation of the exception is brittle. The test could fail due to minor, unrelated changes in whitespace or formatting within the openai library's BadRequestError.__str__ method or vLLM's error message.
A more robust approach is to inspect the structured error data from the exception object. This makes the test resilient to formatting changes and more clearly expresses the intent of what is being tested.
assert err.value.http_status == 400
# The error from the server is nested under the 'error' key in the response body.
error_details = err.value.body["error"]
assert error_details["type"] == "BadRequestError"
assert error_details["code"] == 400
# The underlying ValueError is a single-line string. Comparing against
# that is more robust than a multi-line string sensitive to indentation.
expected_message = (f"truncate_prompt_tokens value "
f"({truncation_size}) "
f"is greater than max_model_len ({max_model_len}). "
f"Please, select a smaller truncation size.")
assert error_details["message"] == expected_message
github-actions
bot
added
the
ready
Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
Head branch was pushed to by a user without write access
|
The test fails now, PTAL |
|
Probably it is better to parse the error message as suggested by Gemini |
Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
|
Will fix it again. |
|
Fixing my local setup to run the test on my end before pushing here again |
Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
Head branch was pushed to by a user without write access
Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Ekagra Ranjan <3116519+ekagra-ranjan@users.noreply.github.com>
…oject#23425) Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>
2 participants
Purpose
In the test_bigger_truncation_size test, the 'err' variable from the pytest.raises context manager was being incorrectly reassigned by the API call.
Because the
await client.post(...)line raises an exception as expected, theassertstatement on the following line was unreachable and never executed. This means the test was only verifying the exception type, not the content of the error message.This commit fixes the test to correctly handle the exception. The API call is now made without assignment, and the assertion is moved outside the
withblock, checkingerr.valueto inspect the actual exception instance. This ensures the test is robust and properly verifies the error message content.Test Plan
Test Result
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.