Skip to content

Conversation

@hugocasa
Copy link
Collaborator

@hugocasa hugocasa commented Jan 6, 2026

No description provided.

@claude
Copy link
Contributor

claude bot commented Jan 6, 2026

Claude finished @hugocasa's task in 1m 1s —— View job


PR Review: fix(ci): increase integration test http client timeout for go test

  • Gather context and analyze PR changes
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Provide comprehensive feedback

Summary

This PR adds an explicit timeout=10.0 parameter to the httpx.Client initialization in _init_client() at integration_tests/test/wmill_integration_test_utils.py:59.

Review Feedback

✅ The change is appropriate and well-implemented.

The change is straightforward and addresses potential timeout issues during integration tests. Key observations:

  1. Reasonable timeout value: 10 seconds is a sensible default for HTTP requests in integration tests - long enough to handle slower operations (like script deployments or flow executions) but short enough to fail fast if something is truly stuck.

  2. Consistency consideration: Note that the _login() method at line 31 uses a separate httpx.Client instance without an explicit timeout. If timeout issues were occurring, you may want to consider adding a timeout there as well for consistency.

  3. Minor note on commit message: The commit message mentions "go test" but this is a Python test utility file. While the underlying tests may involve Go scripts, the commit message could be slightly clearer (e.g., "increase integration test http client timeout").

Suggested Improvement (Optional)

For consistency, you could also add a timeout to the unauthenticated client in _login():

def _login(self) -> str:
    with httpx.Client(base_url=self._url, timeout=10.0) as unauth_client:
        # ...

This is a minor suggestion and not blocking.


Verdict

Approved ✅ - The change is simple, targeted, and addresses the CI timeout issue. No bugs, security issues, or performance concerns.


Testing Instructions

This change affects the integration test infrastructure timeout settings. To verify: Run the integration tests locally (particularly any tests that previously timed out) and confirm they complete successfully with the new 10-second timeout. The change is internal to the test utilities and doesn't affect the Windmill application itself - no UI verification needed.

@cloudflare-workers-and-pages
Copy link

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 119b615
Status: ✅  Deploy successful!
Preview URL: https://5d99e100.windmill.pages.dev
Branch Preview URL: https://hc-integration-test-timeout.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel merged commit f708577 into main Jan 6, 2026
6 checks passed
@rubenfiszel rubenfiszel deleted the hc/integration-test-timeout branch January 6, 2026 10:12
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants