[smg][ci] migrate reasoning_content tests to new infrastructure#16741
[smg][ci] migrate reasoning_content tests to new infrastructure#16741
Conversation
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the testing suite by migrating existing end-to-end tests for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the reasoning_content tests to the new pytest-based infrastructure. The new test file is well-structured and the migration appears correct. I've included a few suggestions to enhance code readability and reduce duplication by extracting a helper method and simplifying some assertions. Overall, this is a solid improvement to the test suite.
| def test_streaming_separate_reasoning_false(self, setup_backend): | ||
| """Test streaming with separate_reasoning=False, reasoning_content should be empty.""" | ||
| _, model, client, gateway = setup_backend | ||
|
|
||
| response = client.chat.completions.create( | ||
| model=model, | ||
| messages=[ | ||
| { | ||
| "role": "user", | ||
| "content": "What is 1+3?", | ||
| } | ||
| ], | ||
| max_tokens=100, | ||
| stream=True, | ||
| extra_body={"separate_reasoning": False}, | ||
| ) | ||
|
|
||
| reasoning_content = "" | ||
| content = "" | ||
| for chunk in response: | ||
| if chunk.choices[0].delta.content: | ||
| content += chunk.choices[0].delta.content | ||
| elif chunk.choices[0].delta.reasoning_content: | ||
| reasoning_content += chunk.choices[0].delta.reasoning_content | ||
|
|
||
| assert len(reasoning_content) == 0 | ||
| assert len(content) > 0 | ||
|
|
||
| def test_streaming_separate_reasoning_true(self, setup_backend): | ||
| """Test streaming with separate_reasoning=True, reasoning_content should not be empty.""" | ||
| _, model, client, gateway = setup_backend | ||
|
|
||
| response = client.chat.completions.create( | ||
| model=model, | ||
| messages=[ | ||
| { | ||
| "role": "user", | ||
| "content": "What is 1+3?", | ||
| } | ||
| ], | ||
| max_tokens=100, | ||
| stream=True, | ||
| extra_body={"separate_reasoning": True}, | ||
| ) | ||
|
|
||
| reasoning_content = "" | ||
| content = "" | ||
| for chunk in response: | ||
| if chunk.choices[0].delta.content: | ||
| content += chunk.choices[0].delta.content | ||
| elif chunk.choices[0].delta.reasoning_content: | ||
| reasoning_content += chunk.choices[0].delta.reasoning_content | ||
|
|
||
| assert len(reasoning_content) > 0 | ||
| assert len(content) > 0 |
There was a problem hiding this comment.
There's duplicated code for processing streaming responses in test_streaming_separate_reasoning_false and test_streaming_separate_reasoning_true. You can extract this logic into a helper method to improve code reuse and readability. I've also simplified the assertions to be more Pythonic and removed the unused gateway variable.
def _process_stream(self, response):
"""Helper to process streaming response and collect content."""
reasoning_content = ""
content = ""
for chunk in response:
if chunk.choices[0].delta.content:
content += chunk.choices[0].delta.content
elif chunk.choices[0].delta.reasoning_content:
reasoning_content += chunk.choices[0].delta.reasoning_content
return content, reasoning_content
def test_streaming_separate_reasoning_false(self, setup_backend):
"""Test streaming with separate_reasoning=False, reasoning_content should be empty."""
_, model, client, _ = setup_backend
response = client.chat.completions.create(
model=model,
messages=[
{
"role": "user",
"content": "What is 1+3?",
}
],
max_tokens=100,
stream=True,
extra_body={"separate_reasoning": False},
)
content, reasoning_content = self._process_stream(response)
assert not reasoning_content
assert content
def test_streaming_separate_reasoning_true(self, setup_backend):
"""Test streaming with separate_reasoning=True, reasoning_content should not be empty."""
_, model, client, _ = setup_backend
response = client.chat.completions.create(
model=model,
messages=[
{
"role": "user",
"content": "What is 1+3?",
}
],
max_tokens=100,
stream=True,
extra_body={"separate_reasoning": True},
)
content, reasoning_content = self._process_stream(response)
assert reasoning_content
assert content| self, setup_backend | ||
| ): | ||
| """Test streaming with separate_reasoning=True and stream_reasoning=False.""" | ||
| _, model, client, gateway = setup_backend |
There was a problem hiding this comment.
The gateway variable is unused and can be replaced with _ for clarity.
| _, model, client, gateway = setup_backend | |
| _, model, client, _ = setup_backend |
| _, model, client, gateway = setup_backend | ||
|
|
||
| response = client.chat.completions.create( | ||
| model=model, | ||
| messages=[ | ||
| { | ||
| "role": "user", | ||
| "content": "What is 1+3?", | ||
| } | ||
| ], | ||
| max_tokens=100, | ||
| extra_body={"separate_reasoning": False}, | ||
| ) | ||
|
|
||
| assert ( | ||
| not response.choices[0].message.reasoning_content | ||
| or len(response.choices[0].message.reasoning_content) == 0 | ||
| ) | ||
| assert len(response.choices[0].message.content) > 0 |
There was a problem hiding this comment.
The assertion for checking empty reasoning_content can be simplified. Also, the gateway variable is unused and can be removed. The assertion for content can also be made more Pythonic.
_, model, client, _ = setup_backend
response = client.chat.completions.create(
model=model,
messages=[
{
"role": "user",
"content": "What is 1+3?",
}
],
max_tokens=100,
extra_body={"separate_reasoning": False},
)
assert not response.choices[0].message.reasoning_content
assert response.choices[0].message.content| _, model, client, gateway = setup_backend | ||
|
|
||
| response = client.chat.completions.create( | ||
| model=model, | ||
| messages=[ | ||
| { | ||
| "role": "user", | ||
| "content": "What is 1+3?", | ||
| } | ||
| ], | ||
| max_tokens=100, | ||
| extra_body={"separate_reasoning": True}, | ||
| ) | ||
|
|
||
| assert len(response.choices[0].message.reasoning_content) > 0 | ||
| assert len(response.choices[0].message.content) > 0 |
There was a problem hiding this comment.
The gateway variable is unused. Also, the assertions can be simplified to be more Pythonic by checking the truthiness of the strings directly.
_, model, client, _ = setup_backend
response = client.chat.completions.create(
model=model,
messages=[
{
"role": "user",
"content": "What is 1+3?",
}
],
max_tokens=100,
extra_body={"separate_reasoning": True},
)
assert response.choices[0].message.reasoning_content
assert response.choices[0].message.content
Checklist
Review Process
/tag-run-ci-label,/rerun-failed-ci,/tag-and-rerun-ci) or contact authorized users to do so.