Skip to content

test(tracing): mock urlopen as a context manager in flush_pending test - #55

Merged
alessiodevoto merged 1 commit into
NVIDIA-NeMo:mainfrom
Yeriahz:fix-flush-pending-mock
Jul 31, 2026
Merged

test(tracing): mock urlopen as a context manager in flush_pending test#55
alessiodevoto merged 1 commit into
NVIDIA-NeMo:mainfrom
Yeriahz:fix-flush-pending-mock

Conversation

@Yeriahz

@Yeriahz Yeriahz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a broken mock in test_pending_threads_self_evict_when_post_returns. Test-only change; no production code touched.

Problem

The test patches urlopen with side_effect=fast_post, where fast_post returns None. The production code at src/nooa/tracing/_litellm_journal.py:218 uses the result as a context manager:

with urllib.request.urlopen(req, timeout=timeout) as resp:

with None: raises TypeError: 'NoneType' object does not support the context manager protocol, so every POST fails, retries three times, and takes ~4s. The test waits at most 5s for _PENDING_THREADS to drain, which makes it flaky on slower machines.

More importantly, the test never exercises what its name and docstring describe. The POST never returns; it errors and gives up, so the eviction being verified happens on the failure path rather than the success path. That holds regardless of hardware.

Fix

Return a MagicMock with __enter__/__exit__ defined, matching the existing _ok_response() helper in tests/tracing/test_http_exporter_drops.py:51.

Related issues

None.

Validation

uv run pytest tests/tracing/test_flush_pending.py::test_pending_threads_self_evict_when_post_returns

Before: fails 4 of 5 runs, ~5s each. After: passes 5 of 5, ~0.07s each.

uv run pytest tests/tracing/ -q                      # 181 passed
uv run ruff format --check / ruff check              # clean
uv run pytest -m "not integration and not stress"    # 6334 passed (baseline 6332)

Two unrelated timing tests fail intermittently in my environment (a 4-vCPU VM); the set differs run to run and includes tests in modules this change doesn't touch.

Checklist

  • Code follows the project style (uv run ruff check . and uv run ruff format --check . pass)
  • Tests added/updated and passing (uv run pytest)
  • Docs updated if behavior or public APIs changed — no behavior change
  • New source files carry an SPDX license header — no new files

Signed-off-by: Jeriah Keith <jeriahkeithit@gmail.com>
@alessiodevoto alessiodevoto self-assigned this Jul 31, 2026
@alessiodevoto

Copy link
Copy Markdown
Collaborator

Hi @Yeriahz thanks for catching this! LGTM, merging!

@alessiodevoto
alessiodevoto merged commit abe177e into NVIDIA-NeMo:main Jul 31, 2026
5 checks passed
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