Skip to content

test(compat): autouse fixture + cross-adapter dedup test for brand_manifest warning#690

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-689-brand-manifest-dedup-test-hygiene
May 12, 2026
Merged

test(compat): autouse fixture + cross-adapter dedup test for brand_manifest warning#690
bokelley merged 1 commit into
mainfrom
claude/issue-689-brand-manifest-dedup-test-hygiene

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

Fixes #689. Test-hygiene follow-ups flagged on the #688 review.

  • Autouse fixture in tests/conftest.py clears adcp.compat.legacy.v2_5._url._brand_manifest_path_warned between every test. The 10 existing warning tests drop their monkeypatch.setattr(...) boilerplate. A future test that forgets the reset can no longer flake on inherited state.
  • New test_brand_manifest_dedup_is_cross_adapter documents the shared-state semantic introduced in Extend brand_manifest non-standard-path warning to inline-object branch #687 — same offending URL through get_products then create_media_buy warns exactly once per process. Catches a regression back to per-module dedup.

Verification

  • pytest tests/test_legacy_adapter_v2_5_*.py — 65 passed (was 64; +1 cross-adapter test, no net loss from the removed monkeypatch calls).
  • Manually simulated the per-module regression (_brand_manifest_path_warned.clear() between adapter calls): two warnings emitted, confirming the new test would catch that change.
  • ruff check + mypy clean.

Out of scope

Other nits flagged across this cluster but not addressed (intentional — small or theoretical):

  • Bounded dedup cache (_brand_manifest_path_warned is an unbounded set). Per-deployment cardinality of distinct brand_manifest URLs is small in practice; revisit if a real growth signal appears.
  • strip_url_scheme rename (now a bare-domain fallback for extract_brand_domain) — cosmetic.
  • IPv6 silent failure on extract_brand_domain ([::1]"::1" fails BrandReference.domain regex without typed error) — theoretical for brand_manifest URLs in practice.

…nifest warning

* Autouse fixture in tests/conftest.py clears the v2.5 ``_url._brand_manifest_path_warned``
  set between every test, removing the per-test ``monkeypatch.setattr(...)``
  boilerplate from the 10 existing warning tests. A future test that forgets
  the reset can no longer flake on inherited state.

* New ``test_brand_manifest_dedup_is_cross_adapter`` asserts that the same
  offending URL through ``get_products`` and then ``create_media_buy`` warns
  exactly once per process — documents the shared-state semantic introduced
  in #687 and catches a regression back to per-module dedup.

Fixes #689

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley
Copy link
Copy Markdown
Contributor Author

Expert review

code-reviewer: approved — no blockers. Confirmed:

  • .clear() vs monkeypatch.setattr(..., set()) semantic change is safe (state is only referenced via attribute lookup inside _url.py; no code caches the set by reference)
  • Cross-adapter test is correctly placed with an explicit regression-mode assertion message
  • Direct attribute access for the test-only reset is preferable to expanding the public API with a clear_dedup() helper

Nit (non-blocking): the autouse fixture lives at top-level conftest.py so it runs .clear() on every test in the suite. Cost is negligible; a scoped sub-conftest would be more surgical if compat tests ever get their own subdirectory.

CI: 15/15 green. Merging.

@bokelley bokelley merged commit 3a68c3f into main May 12, 2026
16 checks passed
@bokelley bokelley deleted the claude/issue-689-brand-manifest-dedup-test-hygiene branch May 12, 2026 10:06
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.

test: autouse fixture for brand_manifest warning dedup + cross-adapter dedup coverage

1 participant