test(integration): add poll_until predicate; fix inventory storage-location search flake

[lkubie]

What

Adds an optional predicate parameter to poll_until (tests/integration/utils/wait.py) and uses it in test_inventory_search_with_name_only_storage_location_filter.

Why

poll_until stops at the first non-empty result. The inventory storage-location test asserts exact set equality ({INVA..., INVB...}), so a partially indexed search page ended polling early and the assert failed intermittently — e.g. PR #776's Integration Tests (build 8615) failed with only one of two items found, while main passed the same test a minute earlier. Nothing in #776 touches inventory.

Linear: SDK-174

How

• poll_until(fetch, *, predicate=...): when given, polling continues until the predicate accepts the result (default behavior unchanged: any non-empty result).
• Both exact-set call sites in the inventory test now poll until the found id set equals the expected seeded set (still bounded by the 30s timeout, after which the assert reports the mismatch as before).
• Grep-verified these are the only two exact-set-after-poll sites in the integration suite.
• New offline unit tests for the helper in tests/unit/utils/test_wait.py (default behavior, predicate waiting, timeout fallback).

Testing

• uv run ruff format --check / uv run ruff check clean.
• uv run pytest tests/unit -q: 1123 passed, 12 xfailed (3 new).
• uv run pytest tests/integration --collect-only -q: 409 collected, no import errors.
• Integration behavior itself is exercised by this PR's Integration Tests job.

Cake session: https://agents.ai.albertinventdev.com/sessions/910a4725-9404-4336-81dd-e30bfef0ce73

[claude]

Code Review

Issues Found

• [tests/unit/utils/test_wait.py:1] The new unit test imports from tests.integration.utils.wait import poll_until, which violates a documented non-negotiable. tests/unit/TESTING.md explicitly prohibits imports from tests/integration/ and names poll_until by name in that banned list. Unit tests must not depend on the integration tree. Since poll_until is a pure, network-free helper, move it to a shared/unit-accessible location (e.g. under tests/unit/utils/ or a shared module) and import it from there, then unit-test it in place.

Summary
1 issue found. Test-only change; the poll_until logic (predicate wiring, default non-empty behavior, timeout return) is correct, but the new unit test breaks the unit/integration import boundary the repo enforces.

[lkubie]

Per Lenore: Rule 3 of tests/integration/TESTING.md now documents why search asserts flake (NoSQL + eventually consistent index, items index one by one), when to pass a predicate to poll_until (any complete-set/exact-equality assertion), and that a rare predicate timeout is a re-run-level event, not a code change. Checklist updated to match.

[claude]

Code Review

Issues Found

• tests/unit/utils/test_wait.py:1 from tests.integration.utils.wait import poll_until violates the documented unit-test boundary in AGENTS.md/OPINIONS.md: "No network, no env vars, no imports from tests/integration/". It passes today only because tests/integration/__init__.py and tests/integration/utils/__init__.py are empty; if either package init ever gains a session/env/network-dependent import, the offline unit suite breaks. Consider relocating wait.py to a shared test-utils location (e.g. tests/utils/) importable by both suites, or moving this test under tests/integration/.

Summary

1 issue found. Low severity: the change is correct and works now, but the new unit test crosses the unit/integration import boundary the repo explicitly forbids.

[lkubie]

Addressed the review: wait.py now lives at tests/utils/wait.py (shared by both suites) and the unit test imports from there. The old path keeps a one-line re-export shim so the 44 integration call sites — and every in-flight PR branch importing from it — keep working without conflicts. TESTING.md references updated to the shared location. Gates: ruff clean, 1123 unit passed, 409 integration collect.

[claude]

Looks good - no bugs or correctness issues found.

[lkubie]

Review feedback applied in commits a5866e8, 7e22b9c, 968869a.

A. Remaining flake sites — polls now stop only when the stop condition implies the assertion:

• test_inventory.py three-way created_by equality: each variant poll now carries predicate= scoped set == seeded_ids (every seed is created by static_user and matches text=seed_prefix), so by_id == by_name == by_user and test_item.id in by_id are deterministic once polling stops.
• test_inventory.py recently_created and search_hits: fetch filtered to test_item.id exactly; non-empty result implies the item is present (no more StopIteration path).
• test_parameter_groups.py: owner/tags/parameters search filtered to hit.id == pg.id.
• test_reports.py: filtered to hit.id == expected.id.
• test_product_design.py: filtered to hit.id == product.id.
• test_notebooks.py: filtered to hit.notebook_id == nb.id (kept the block_id condition).
• test_parameters.py (3 sites): kept as true complete-set assertions, added predicate=lambda results: len(results) == len(ids) (and the known_ids variant).

B. Import paths — all 18 integration callers now import from tests.utils.wait import poll_until; the tests/integration/utils/wait.py shim is deleted (package kept: __init__.py + metadata.py remain). Grep-verified zero references to the old path; also fixed the stale path reference in AGENTS.md.

C. Re-run guidance — TESTING.md now states the one rule (whatever stops the polling must imply the assertion that follows) and replaces the re-run paragraph: a timeout usually means indexing lag, but a repeat failure means the predicate or the code is wrong. Em dash removed (none remain in the file).

D. Unit tests — test_poll_until_returns_last_result_on_timeout now passes predicate=lambda result: False; added a growing-partials test asserting the LAST partial result is returned on timeout; added transient-error tests (raise-once-then-succeed recovers; always-raise re-raises after the deadline).

E. Transient errors — poll_until catches fetch exceptions, keeps polling until the deadline, and re-raises the last exception if the deadline expires on an erroring attempt. Signature unchanged; docstring documents the behavior.

Gates: uv sync OK; ruff format . --check OK (451 files); ruff check . OK; pytest tests/unit -q 1126 passed, 12 xfailed; pytest tests/integration --collect-only -q 409 collected.

Cake session: https://agents.ai.albertinventdev.com/sessions/910a4725-9404-4336-81dd-e30bfef0ce73

[claude]

Looks good - no bugs or correctness issues found.

[prasad-albert]

Approving. The predicate design is correct, and the "whatever stops the polling must imply the assertion that follows" rule is a great addition to TESTING.md. Two non-blocking comments:

1. Narrow the exception handling in poll_until (inline on tests/utils/wait.py).
2. tests/unit/TESTING.md:34-35 (not in this diff, so noting it here): the "never import from tests/integration/" rule still lists poll_until, which now lives in tests/utils/wait.py and is imported by the new unit test. Suggested update:

- **Import from `tests/integration/`** (`client`, `seeded_*`, `static_*`, `seed_prefix`,
  `seeding`). Shared, network-free helpers live in `tests/utils/` (e.g. `poll_until`).

Fine to address in a follow-up.

[prasad-albert]

Non-blocking: except Exception catches everything fetch raises, not just transient API errors. A lambda typo (AttributeError/NameError), a ValidationError from a model change, or a NotFoundError/401 now stalls for the full 30s timeout before surfacing, across ~43 call sites. The session's urllib3 Retry already retries 500/502/503/504 (src/albert/core/session.py:78-84), so consider narrowing to server errors:

Suggested change

	try:
	result = fetch()
	except Exception:
	if time.monotonic() >= deadline:
	raise
	try:
	result = fetch()
	except AlbertServerError:
	if time.monotonic() >= deadline:
	raise

(with from albert.exceptions import AlbertServerError). The docstring also mentions 429, which maps to AlbertClientError: either handle it deliberately or drop it from the docstring.

[prasad-albert]

Approving. The predicate design is correct, and the "whatever stops the polling must imply the assertion that follows" rule is a great addition to TESTING.md. Two non-blocking comments:

1. Narrow the exception handling in poll_until (inline on tests/utils/wait.py).
2. tests/unit/TESTING.md:34-35 (not in this diff, so noting it here): the "never import from tests/integration/" rule still lists poll_until, which now lives in tests/utils/wait.py and is imported by the new unit test. Suggested update:

- **Import from `tests/integration/`** (`client`, `seeded_*`, `static_*`, `seed_prefix`,
  `seeding`). Shared, network-free helpers live in `tests/utils/` (e.g. `poll_until`).

Fine to address in a follow-up.

@claude can you address these review comments?

[claude]

Claude finished @prasad-albert's task in 2m 32s —— View job

---

Review Comments Addressed

• Read relevant files for context
• Narrow exception handling in tests/utils/wait.py (except Exception → except AlbertServerError)
• Update tests/unit/TESTING.md to clarify import rule for poll_until
• Update unit tests to match narrowed exception type
• Commit and push changes

---

Changes made in commit 2367409:

tests/utils/wait.py

• Added from albert.exceptions import AlbertServerError
• Changed except Exception → except AlbertServerError so programming errors (AttributeError, ValidationError) and 4xx errors propagate immediately instead of stalling for the full 30s timeout
• Removed "429" from the docstring (429 maps to AlbertClientError, not AlbertServerError)

tests/unit/utils/test_wait.py

• Replaced RuntimeError with InternalServerError (via _make_server_error() helper) in the two transient-error tests to match the narrowed catch clause
• Added test_poll_until_does_not_catch_non_server_errors — asserts that a non-server error (AttributeError) propagates after exactly one call, not after the timeout

tests/unit/TESTING.md (lines 34-35)

• Removed poll_until from the banned tests/integration/ import list
• Added: "Shared, network-free helpers live in tests/utils/ (e.g. poll_until)."

Note: uv run commands required interactive approval in this environment; the changes are straightforward imports and exception narrowing. CI will validate.
| Branch

[claude]

Looks good - no bugs or correctness issues found.