Skip to content

Comments

Add unit tests for TimeoutHTTPAdapter#1361

Merged
Zeyi-Lin merged 8 commits intomainfrom
copilot/add-tests-for-timeout-adapter
Dec 5, 2025
Merged

Add unit tests for TimeoutHTTPAdapter#1361
Zeyi-Lin merged 8 commits intomainfrom
copilot/add-tests-for-timeout-adapter

Conversation

Copy link
Contributor

Copilot AI commented Dec 5, 2025

Description

Adds comprehensive unit tests for TimeoutHTTPAdapter to validate timeout injection behavior and parameter handling.

Closes: #1358

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue
for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Test Coverage

Added 8 tests covering:

  • Initialization: timeout parameter storage, default None behavior, parameter passthrough to HTTPAdapter
  • Timeout injection: default timeout application, explicit override, None handling (no injection)
  • Integration: create_session() uses DEFAULT_TIMEOUT (60s), correct inheritance from HTTPAdapter

Tests use unittest.mock.patch to verify timeout values passed to parent send() method.

Additional Changes

Fixed tutils/check.py to handle missing swanboard package gracefully (prevents IndexError during test setup).

Original prompt

This section details on the original issue you should resolve

<issue_title>为TimeoutAdapter添加测试</issue_title>
<issue_description>### 功能请求是否与某个问题相关?请描述。

与此issue相关: #1358

描述您希望的解决方案

在test/unit对应目录下添加单测

描述您考虑过的替代方案

No response</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 6 commits December 5, 2025 09:07
Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
…port to top

Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
Co-authored-by: SAKURA-CAT <79990647+SAKURA-CAT@users.noreply.github.com>
Copilot AI changed the title [WIP] Add tests for TimeoutAdapter Add unit tests for TimeoutHTTPAdapter Dec 5, 2025
Copilot AI requested a review from SAKURA-CAT December 5, 2025 09:20
@SAKURA-CAT SAKURA-CAT marked this pull request as ready for review December 5, 2025 09:21
@SAKURA-CAT
Copy link
Member

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a comprehensive set of unit tests for the TimeoutHTTPAdapter, which is great for ensuring its behavior is correct and preventing future regressions. The tests cover initialization, timeout injection logic, and integration with create_session. The fix in tutils/check.py to gracefully handle a missing swanboard package is also a good improvement.

I've left a couple of suggestions for minor improvements:

  • In tutils/check.py, the logic for parsing the package version can be made more robust and secure.
  • In test/unit/core_python/test_session.py, one of the tests has a redundant assertion that can be removed to improve clarity.

Overall, this is a solid contribution that improves the test coverage and robustness of the codebase.

Improves the extraction of the swanboard version from pip output by handling whitespace and line endings more robustly.
@Zeyi-Lin Zeyi-Lin self-requested a review December 5, 2025 10:27
@Zeyi-Lin Zeyi-Lin merged commit f5adae3 into main Dec 5, 2025
5 checks passed
@SAKURA-CAT SAKURA-CAT deleted the copilot/add-tests-for-timeout-adapter branch December 7, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

为TimeoutAdapter添加测试

3 participants