refactor: add shared pubsub test fixtures and wait_for polling helper

[yashksaini-coder]

Summary

Introduces shared test infrastructure for the pubsub test suite restructuring (PR 1 of 6 per the plan in #378):

• tests/utils/pubsub/wait.py — wait_for() generic polling helper and wait_for_convergence() multi-node convergence helper, replacing inline definitions
• tests/core/pubsub/conftest.py — GossipSubHarness dataclass with .pubsubs, .hosts, .routers properties, plus gossipsub_nodes(), connected_gossipsub_nodes(), subscribed_mesh() context managers and a connected_gossipsub_pair fixture
• tests/core/pubsub/test_dummyaccount_demo.py — migrated to import wait_for_convergence from the shared module; removed 9 redundant @pytest.mark.trio decorators (covered by trio_mode = true in pyproject.toml)

This lays the foundation for subsequent PRs that will replace ~253 synchronization sleeps, standardize router extraction, flatten class-based tests, consolidate overlapping files, and tag slow tests.

Test plan

• All 9 tests in test_dummyaccount_demo.py pass
• All 395 pubsub tests collect successfully with the new conftest.py
• CI passes

Refs #378

cc @IronJam11 @seetadev

[Copilot]

Pull request overview

Adds shared pubsub test infrastructure to support the upcoming pubsub test suite refactor (per #378), centralizing synchronization helpers and common gossipsub fixtures/context managers.

Changes:

• Introduce tests/utils/pubsub/wait.py with reusable polling (wait_for) and multi-node convergence (wait_for_convergence) helpers.
• Add tests/core/pubsub/conftest.py with a GossipSubHarness wrapper plus context managers/fixtures for creating connected/subscribed gossipsub nodes.
• Refactor test_dummyaccount_demo.py to reuse the shared convergence helper and rely on global trio_mode = true instead of per-test @pytest.mark.trio.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/utils/pubsub/wait.py	New shared polling/convergence utilities for pubsub test synchronization.
tests/core/pubsub/conftest.py	New shared harness + context managers/fixtures for creating connected/subscribed GossipSub test topologies.
tests/core/pubsub/test_dummyaccount_demo.py	Swaps inline convergence helper for shared import; removes redundant trio markers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

wait_for() determines async-ness via inspect.iscoroutinefunction(predicate), which misses common patterns like lambda: async_fn() or functools.partial(async_fn, ...). In those cases predicate() returns a coroutine object (truthy) and the helper will return immediately without awaiting it, potentially causing false positives and "coroutine was never awaited" warnings. Consider calling predicate() each loop and then await if the returned value is awaitable (e.g., inspect.isawaitable(result)), and update the type hint accordingly to avoid the type: ignore.

[yashksaini-coder]

@copilot apply changes based on this feedback

[acul71]

Hello @yashksaini-coder Thanks for this PR.

AI PR Review: #1298

PR: refactor: add shared pubsub test fixtures and wait_for polling helper
Branch reviewed: refactor/378-pubsub-test-shared-fixtures (local checkout via gh pr checkout 1298)
Date: 2026-04-11

---

1. Summary of Changes

This PR introduces shared test infrastructure for the ongoing pubsub test-suite refactor tracked under #378 (meta-issue describing duplicated setup, ~253 sync sleeps, mixed router-extraction patterns, redundant @pytest.mark.trio, etc., per contributor comment on that issue).

What changed:

• tests/utils/pubsub/wait.py — Adds wait_for() (generic polling until a predicate is truthy) and wait_for_convergence() (multi-DummyAccountNode convergence), extracted from inline code previously in test_dummyaccount_demo.py.
• tests/core/pubsub/conftest.py — Adds GossipSubHarness, async context managers gossipsub_nodes, connected_gossipsub_nodes, subscribed_mesh, and fixture connected_gossipsub_pair, using PubsubFactory, dense_connect, and optional peer wait loops.
• tests/core/pubsub/test_dummyaccount_demo.py — Imports wait_for_convergence from the shared module and removes redundant @pytest.mark.trio markers (relying on global trio_mode).

Architecture: Changes are confined to tests under pubsub (tests/core/pubsub, tests/utils/pubsub). No production libp2p/ API changes.

Breaking changes: None.

Issue linkage: PR body says Closes #378. Issue #378 is the umbrella “revisit pubsub tests” item; the PR description positions this as “PR 1 of 6”, so closing the issue on merge may be premature (see §4 and §10).

---

2. Branch Sync Status and Merge Conflicts

Branch sync status

• Source: downloads/AI-PR-REVIEWS/1298/branch_sync_status.txt
• Raw: 0 4 from git rev-list --left-right --count origin/main...HEAD
• Interpretation: HEAD is 0 commits behind origin/main and 4 commits ahead — the branch includes 4 commits not yet on main and is up to date with main (not behind).

Merge conflict analysis

• Result: Test merge origin/main into HEAD completed with exit code 0.
• Log: downloads/AI-PR-REVIEWS/1298/merge_conflict_check.log records “NO MERGE CONFLICTS DETECTED” and merge was aborted cleanly.

✅ **No merge conflicts detected.** The PR branch can be merged cleanly into origin/main at the time of this review.

---

3. Strengths

• Aligns with Revisit high-level structure of pubsub tests after move to trio #378 plan: Centralizes wait_for / convergence helpers and introduces a typed GossipSubHarness plus reusable topology builders — exactly the direction described in the issue discussion.
• GossipSubHarness.routers uses a single, explicit isinstance + assert pattern instead of scattering three styles across files (future PRs can migrate call sites to this).
• connected_gossipsub_nodes uses trio.fail_after(peer_wait_timeout) around wait_for_peer, bounding peer discovery waits instead of unbounded sleeps.
• Test migration is minimal-risk: test_dummyaccount_demo.py behavior is preserved; removing redundant trio markers matches project config (trio_mode = true).
• Tooling: make lint, make typecheck, make test, and make linux-docs all succeeded locally on the reviewed branch (see §8).

---

4. Issues Found

Critical

None identified in the reviewed diff; full test suite passed locally.

Major

• File: tests/utils/pubsub/wait.py
  Line(s): 32–39
  Issue: wait_for() decides async vs sync using inspect.iscoroutinefunction(predicate). That does not treat callables that return a coroutine (e.g. lambda: async_fn(), functools.partial(async_fn, ...)) as async: predicate() can return a truthy coroutine object and return immediately without awaiting, risking false positives and “coroutine was never awaited” warnings. This matches Copilot’s review comment on the PR.
  Suggestion: Each iteration, call result = predicate(), then if inspect.isawaitable(result), result = await result (with appropriate typing). That removes the need for # type: ignore[misc] and matches the helper’s docstring (“Supports both sync and async predicates”) more faithfully.

tests/utils/pubsub/wait.py — lines 32–39

    _is_async = inspect.iscoroutinefunction(predicate)
    start = trio.current_time()
    last_exc: Exception | None = None

    while True:
        try:
            result = (await predicate()) if _is_async else predicate()  # type: ignore[misc]
            if result:
                return

• File: GitHub PR description — body
  Line(s): n/a
  Issue: Closes #378 on a “PR 1 of 6” change set likely closes the umbrella issue too early, removing visibility for the remaining refactor work unless maintainers explicitly want one mega-issue closed incrementally.
  Suggestion: Prefer Refs #378, Part of #378, or checklist items on Revisit high-level structure of pubsub tests after move to trio #378 until the series is done; or split child issues per PR and close those. Confirm with maintainers (pacrob, acul71, seetadev).

Minor

• File: tests/core/pubsub/conftest.py
  Line(s): 76–88
  Issue: subscribed_mesh still uses a fixed trio.sleep(settle_time) for “mesh formation.” Issue Revisit high-level structure of pubsub tests after move to trio #378 calls out replacing magic sleeps with polling where possible; for a first PR this is acceptable scaffolding but works against the stated end goal if copied widely without follow-up.
  Suggestion: In a follow-up, replace with wait_for-style polling on a mesh-ready predicate (or document settle_time as temporary with a # TODO(#378)).

• File: tests/core/pubsub/test_dummyaccount_demo.py
  Line(s): 38–39
  Issue: await trio.sleep(0.25) remains for “network creation” — same theme as Revisit high-level structure of pubsub tests after move to trio #378 (prefer condition-based waits when this file is next touched).
  Suggestion: Optional follow-up only; not blocking if left as-is for this PR’s scope.

---

5. Security Review

• Risk: None identified. Changes are test-only helpers and fixtures; no new network surface, crypto, or deserialization of untrusted input in production paths.
• Impact: None.
• Mitigation: N/A.

---

6. Documentation and Examples

• In-code: wait_for, wait_for_convergence, and context managers have docstrings suitable for test utilities.
• User-facing docs: Not required — no public API changes.
• Gap: If wait_for semantics change (awaitable results), update the docstring to state clearly that predicates may be sync, async, or return awaitables.

---

7. Newsfragment Requirement

Status: The PR diff contains no newsfragments/*.rst file.

• Linked issue: Revisit high-level structure of pubsub tests after move to trio #378 is referenced (Closes #378).
• Towncrier expectation: Per project practice (and this review prompt), a fragment such as newsfragments/378.internal.rst (or .misc.rst) with a short contributor-facing line about shared pubsub test fixtures would satisfy the requirement for internal/test-only work.

Blocker (process):

• Severity: CRITICAL / BLOCKER (per project merge rules for newsfragments)
• Issue: Missing newsfragment for the tracked issue.
• Impact: CI/release notes workflows that enforce towncrier may reject the PR until a fragment exists.
• Suggestion: Add newsfragments/378.internal.rst (or appropriate type) with a one-line ReST summary and trailing newline; align PR wording with whether Revisit high-level structure of pubsub tests after move to trio #378 should close on merge (see §4 Major).

---

8. Tests and Validation

Logs under downloads/AI-PR-REVIEWS/1298/:

Command	Log	Result
make lint	lint_output.log	Exit 0 — pre-commit hooks (yaml, toml, ruff, ruff format, mdformat, mypy, pyrefly, etc.) Passed
make typecheck	typecheck_output.log	Exit 0 — mypy and pyrefly Passed
make test	test_output.log	Exit 0 — 2610 passed, 15 skipped, 24 warnings in ~116s (pytest -n auto)
make linux-docs	docs_output.log	Exit 0 — Sphinx HTML build succeeded (sphinx-build -W)

Notes:

• Warnings: Summary reports 24 warnings; xdist output did not surface a detailed warning list in the captured tail — no failures attributed to this PR.
• Skipped: 15 tests skipped (project-wide; not specific to this diff).

---

9. Recommendations for Improvement

1. Implement awaitable-result handling in wait_for() (§4 Major) before broad adoption across the suite.
2. Add newsfragments/378.<type>.rst and reconcile Closes #378 vs multi-PR tracking (§4, §7).
3. Plan subscribed_mesh to move from sleep(settle_time) to predicate-based waiting in a later Revisit high-level structure of pubsub tests after move to trio #378 PR.

---

10. Questions for the Author

1. Issue closure: Should Revisit high-level structure of pubsub tests after move to trio #378 remain open until all six planned PRs land? If yes, please change the PR description from Closes Revisit high-level structure of pubsub tests after move to trio #378 to Refs Revisit high-level structure of pubsub tests after move to trio #378 (or use sub-issues per PR).
2. wait_for semantics: Will you adopt the isawaitable pattern so callers can pass lambda: some_async() safely?
3. Copilot follow-up: GitHub shows a follow-up commit touching conftest.py; does the final branch still need any hidden Unicode cleanup, or was that resolved? (Current local conftest.py is ASCII-only.)

---

11. Overall Assessment

Dimension	Assessment
Quality rating	Good — structure matches #378; one robustness gap in wait_for and process items (newsfragment, issue closing) remain.
Security impact	None
Merge readiness	Needs fixes — add newsfragment; strongly recommend wait_for fix and PR/issue linkage cleanup before merge per maintainer bar.
Confidence	High for test behavior (full suite green locally); Medium on repo policy for closing #378 with only PR 1.

[IronJam11]

Findings (ordered by severity)

1. Minor: subscribed mesh setup still relies on fixed sleep.

   • Evidence: tests/core/pubsub/conftest.py:76-89
   • Details: subscribed_mesh waits a hard-coded settle_time via trio.sleep(settle_time) before yielding. This can still be flaky under variable CI timing, even though a TODO is present.
   • Recommendation: In a follow-up PR, replace with a predicate-based wait helper (e.g., wait_for on expected mesh state), then keep settle_time only as a fallback override.

2. Minor: connected topology readiness check only validates one peer per node.

   • Evidence: tests/core/pubsub/conftest.py:67-71
   • Details: after dense_connect, connected_gossipsub_nodes waits for each node to observe only its next ring neighbor. This is likely sufficient for smoke readiness but may not fully assert all dense links are visible.
   • Recommendation: Optional follow-up: verify all expected peers (or expected minimum degree) when this fixture is reused for stricter tests.
     @yashksaini-coder

[IronJam11]

Other than that, keeping luca's concerns in mind (as in the review above), it looks well drafted and well coded. well done @yashksaini-coder

[yashksaini-coder]

Thanks @acul71 for the thorough review and @IronJam11 for the follow-up — all feedback has been addressed:

• wait_for() fix — switched from inspect.iscoroutinefunction(predicate) to inspect.isawaitable(result) so lambdas and functools.partial returning coroutines are properly awaited. Removes the # type: ignore as well.
• Newsfragment — added newsfragments/378.internal.rst.
• Issue linkage — changed Closes #378 → Refs #378 since this is PR 1 of 6.
• settle_time sleep — added TODO(#378) comment. Opened a dedicated follow-up issue for replacing it with predicate-based polling: refactor: replace fixed sleeps in pubsub test fixtures with predicate-based polling #1307.

All changes are in commit 77dbab8. Could you give this a final look when you get a chance? Happy to address anything else before merge.

[acul71]

Hi @yashksaini-coder thanks, all changes looks good.

This can be merged @seetadev .
One minor improvement can be done optionally for
connected_gossipsub_nodes

• Would you like connected_gossipsub_nodes to remain lightweight by default, with a separate strict fixture for full peer-visibility checks?

Full review here:

AI PR Review: #1298

PR: refactor: add shared pubsub test fixtures and wait_for polling helper
Branch reviewed: refactor/378-pubsub-test-shared-fixtures
Date: 2026-04-21

---

1. Summary of Changes

This PR introduces shared pubsub test infrastructure for issue #378, focused on improving maintainability and reducing flaky synchronization patterns in tests.

• Adds tests/utils/pubsub/wait.py with reusable wait_for() and extracted wait_for_convergence() polling helpers.
• Adds tests/core/pubsub/conftest.py with GossipSubHarness and shared async context managers/fixture (gossipsub_nodes, connected_gossipsub_nodes, subscribed_mesh, connected_gossipsub_pair).
• Refactors tests/core/pubsub/test_dummyaccount_demo.py to import shared helpers and removes redundant @pytest.mark.trio decorators.
• Adds newsfragments/378.internal.rst.

The changes are test-focused (tests/core/pubsub, tests/utils/pubsub) plus one internal newsfragment; there are no production libp2p/ runtime behavior changes and no breaking API changes.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• Status: ℹ️ Ahead of origin/main
• Details: branch_sync_status.txt shows 0 9 from git rev-list --left-right --count origin/main...HEAD, meaning the branch is 0 commits behind and 9 commits ahead of origin/main.

Merge Conflict Analysis

• Conflicts detected: ✅ No conflicts
• merge_conflict_check.log records Already up to date. and === NO MERGE CONFLICTS DETECTED ===.

✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/main.

---

3. Strengths

• Good extraction of repeated pubsub setup/synchronization code into shared helpers, which directly aligns with the maintainability goals in Revisit high-level structure of pubsub tests after move to trio #378.
• wait_for() now correctly handles awaitable-returning predicates via inspect.isawaitable(result), fixing the earlier coroutine-handling concern.
• GossipSubHarness gives a consistent typed access pattern for .pubsubs, .hosts, and .routers.
• Maintainer/community feedback visible on the PR thread appears to have been addressed (issue linkage changed to Refs #378, wait helper fixed, newsfragment added).
• Local validation is strong: lint, typecheck, tests, and docs commands all exited successfully.

---

4. Issues Found

Critical

None.

Major

None.

Minor

• File: tests/core/pubsub/conftest.py
• Line(s): 67–71
• Issue: In connected_gossipsub_nodes, readiness is validated using exactly one target peer per node ((index + 1) % n) after dense_connect(). This confirms that each node can see at least one expected neighbor, but it does not confirm that every expected dense connection is visible before yielding.
• Why this matters: Most tests only need a minimally healthy topology to begin, so this is often fine and keeps setup fast. However, tests that depend on fully established mesh state (for example, tests that assert exact peer counts, full fanout behavior, or immediate all-to-all propagation assumptions) can become intermittently flaky if fixture return happens before all peer views converge.
• Examples:
  • Current check passes, but topology is still incomplete: with 4 nodes, each node might only have discovered the "next" peer in the ring at the moment of check. The fixture yields, and a test expecting broader connectivity may race with ongoing peer discovery.
  • When current behavior is sufficient: smoke-style tests where early connectivity is enough and assertions naturally tolerate eventual convergence.
  • When stricter behavior helps: deterministic topology-sensitive tests where assertions assume all nodes have observed all other expected peers before any publish/assert sequence starts.
• Suggestion: Keep the current lightweight default for speed, and optionally add a stricter variant (or a flag) that waits for full expected peer visibility when needed. This preserves fast setup for most tests while giving stronger guarantees to topology-sensitive tests.

tests/core/pubsub/conftest.py — lines 67-71

        if n > 1:
            with trio.fail_after(peer_wait_timeout):
                for index, pubsub in enumerate(harness.pubsubs):
                    target_host = harness.hosts[(index + 1) % n]
                    await pubsub.wait_for_peer(target_host.get_id())

---

5. Security Review

• Risk: No direct security issue identified.
• Impact: Low (test-only changes, no new production input surface or crypto/transport behavior changes).
• Mitigation: N/A.

---

6. Documentation and Examples

• User-facing docs are not required for this PR scope (test infrastructure/internal cleanup).
• Utility docstrings are present and clear in new helper modules.
• Newsfragment is present and correctly scoped as internal (newsfragments/378.internal.rst).

---

7. Newsfragment Requirement

• ✅ Present: newsfragments/378.internal.rst
• ✅ Filename follows required format <ISSUE_NUMBER>.<TYPE>.rst
• ✅ Content is user/contributor-facing and concise
• ✅ PR references an issue (Refs #378)

No blocker found in this section.

---

8. Tests and Validation

Artifacts reviewed from downloads/AI-PR-REVIEWS/1298/:

• lint_output.log: all listed checks passed (check yaml, check toml, ruff, ruff format, mdformat, mypy, pyrefly, path audit).
• typecheck_output.log: mypy and pyrefly passed.
• test_output.log: 2659 passed, 15 skipped in 105.99s.
• docs_output.log: make linux-docs command exited 0; documentation build completed without fatal errors.

No validation blockers were observed.

---

9. Recommendations for Improvement

1. Consider adding a stricter topology-readiness variant for fixtures that need full mesh visibility guarantees.
2. Continue follow-up work from Revisit high-level structure of pubsub tests after move to trio #378 to replace remaining synchronization sleeps with condition-based waits where practical.

---

10. Questions for the Author

1. Would you like connected_gossipsub_nodes to remain lightweight by default, with a separate strict fixture for full peer-visibility checks?
2. For the Revisit high-level structure of pubsub tests after move to trio #378 follow-up sequence, is refactor: replace fixed sleeps in pubsub test fixtures with predicate-based polling #1307 the canonical tracker for replacing subscribed_mesh fixed settling sleep?

---

11. Overall Assessment

• Quality Rating: Good
• Security Impact: None
• Merge Readiness: Ready (minor non-blocking improvement opportunity noted)
• Confidence: High

[yashksaini-coder]

Thanks @acul71 for the review.

On the minor improvement

Went with option (a) — kept the default lightweight and added an opt-in strict=True keyword. Pushed as 146d36a:

async with connected_gossipsub_nodes(4, strict=True) as harness:
    # every node has observed every other expected peer before we yield

When strict=True, the fixture waits for each pubsub to see every other expected host (N*(N-1) wait_for_peer calls under the existing peer_wait_timeout). Default strict=False preserves the current ring-style one-neighbour check, so no test behaviour changes unless a caller opts in. Same peer_wait_timeout envelope is used for both paths.

Answering your questions

1. Would you like connected_gossipsub_nodes to remain lightweight by default, with a separate strict fixture for full peer-visibility checks?

Yes — went with a keyword-only strict flag on the same fixture rather than a second fixture, to avoid forking the context-manager surface. Easy to split into a dedicated fixture later if a caller prefers that shape.

2. For the Revisit high-level structure of pubsub tests after move to trio #378 follow-up sequence, is refactor: replace fixed sleeps in pubsub test fixtures with predicate-based polling #1307 the canonical tracker for replacing subscribed_mesh fixed settling sleep?

Yes — #1307 is the canonical tracker for that item. The TODO(#378) on line 87 of tests/core/pubsub/conftest.py will be resolved there.

Validation

• pytest tests/core/pubsub/test_dummyaccount_demo.py — 9 passed in 30s (no regressions; the existing callers all use the default non-strict path)
• make lint — clean (ruff, mypy, pyrefly)

Happy to squash/rebase before @seetadev merges.