Skip to content

Conversation

evansims
Copy link
Member

@evansims evansims commented Oct 3, 2025

This PR addresses flaky unit tests resulting from race conditions in async batch check operations and timing precision issues in retry logic.

  • Fixed race conditions when running BatchCheck tests with parallelization
    Replaced sequential mock responses with conditional responses based on request body content. I only noticed this after the package modernization and adoption of uv, where pytest is run with parallelization enabled by default, unlike previously.

  • Improved retry timing stability
    I adjusted the retry delay from exact matches to tolerance ranges of 1 or 2 seconds, and increased test timeouts to account for variance. I haven't encountered this locally, but have seen it pop up a few times on our CI during PR status checks.

References

openfga/python-sdk#231

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • Tests
    • Updated retry/backoff expectations for 429 handling, increasing Retry-After duration and maximum wait limits.
    • Adjusted assertions to verify sleep durations within a bounded range around the Retry-After value.
    • Aligned tests using HTTP-date Retry-After headers with the new timing values.
    • Replaced static mock sequences with request-driven side effects to return responses based on payload content, applied to both async and sync tests.
    • Maintained existing success and error assertions with the new mocking approach.

@evansims evansims added the enhancement New feature or request label Oct 3, 2025
@evansims evansims requested a review from a team as a code owner October 3, 2025 22:36
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

Updates test logic in Python client templates: adjusts 429 Retry-After timing values and assertions, and replaces static mock side-effect lists with body-aware side_effect functions in client tests for both async and sync variants.

Changes

Cohort / File(s) Summary
Retry/Backoff test updates
config/clients/python/template/test/api_test.py.mustache, config/clients/python/template/test/sync/api_test.py.mustache
Increased retry_after_in_sec from 5→10; renamed five_seconds_from_now→ten_seconds_from_now; raised RetryParams max_wait_in_sec to 15; updated Retry-After header expectations and sleep assertions to range checks aligned to new timing.
Dynamic mock side-effects in client tests
config/clients/python/template/test/client/client_test.py.mustache, config/clients/python/template/test/sync/client/client_test.py.mustache
Replaced static side-effect lists with a function-based mock_side_effect that inspects request body (e.g., tuple_key/user) to return or raise specific responses; applied across affected tests while preserving existing assertions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Test
  participant Client
  participant ServerMock as Server Mock
  note over Test,Client: 429 Retry flow with Retry-After (HTTP-date or seconds)

  Test->>Client: send request
  Client->>ServerMock: HTTP request
  ServerMock-->>Client: 429 with Retry-After=10s (or HTTP-date)
  Client->>Client: compute wait = ~10s (bounded by max_wait_in_sec=15)
  Client-->>Test: sleep(duration within [8s..10s])
  Client->>ServerMock: retry request
  ServerMock-->>Client: success or terminal response
  Client-->>Test: final result
Loading
sequenceDiagram
  autonumber
  actor Test
  participant Client
  participant Mock as mock_request.side_effect

  note over Mock: Determines response by inspecting request body (e.g., user/tuple_key)

  Test->>Client: invoke API with body A
  Client->>Mock: request(body A)
  Mock-->>Client: return response mapped to body A (200/400/exception)
  Client-->>Test: assertion on result/path

  Test->>Client: invoke API with body B
  Client->>Mock: request(body B)
  Mock-->>Client: return response mapped to body B
  Client-->>Test: assertion on result/path
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely reflects the primary change of stabilizing the Python SDK tests by fixing flaky unit tests through timing and mock behavior adjustments.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ci/python/fix-flaky-unit-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@evansims evansims added the python-sdk Affects the Python SDK label Oct 3, 2025
@evansims evansims added this pull request to the merge queue Oct 3, 2025
Merged via the queue into main with commit b60861c Oct 3, 2025
19 checks passed
@evansims evansims deleted the ci/python/fix-flaky-unit-tests branch October 3, 2025 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request python-sdk Affects the Python SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants