Skip to content

fix(tests): isolate mock session payloads in Vertex AI session service tests - #7158

Open
Jinzhengxu wants to merge 1 commit into
google:mainfrom
Jinzhengxu:fix/vertex-session-test-fixture-isolation
Open

Jinzhengxu wants to merge 1 commit into
google:mainfrom
Jinzhengxu:fix/vertex-session-test-fixture-isolation

Conversation

@Jinzhengxu

Copy link
Copy Markdown

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

  • Closes: N/A (no existing issue; test-only fix, described below following the bug template)

2. Or, if no issue exists, describe the change:

Problem:

tests/unittests/sessions/test_vertex_ai_session_service.py is order-dependent.
The mock_api_client_instance fixture registers the module-level
MOCK_SESSION_JSON_1/2/3/PAGE1/PAGE2 dicts directly in session_dict, while
MockAsyncClient._append_event rewrites session_dict[id]['update_time'] in
place. Every test_append_event* test therefore mutates the shared
MOCK_SESSION_JSON_1 (its update_time becomes the appended event's
timestamp), and any later test that compares get_session() against
MOCK_SESSION (built once at import time from the original timestamp) fails:

tests/unittests/sessions/test_vertex_ai_session_service.py:716: in test_get_and_delete_session
    assert (
E   AssertionError: assert Session(id='1...=1734005534.0) == Session(id='1...005532.123456)

In the default file order the append tests happen to run after the comparisons,
so this is invisible in CI today, but it surfaces as soon as the order changes:
with pytest-randomly, the file alone fails under every seed tried (1–6, 1–2
failures each), and pytest tests/unittests -n 8 fails
test_get_and_delete_session and test_delete_session_rejects_other_users_session
under seeds 1 and 2. Running each of the nine test_append_event* tests
immediately before test_get_and_delete_session reproduces the failure
deterministically in a single process.

Solution:

Deep-copy the session payloads in the fixture, exactly as the fixture already
does for the event payloads (copy.deepcopy(MOCK_EVENT_JSON)), so each test
gets its own session_dict entries and in-place updates cannot leak across
tests. No production code is touched.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Test-only change; the fixture itself is the fix. Results on this branch:

  • pytest tests/unittests/sessions/test_vertex_ai_session_service.py: 54 passed
    in default order and under pytest-randomly seeds 1–6 (previously 1–2
    failures per seed).
  • pytest tests/unittests -n 8 with pytest-randomly seeds 1 and 2: 2 of 2 runs green (15099 passed, 84 skipped, 27 xfailed, 2 xpassed each)
    (on main both seeds failed the two tests above).
  • pyink --check and isort --check-only on the file: unchanged.

Manual End-to-End (E2E) Tests:

Not applicable (test-only change).

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (N/A: test-only)
  • Any dependent changes have been merged and published in downstream modules. (N/A)

Additional context

Found while running the unit suite under random ordering. The nine polluting
tests are test_append_event, test_append_event_with_compaction,
test_append_event_with_compaction_and_custom_metadata,
test_append_event_with_usage_metadata,
test_append_event_with_usage_metadata_and_compaction,
test_append_event_with_part_metadata_round_trips,
test_append_event_strips_unsupported_part_metadata,
test_append_event_retries_once_on_429 and
test_append_event_fallback_for_older_sdk; each appends an event to session
'1'.

…e tests

The mock_api_client_instance fixture registered the module-level
MOCK_SESSION_JSON_* dicts directly in session_dict, while MockAsyncClient
rewrites session_dict[id]['update_time'] whenever an event is appended. Any
test that appends an event to session '1' therefore mutated the shared dict,
and later tests comparing get_session() against MOCK_SESSION (built at import
time from the original timestamp) failed depending on execution order.

Deep-copy the session payloads in the fixture, matching how the fixture
already copies the event payloads.
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.

2 participants