Skip to content

fix(ci): green main — pyvis.shiny unit guard + de-flake leverage/simulation e2e#10

Merged
razinkele merged 3 commits into
mainfrom
fix/ci-unit-pyvis-shiny-guard
Jul 2, 2026
Merged

fix(ci): green main — pyvis.shiny unit guard + de-flake leverage/simulation e2e#10
razinkele merged 3 commits into
mainfrom
fix/ci-unit-pyvis-shiny-guard

Conversation

@razinkele

@razinkele razinkele commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Why

main CI has been red on every push since ~2026-07-01. Two independent causes:

  1. Unit-test jobs (py3.11 + py3.12) fail deterministically at collection with
    ModuleNotFoundError: No module named 'pyvis.shiny' on tests/test_cld.py and
    tests/test_cld_filter.py. Those modules import sespy.modules.cld_visualization /
    analysis_* at top level, which import the pyvis fork at load. The unit jobs install
    stock pyvis from PyPI (by design — fork tests belong in the conda jobs), so collection
    crashes. The two files simply missed the importorskip guard that test_network /
    test_report already use.
  2. Playwright e2e job is flakytest_leverage_e2e hard-asserted it caught a
    transient "Computing…" caption (racy), and separately read window.pyvisNetworks[...]
    after a fixed 2.5s sleep (crashed on undefined.nodes when the network hadn't
    registered). test_simulation_e2e had the same arbitrary-timeout-then-assert pattern.

What

  • test_cld.py / test_cld_filter.py: add module-level pytest.importorskip("pyvis.shiny").
    Skips cleanly in the stock-pyvis unit jobs; still runs for real in the conda full-app/e2e jobs.
  • test_simulation_e2e.py: replace wait_for_timeout→assert with wait_for_selector on the
    plot images (and run button), matching the Monte-Carlo poll already in the file.
  • test_leverage_e2e.py: downgrade the transient-caption sighting to best-effort (log on miss;
    the load-bearing 95% CI-column assertion is kept and already polled), and replace the pyvis
    network sleep with wait_for_function waits.

Verification (local)

  • pyvis guard: reproduced stock-pyvis env by blocking pyvis.shiny2 skipped, not errored;
    fork present → 5 passed; ruff clean.
  • simulation e2e: 3/3 green (was failing on a slower machine).
  • leverage e2e: 4/4 green (was ~1/2; caught + fixed a second race during verification).
  • full unit suite (fork env): 474 passed, 1 skipped.

Not reproducible locally: the exact CI unit env (ubuntu + stock pyvis) and the fast-runner e2e
timing — this PR exists to confirm those go green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Improved end-to-end test reliability by waiting for screens and results to load before checking them.
    • Made some test suites skip automatically when an optional visualization dependency isn’t available, reducing false failures in limited environments.
    • Loosened one transient loading-state check so tests focus on the final rendered result instead of brief intermediate states.

razinkele and others added 3 commits July 2, 2026 16:17
test_cld.py and test_cld_filter.py import sespy.modules.cld_visualization /
analysis_* at module top, which import the pyvis fork (`pyvis.shiny`) at load.
In the stock-pyvis unit-CI jobs (py3.11 + py3.12) that raised
ModuleNotFoundError at collection, failing the whole job. The other fork tests
(test_network, test_report) already skip cleanly via importorskip; these two
missed the guard. Add a module-level importorskip so they skip when the fork is
absent and still run for real in the conda full-app/e2e jobs. E402 is already
ignored for tests/ for exactly this idiom.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eeps)

The trajectory and final-state plot checks did wait_for_timeout(...) then
asserted the <img> was present, so a render slower than the guessed delay
(under full-suite / CI load) failed the assert prematurely — it flaked on a
slower machine, failing at 3 different lines across reruns. Replace the
sleep-then-assert pairs with wait_for_selector on the plot images (and the run
button), matching the Monte-Carlo poll already used lower in the same file.
Verified 3/3 green locally.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ace)

Two timing races made this the CI-red e2e:

1. It hard-asserted it *caught* the transient "Computing…" caption by polling.
   The MC run on the 17-node/n=50 sample can finish between polls (esp. on a
   fast CI runner), so the sighting is inherently racy. Downgrade to best-effort
   (log on miss); the load-bearing guarantee is the 95% CI column appearing,
   which is already polled and still hard-asserted.
2. It read window.pyvisNetworks['leverage-leverage_network'].nodes after a fixed
   2.5s sleep; when the network hadn't registered yet it crashed on
   undefined.nodes. Replace the sleep with wait_for_function on nav-active and on
   the network object existing.

Verified 4/4 green locally (was ~1/2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Test modules test_cld.py and test_cld_filter.py now skip entirely when the pyvis.shiny dependency is unavailable. E2E tests for Leverage Points and Dynamic Simulation replace fixed timeouts with polling/selector-based waits and relax a transient "Computing" caption assertion to a warning.

Changes

Test Robustness Updates

Layer / File(s) Summary
pyvis.shiny import guards
tests/test_cld.py, tests/test_cld_filter.py
Adds module-level pytest.importorskip("pyvis.shiny") with explanatory comments so these modules skip when the pyvis fork is missing.
Leverage Points E2E polling
tests/test_leverage_e2e.py
Replaces fixed wait_for_timeout with polling for nav button activation and pyvis network/node readiness; makes the "Computing" caption check best-effort with a warning instead of a hard failure.
Simulation E2E selector waits
tests/test_simulation_e2e.py
Replaces fixed timeouts and JS DOM checks with wait_for_selector for trajectory, final-state, and Monte Carlo histogram plot images.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the CI-focused pyvis.shiny test guard and e2e flake fixes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ci-unit-pyvis-shiny-guard

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_leverage_e2e.py (1)

17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor redundancy: double-checking nav activation.

wait_for_function (Line 18-23) already asserts the exact same condition (.sespy-nav-btn.active has exactly one match equal to sespy_nav_leverage) that the follow-up eval_on_selector_all + assert (Line 24-27) re-verifies. Not incorrect, just a redundant round trip.

♻️ Optional simplification
-        await page.wait_for_function(
-            "() => { const a = Array.from(document.querySelectorAll("
-            "'.sespy-nav-btn.active')).map(e => e.id);"
-            " return a.length === 1 && a[0] === 'sespy_nav_leverage'; }",
-            timeout=15000,
-        )
-        nav_active = await page.eval_on_selector_all(
-            ".sespy-nav-btn.active", "els => els.map(e => e.id)"
-        )
-        assert nav_active == ["sespy_nav_leverage"], f"unexpected: {nav_active}"
+        nav_active = await page.wait_for_function(
+            "() => { const a = Array.from(document.querySelectorAll("
+            "'.sespy-nav-btn.active')).map(e => e.id);"
+            " return (a.length === 1 && a[0] === 'sespy_nav_leverage') ? a : null; }",
+            timeout=15000,
+        )
+        assert (await nav_active.json_value()) == ["sespy_nav_leverage"]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_leverage_e2e.py` around lines 17 - 27, The nav activation check in
the leverage e2e test is duplicated: wait_for_function already verifies that
.sespy-nav-btn.active contains exactly sespy_nav_leverage, so the follow-up
eval_on_selector_all and assert in the same test are redundant. Simplify the
test by keeping the wait_for_function in the leverage flow and removing the
extra selector readback/assert, or otherwise consolidate the check into a single
validation using the existing page wait logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_leverage_e2e.py`:
- Around line 17-27: The nav activation check in the leverage e2e test is
duplicated: wait_for_function already verifies that .sespy-nav-btn.active
contains exactly sespy_nav_leverage, so the follow-up eval_on_selector_all and
assert in the same test are redundant. Simplify the test by keeping the
wait_for_function in the leverage flow and removing the extra selector
readback/assert, or otherwise consolidate the check into a single validation
using the existing page wait logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dd351d48-0603-4a48-86f0-8cbef7a4ed1c

📥 Commits

Reviewing files that changed from the base of the PR and between 57ae262 and 55e0650.

📒 Files selected for processing (4)
  • tests/test_cld.py
  • tests/test_cld_filter.py
  • tests/test_leverage_e2e.py
  • tests/test_simulation_e2e.py

@razinkele razinkele merged commit 4bd15d7 into main Jul 2, 2026
6 checks passed
@razinkele razinkele deleted the fix/ci-unit-pyvis-shiny-guard branch July 2, 2026 15:29
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.

1 participant