From b51021581e814280474aa52ba9ff3744f86e466d Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 10:32:20 +0530 Subject: [PATCH 01/35] Stop visual baselines from encoding broken pages 8 of 12 authenticated baselines captured something other than the page they named: a 404 (the route /evaluation/288 does not exist), stuck "Loading ..." curtains, a false-empty list, and four byte-identical copies of the logged-out marketing homepage (bug #15). Most sat under hard pixel-diff assertions, so they passed *because* the app was broken - ai_maker_dashboard would have gone red the day bug #14 was fixed. The root cause was the wait, not the missing check: captures settled a flat 2.5s after load while lessons.md documents curtains at 15-30s+, so they landed mid-curtain by design. Adds wait_for_render_settled (45s) plus a gate that refuses to save or compare a capture that is a 404, a curtain, logged-out chrome on an auth route, or blank. It skips rather than fails: an invalid capture means "did the pixels change?" was never asked, so the honest result is no signal. Also fixes the evaluation-detail route to use the completed_eval_id fixture, and masks the account name/initials that every authenticated baseline was embedding. --- tests/visual/test_visual_regression.py | 101 ++++++++++++- utils/visual_guards.py | 196 +++++++++++++++++++++++++ 2 files changed, 292 insertions(+), 5 deletions(-) create mode 100644 utils/visual_guards.py diff --git a/tests/visual/test_visual_regression.py b/tests/visual/test_visual_regression.py index 0dc393a..9726d4b 100644 --- a/tests/visual/test_visual_regression.py +++ b/tests/visual/test_visual_regression.py @@ -15,6 +15,7 @@ from playwright.sync_api import Page from utils.config import Config +from utils.visual_guards import capture_integrity_problem, wait_for_render_settled pytestmark = [pytest.mark.visual] @@ -90,12 +91,22 @@ def _compare_or_save_baseline( def _capture_page(page: Page, url: str) -> Image.Image: - """Navigate to url and return a PIL Image of the full page screenshot.""" + """Navigate to url and return a PIL Image of the full page screenshot. + + Skips when the capture fails an integrity check (404 / stuck curtain / + blank) so a bad render is never written to a baseline. The homepage-fallback + check is off here — these are the public pages, so that content is expected. + """ page.goto(url, wait_until="networkidle", timeout=30_000) page.wait_for_timeout(1_000) # allow fonts/animations to settle raw = page.screenshot(full_page=True) import io - return Image.open(io.BytesIO(raw)) + img = Image.open(io.BytesIO(raw)) + + problem = capture_integrity_problem(page, img) + if problem: + pytest.skip(f"Capture integrity check failed for {url}: {problem}") + return img def _page_at_viewport(browser, width: int, height: int) -> Page: @@ -137,6 +148,10 @@ def _capture_page_masked(page: Page, url: str, masks: list) -> Image.Image: instead. """ page.goto(url, wait_until="load", timeout=30_000) + # Wait out the auth/data curtains before settling — a flat 2.5s here was + # capturing mid-curtain, which is how stuck "Loading …" states ended up + # saved as baselines. See utils.visual_guards.wait_for_render_settled. + wait_for_render_settled(page) page.wait_for_timeout(2_500) mask_locators = [] for sel in masks: @@ -158,6 +173,40 @@ def _capture_page_masked(page: Page, url: str, masks: list) -> Image.Image: "[class*='polling']", ] +# Personal data rendered by the logged-in shell. Masked on every authenticated +# capture for two reasons: +# 1. Privacy — these baselines embed the real test account's full name +# ("Welcome, ") and avatar initials. `snapshots/` is +# gitignored so they never reach git history, but CI both caches the +# directory and uploads it as a 30-day build artifact +# (.github/workflows/ci.yml), so the images are retrievable by anyone with +# repo access. +# 2. Portability — an unmasked name pins every baseline to one account, so +# re-running the suite as TEST_USER_2 would diff on the sidebar alone. +# Deliberately NOT matching initials literally: locators/workspace_locators.py +# pins `text=MSM` while this account renders "SM", confirming initials vary per +# account. Anchored `^Welcome,` is used rather than +# AIMakerLocators.WELCOME_MESSAGE's bare `text=Welcome`, which would also match +# body copy containing the word. `_capture_page_masked` drops zero-count +# selectors, so listing extras is free. +# Verified against the live DOM 2026-08-12: +#
← masked (the whole identity block) +#
SM
← initials, NO avatar class +#

Welcome, Saqib Manan

+# Switch Roles +#
+# The sidebar initials circle is a bare Tailwind div, so `[class*='avatar' i]` +# (which matches only the 2 header avatars) does not cover it. Masking the +# parent block catches the circle and the name together. That also masks the +# "Switch Roles" link — an accepted trade: a small, static control loses pixel +# coverage so no capture carries identity. +_AUTH_PII_MASKS = [ + "[class*='avatar' i]", # header avatar circle(s) rendering initials + ".welcome-text >> xpath=..", # sidebar identity block (circle + name) + "text=/^Welcome,/ >> xpath=..", # fallback if .welcome-text is renamed + "button[aria-label='Open profile']", # per locators/dashboard_locators.py +] + # Auth routes whose content is genuinely non-deterministic between two captures # against the live/shared dev environment: the New Evaluation wizard's model # dropdown loads asynchronously (~35s) and prefills a live timestamp; the @@ -336,7 +385,16 @@ class TestAuthenticatedPageVisuals: ("/dashboard/auditor", "auditor_dashboard"), ("/dashboard/auditor/assignments", "auditor_assignments"), ("/dashboard/auditor/evaluations", "auditor_evaluations"), - ("/evaluation/288", "evaluation_detail_completed"), + # `{eval_id}` is substituted at runtime from the session-scoped + # `completed_eval_id` fixture. This route previously read + # "/evaluation/288" — a path that does not exist in the frontend app + # router (the real shape is + # /dashboard/ai-maker/[orgId]/evaluations/[evaluationId]), so every run + # captured a 404 and then hard-asserted that 404 against itself. The + # hardcoded 288 is the same drifting-constant pattern conftest's + # `completed_eval_id` fixture was written to retire; this was the last + # caller still using it. + ("/dashboard/ai-maker/1/evaluations/{eval_id}", "evaluation_detail_completed"), ] @pytest.mark.parametrize( @@ -345,19 +403,52 @@ class TestAuthenticatedPageVisuals: @pytest.mark.auth @pytest.mark.regression def test_authenticated_page_desktop( - self, browser, authenticated_storage_state, path, name + self, request, browser, authenticated_storage_state, path, name ): + if "{eval_id}" in path: + # Resolved lazily so the (session-scoped, potentially seeding) + # lookup only runs for the one route that needs it. + # + # `completed_eval_id` skips cleanly when it simply finds no COMPLETED + # audit, but its UI-seeding fallback can also raise outright (observed + # 2026-08-12: Playwright TimeoutError on + # input[name='evaluatorType'][value='Technical']). That is a data-fixture + # failure, not a visual one — letting it fail here would make the visual + # suite red for a reason no baseline can express. Same rationale as the + # capture-integrity gate: no valid subject means no signal, so skip. + # pytest.skip raises from BaseException, so a genuine skip still passes + # through this handler untouched. + try: + eval_id = request.getfixturevalue("completed_eval_id") + except Exception as exc: # noqa: BLE001 + pytest.skip( + "Could not resolve a COMPLETED evaluation to capture: " + f"{type(exc).__name__}: {exc}. The `completed_eval_id` fixture's " + "discovery found no match and its seeding fallback failed — a " + "fixture/seeding issue, not a visual regression." + ) + path = path.format(eval_id=eval_id) + page = _authenticated_page_at_viewport( browser, authenticated_storage_state, 1440, 900 ) try: try: - img = _capture_page_masked(page, Config.url(path), _DEFAULT_MASKS) + img = _capture_page_masked( + page, Config.url(path), _DEFAULT_MASKS + _AUTH_PII_MASKS + ) except Exception as exc: # noqa: BLE001 pytest.skip( f"Could not capture {path}: {exc}. " "Page may be unreachable for this account." ) + + # Never save or diff a capture that isn't the page under test — + # see utils/visual_guards for why this skips rather than fails. + problem = capture_integrity_problem(page, img, expect_auth_route=True) + if problem: + pytest.skip(f"Capture integrity check failed for {path}: {problem}") + if name in _NON_DETERMINISTIC_VISUAL: try: _compare_or_save_baseline(img, f"auth_{name}_desktop_1440x900") diff --git a/utils/visual_guards.py b/utils/visual_guards.py new file mode 100644 index 0000000..6e09259 --- /dev/null +++ b/utils/visual_guards.py @@ -0,0 +1,196 @@ +"""Capture-integrity guards for visual regression tests. + +Why this exists +--------------- +`_compare_or_save_baseline` treats whatever is on screen as truth: if no +baseline exists it saves the capture verbatim. That is only safe if the capture +is actually the page we asked for — and against the shared dev environment it +frequently is not. Three documented failure modes all render a *plausible-looking* +page that is not the one under test: + +* bug #13 / #14 — the route hangs on a "Loading …" curtain that never resolves. +* bug #15 — a deep-link to an auth route intermittently renders the **public + marketing homepage** (~17-25% of loads) while the session is still valid. +* a wrong/stale route simply 404s (this is how `/evaluation/288` came to have a + 404 saved as the "completed evaluation detail" baseline). + +A baseline captured during any of those bakes the defect in permanently, and the +resulting test is worse than no test: `auth_ai_maker_dashboard` had a stuck +"Loading overview..." saved as its baseline under a *hard* pixel-diff assertion, +so it passed only while the app stayed broken and would have gone red the day the +product was fixed. + +skip vs fail +------------ +These guards **skip** rather than fail, in both the save and the compare path. +The reasoning is deliberate: + +A visual regression test answers exactly one question — "did the pixels change?" +— and it can only answer that from a valid capture. A 404, a stuck curtain or a +homepage fallback means the question was never asked, so the honest result is +*no signal*, which is a skip. Failing would assert a pixel regression that was +never observed; passing (today's behaviour) silently certifies a page nobody +looked at. + +Skips are visible: pytest tallies them and `reports/TEST_REPORT.md` lists them +with the reason string, so a route that stops rendering surfaces as a growing +skip count rather than a green tick. + +"But a stuck curtain IS a regression" — agreed, and it is already owned by +suites better suited to it: the e2e and accessibility suites assert on page +content and track these exact states as bugs #13/#14 with explicit xfails. +Detecting "the page failed to load" via a full-page pixel diff would duplicate +that concern with a far noisier signal. +""" + +from __future__ import annotations + +import re +import time + +from PIL import Image +from playwright.sync_api import Page + +# Next.js' built-in not-found page, plus the bare status line some routes emit. +_NOT_FOUND_MARKERS = ( + "this page could not be found", + "404: this page could not be found", +) + +# Loading curtains are matched with a trailing ellipsis rather than a bare +# "Loading" so legitimate copy (a column header, a tooltip) can't trip the +# guard. Covers both ASCII "..." and the unicode "…" the app uses in places. +# Real examples: "Loading overview...", "Loading evaluations…", +# "Loading AI models...", "Verifying your session...". +_CURTAIN_RE = re.compile( + # "Loading overview...", "Loading your assignments...", "Loading evaluations…" + r"(Loading[^\n]{0,40}?(\.\.\.|…))" + # A bare, standalone "Loading" line — the org-selector spinner renders exactly + # this with no ellipsis, and slipped past an ellipsis-only pattern into a + # saved baseline. Anchored to a whole line so prose ("Downloading report", + # "Reload the page") still can't trip it. + r"|(^[ \t]*Loading[ \t]*$)" + r"|(Verifying your session)", + re.I | re.M, +) + +# Copy that only exists on the public marketing homepage. If any of these show +# up while we are sitting on a /dashboard route, we caught bug #15's fallback. +_HOMEPAGE_ONLY_MARKERS = ( + "build ai that's trustworthy from day one", + "automation-assisted evaluation environment", + "expert-led evaluations", + "sector-specific test cases", +) + +# Logged-out chrome. A far more reliable tell than the hero copy: the marketing +# nav renders LOGIN/SIGN UP, which the authenticated shell never does. Four of +# the twelve auth baselines turned out to be pixel-identical to the logged-out +# homepage, so this is the single highest-value check in this module. +_LOGGED_OUT_MARKERS = ( + "sign up", + "login", + # An expired/absent session bounces the route to the NextAuth provider + # picker, which renders only this string — confirmed live 2026-08-12. + "sign in with keycloak", +) + +# A capture this uniform is a blank/white screen, not a rendered page. Set +# deliberately high: several real pages (empty states, the role selector) are +# mostly flat background, and a false "blank" verdict would suppress a valid +# baseline. 99.8% single-colour is well past anything that renders content. +_UNIFORM_RATIO = 0.998 + + +def _body_text(page: Page) -> str: + """Visible body text, or "" if the page is in a state that can't yield it.""" + try: + return page.locator("body").inner_text(timeout=5_000) + except Exception: # noqa: BLE001 — a page we can't read is handled by callers + return "" + + +def _is_near_uniform(img: Image.Image) -> bool: + """True when almost every pixel is the same shade — i.e. a blank capture.""" + histogram = img.convert("L").histogram() + total = sum(histogram) + if total == 0: + return True + return (max(histogram) / total) >= _UNIFORM_RATIO + + +def wait_for_render_settled(page: Page, timeout_ms: int = 45_000) -> None: + """Block until no loading curtain is on screen (or the budget runs out). + + `_capture_page_masked` previously settled for 2.5s after `load`. Against dev + that is far too short: lessons.md records the session curtain at ~15-20s on a + cold deep-link and route data curtains at 30s+. Screenshotting at 2.5s + therefore captured a curtain most of the time — which is the mechanism behind + the poisoned baselines this module exists to prevent. The integrity gate + alone would just convert that into a permanent skip; waiting properly is what + lets a good baseline actually be produced. + + Returns on timeout rather than raising: the caller's integrity check is the + thing that decides what a still-curtained page means. + """ + deadline = time.monotonic() + (timeout_ms / 1000) + while time.monotonic() < deadline: + text = _body_text(page) + # An empty body means the SPA hasn't mounted yet, not that it's settled + # — returning here would reintroduce the mid-curtain capture this + # function exists to prevent. + if text.strip() and not _CURTAIN_RE.search(text): + return + page.wait_for_timeout(1_000) + + +def capture_integrity_problem( + page: Page, + img: Image.Image, + *, + expect_auth_route: bool = False, +) -> str | None: + """Return a reason string when a capture must not be trusted, else None. + + `expect_auth_route` enables the homepage-fallback check, which is only + meaningful for auth-walled routes — the homepage tests legitimately capture + that exact content. + """ + text = _body_text(page) + lowered = text.lower() + + for marker in _NOT_FOUND_MARKERS: + if marker in lowered: + return ( + f"page rendered a 404 ('{marker}') — the route is wrong or the " + "resource no longer exists, so this capture is not the page " + "under test" + ) + + curtain = _CURTAIN_RE.search(text) + if curtain: + return ( + f"page still showing a loading curtain ({curtain.group(0).strip()!r}) " + "— see app_bugs.md #13/#14; capturing now would bake the stuck " + f"state into the baseline. Body began: {text[:120]!r}" + ) + + if expect_auth_route: + for marker in _LOGGED_OUT_MARKERS: + if marker in lowered: + return ( + f"auth route is rendering logged-out chrome (matched {marker!r}) " + "— the session did not apply, or app_bugs.md #15's deep-link " + "fallback served the public homepage" + ) + for marker in _HOMEPAGE_ONLY_MARKERS: + if marker in lowered: + return ( + "auth route rendered the public marketing homepage " + f"(matched {marker!r}) — app_bugs.md #15 deep-link fallback" + ) + + if _is_near_uniform(img): + return "capture is a blank/near-uniform image — nothing rendered" + + return None From 5ff63bbf9a9cb7fa6b18dcaf8cee1fa5c2bfd107 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 10:32:33 +0530 Subject: [PATCH 02/35] Fix run metrics: duration always 0, and skipped runs reported as passed Four defects in the generated TEST_REPORT.md: - Duration read from summary["duration"], but pytest-json-report puts it at the top level, so every report claimed "0.00s". Per-test durations were 0.00s for the same reason - they live on the setup/call/teardown phases, not on the test object. - Pass rate counted skips in the denominator, so a run that skipped almost everything rendered as "0.0%" - a skip is the absence of a result, not a failed one. Now computed over executed tests, "n/a" when none ran. - A run where nothing executed reported "PASSED". An environment outage that skipped every test looked identical to a clean pass; it now reads "NO TESTS EXECUTED". - xfailed/xpassed were never counted, so on any xfail-using suite the rows silently failed to add up to the total. --- utils/report_generator.py | 62 ++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 7 deletions(-) diff --git a/utils/report_generator.py b/utils/report_generator.py index 41f0b79..16cfb00 100644 --- a/utils/report_generator.py +++ b/utils/report_generator.py @@ -31,6 +31,22 @@ def _duration_str(seconds: float) -> str: return f"{m}m {s}s" +def _test_duration(test: dict) -> float: + """Total wall time for a test, summed across its phases. + + pytest-json-report records duration per phase (`setup`/`call`/`teardown`) + and puts no `duration` key on the test object itself, so reading + `test["duration"]` always fell back to 0.0 — every per-test duration in the + report rendered as "0.00s". Setup is worth including, not just `call`: the + authenticated fixtures do real work (SSO login, storage-state refresh), and + attributing none of it to the test hides the slowest part of the suite. + """ + return sum( + test.get(phase, {}).get("duration", 0.0) + for phase in ("setup", "call", "teardown") + ) + + def generate_markdown_report(json_path: Path) -> Path: """ Read *json_path* (pytest-json-report output) and write a Markdown report. @@ -50,10 +66,34 @@ def generate_markdown_report(json_path: Path) -> Path: failed = summary.get("failed", 0) skipped = summary.get("skipped", 0) errors = summary.get("error", 0) - duration = summary.get("duration", 0.0) - - pass_rate = (passed / total * 100) if total else 0.0 - overall = "✅ PASSED" if failed == 0 and errors == 0 else "❌ FAILED" + # xfail/xpass are their own outcomes in pytest-json-report. Without them the + # rows below silently fail to add up to `total` on any suite using xfail — + # which is most of them (see docs/app_bugs.md). + xfailed = summary.get("xfailed", 0) + xpassed = summary.get("xpassed", 0) + # `duration` is a TOP-LEVEL key in pytest-json-report, not a summary key. + # Reading it from `summary` silently returned the 0.0 default on every run, + # so every report claimed "0.00s" regardless of actual runtime. + duration = data.get("duration", summary.get("duration", 0.0)) + + # Pass rate over *executed* tests only. Counting skips in the denominator + # made a run that skipped almost everything look like a catastrophic failure + # (0 passed / 1 skipped rendered as "0.0%"), which is exactly backwards — + # a skip is the absence of a result, not a failed one. + executed = passed + failed + errors + xpassed + pass_rate = f"{(passed + xpassed) / executed * 100:.1f}%" if executed else "n/a" + + # A run where nothing executed is NOT a pass. The old check was + # `failed == 0 and errors == 0`, so an all-skipped run — including one where + # every test was skipped because the environment was down — reported + # "✅ PASSED". That is the same vacuous-green failure mode this framework + # keeps hitting elsewhere; report it honestly instead. + if failed or errors: + overall = "❌ FAILED" + elif executed == 0: + overall = "⚠️ NO TESTS EXECUTED" + (f" ({skipped} skipped)" if skipped else "") + else: + overall = "✅ PASSED" lines: list[str] = [] @@ -78,8 +118,10 @@ def generate_markdown_report(json_path: Path) -> Path: f"| Passed | {passed} ✅ |", f"| Failed | {failed} ❌ |", f"| Skipped | {skipped} ⏭️ |", + f"| xfailed | {xfailed} 🔶 |", + f"| xpassed | {xpassed} 🔷 |", f"| Errors | {errors} 💥 |", - f"| Pass rate | {pass_rate:.1f}% |", + f"| Pass rate | {pass_rate} |", f"| Duration | {_duration_str(duration)} |", "", ] @@ -99,9 +141,15 @@ def generate_markdown_report(json_path: Path) -> Path: g_passed = sum(1 for t in group_tests if t.get("outcome") == "passed") g_failed = sum(1 for t in group_tests if t.get("outcome") == "failed") g_skipped = sum(1 for t in group_tests if t.get("outcome") == "skipped") + # Included so the per-suite counts reconcile with the suite total; without + # it an xfail-heavy suite shows e.g. "21 tests — 13/0/3" and looks like 5 + # results went missing. + g_xfailed = sum(1 for t in group_tests if t.get("outcome") == "xfailed") + g_xpassed = sum(1 for t in group_tests if t.get("outcome") == "xpassed") lines += [ f"### {group.upper()} ({len(group_tests)} tests — " - f"✅ {g_passed} / ❌ {g_failed} / ⏭️ {g_skipped})", + f"✅ {g_passed} / ❌ {g_failed} / ⏭️ {g_skipped}" + f" / 🔶 {g_xfailed} / 🔷 {g_xpassed})", "", "| Test | Result | Duration |", "| ---- | ------ | -------- |", @@ -110,7 +158,7 @@ def generate_markdown_report(json_path: Path) -> Path: outcome = t.get("outcome", "unknown") icon = _status_icon(outcome) name = t.get("nodeid", "").split("::")[-1] - dur = _duration_str(t.get("duration", 0.0)) + dur = _duration_str(_test_duration(t)) lines.append(f"| `{name}` | {icon} {outcome} | {dur} |") lines.append("") From 763c7b70be5b6e8dc7b30f95319555dd9fd336ea Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 10:32:33 +0530 Subject: [PATCH 03/35] Report real test counts in the CI job summary The summary table printed needs..result per suite, which cannot express a test failure here: every suite runs pytest with continue-on-error: true, so the job succeeds no matter how many tests failed. The most-read signal in the run always said "success". Each suite now writes a named JSON report and uploads it; the summary aggregates them into per-suite pass/fail/skip/xfail counts, flags a suite where nothing executed, and keeps the job-status table in a collapsed block labelled as plumbing. Also bumps the visual baseline cache to a v2- generation. Editing tests/visual/** changes the primary key, but restore-keys would still fall back to the newest old cache - which holds the poisoned baselines, so CI would have diffed correct captures against 404s and homepage clones. --- .github/workflows/ci.yml | 76 +++++++++++++++++++----- scripts/suite_summary.py | 121 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 15 deletions(-) create mode 100644 scripts/suite_summary.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9575aba..bd30f27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,6 +98,7 @@ jobs: --tb=short \ --html=reports/api_report.html \ --self-contained-html \ + --json-report-file=reports/api.json \ -m api - name: Upload API test report @@ -105,7 +106,9 @@ jobs: uses: actions/upload-artifact@v7 with: name: api-test-report - path: reports/api_report.html + path: | + reports/api_report.html + reports/api.json retention-days: 30 # ────────────────────────────────────────────── E2E tests (3-shard matrix) @@ -200,6 +203,7 @@ jobs: --tb=short \ --html=reports/a11y_report.html \ --self-contained-html \ + --json-report-file=reports/accessibility.json \ -m accessibility - name: Upload accessibility reports @@ -209,6 +213,7 @@ jobs: name: accessibility-report path: | reports/a11y_report.html + reports/accessibility.json reports/accessibility_report.json reports/accessibility_login_report.json retention-days: 30 @@ -235,14 +240,22 @@ jobs: - name: Install Playwright browsers run: playwright install --with-deps chromium - # Restore cached baselines so we compare against a known good state + # Restore cached baselines so we compare against a known good state. + # + # The `v2-` generation prefix deliberately orphans every cache written + # before 2026-08-12. Those caches hold the poisoned baselines documented in + # docs/visual_diffs.md (a 404, stuck "Loading …" curtains, and four copies + # of the logged-out homepage). Without the bump, editing tests/visual/** + # changes the primary key but `restore-keys` still falls back to the newest + # old cache — so CI would restore the poisoned generation and diff correct + # captures against it. Bump this again if baselines ever need a clean slate. - name: Restore visual baselines cache uses: actions/cache@v5 with: path: snapshots/ - key: visual-baselines-${{ runner.os }}-${{ hashFiles('tests/visual/**') }} + key: visual-baselines-v2-${{ runner.os }}-${{ hashFiles('tests/visual/**') }} restore-keys: | - visual-baselines-${{ runner.os }}- + visual-baselines-v2-${{ runner.os }}- - name: Run visual regression tests continue-on-error: true @@ -252,6 +265,7 @@ jobs: --tb=short \ --html=reports/visual_report.html \ --self-contained-html \ + --json-report-file=reports/visual.json \ -m visual # Save updated baselines back to cache @@ -260,7 +274,8 @@ jobs: uses: actions/cache@v5 with: path: snapshots/ - key: visual-baselines-${{ runner.os }}-${{ hashFiles('tests/visual/**') }} + # Must stay in lockstep with the restore step's generation prefix above. + key: visual-baselines-v2-${{ runner.os }}-${{ hashFiles('tests/visual/**') }} - name: Upload visual regression report and diffs if: always() @@ -269,6 +284,7 @@ jobs: name: visual-regression-report path: | reports/visual_report.html + reports/visual.json screenshots/DIFF_* snapshots/ retention-days: 30 @@ -292,6 +308,16 @@ jobs: - name: Install report dependencies run: pip install pytest-json-report + # All suite reports, so the summary table can show real test counts + # instead of job status. Each suite's pytest step is `continue-on-error`, + # so its job result is ~always "success" no matter how many tests failed. + - name: Download all suite reports + uses: actions/download-artifact@v4 + with: + path: suite-artifacts/ + merge-multiple: true + continue-on-error: true + - name: Download E2E shard reports if: needs.e2e-tests.result != 'skipped' uses: actions/download-artifact@v4 @@ -314,17 +340,37 @@ jobs: if: needs.e2e-tests.result != 'skipped' run: cat reports/e2e_summary.md >> $GITHUB_STEP_SUMMARY - - name: Print suite status table + # Real per-suite test counts, read from the JSON reports rather than from + # `needs..result`. The old table printed job status, which is + # structurally incapable of showing a test failure here: every suite runs + # its pytest step with `continue-on-error: true`, so the job succeeds even + # when tests fail, and the most-read signal in the run always said + # "success". See scripts/suite_summary.py. + - name: Print suite results table run: | - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Suite | Status |" >> $GITHUB_STEP_SUMMARY - echo "|-------|--------|" >> $GITHUB_STEP_SUMMARY - echo "| API Tests | ${{ needs.api-tests.result }} |" >> $GITHUB_STEP_SUMMARY - echo "| E2E Tests | ${{ needs.e2e-tests.result }} |" >> $GITHUB_STEP_SUMMARY - echo "| Accessibility | ${{ needs.accessibility-tests.result }} |" >> $GITHUB_STEP_SUMMARY - echo "| Visual Regression | ${{ needs.visual-tests.result }} |" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "**Platform:** ${{ env.BASE_URL }}" >> $GITHUB_STEP_SUMMARY + { + echo "" + echo "## Test results by suite" + echo "" + python scripts/suite_summary.py \ + "API=suite-artifacts/api.json" \ + "E2E=shard-artifacts/*/reports/e2e_shard_*.json" \ + "Accessibility=suite-artifacts/accessibility.json" \ + "Visual Regression=suite-artifacts/visual.json" + echo "" + echo "**Platform:** ${{ env.BASE_URL }}" + echo "" + echo "
Job status (plumbing, not test results)" + echo "" + echo "| Job | Result |" + echo "|-----|--------|" + echo "| API Tests | ${{ needs.api-tests.result }} |" + echo "| E2E Tests | ${{ needs.e2e-tests.result }} |" + echo "| Accessibility | ${{ needs.accessibility-tests.result }} |" + echo "| Visual Regression | ${{ needs.visual-tests.result }} |" + echo "" + echo "
" + } >> "$GITHUB_STEP_SUMMARY" - name: Upload combined E2E report if: needs.e2e-tests.result != 'skipped' diff --git a/scripts/suite_summary.py b/scripts/suite_summary.py new file mode 100644 index 0000000..4b5d13a --- /dev/null +++ b/scripts/suite_summary.py @@ -0,0 +1,121 @@ +#!/usr/bin/env python3 +"""Render a per-suite results table from pytest-json-report files. + +Why this exists +--------------- +The CI job summary used to print `needs..result` for each suite. Every +suite's pytest step runs with `continue-on-error: true` (so one red suite does +not abort the rest of the pipeline), which means the *job* almost always +succeeds regardless of how many tests failed. The table therefore reported +"success" for a suite that had just failed 28 tests — the single most-read +signal in the run was structurally incapable of showing a failure. + +This reads the actual JSON reports instead and prints real counts, so the +summary reflects tests rather than job plumbing. + +Usage +----- + python scripts/suite_summary.py API=reports/api.json Visual=reports/visual.json + +Each argument is `Label=path`. Missing or unparseable files are reported as +such rather than skipped silently — a suite whose report never arrived is a +result worth seeing, not a blank row. Shard globs are supported so the E2E +matrix can be passed as one label: + + python scripts/suite_summary.py "E2E=reports/e2e_shard_*.json" +""" + +from __future__ import annotations + +import glob +import json +import sys +from pathlib import Path + + +def _load(pattern: str) -> tuple[dict[str, int], float, str | None]: + """Aggregate summary counts across every file matching *pattern*. + + Returns (counts, duration_seconds, error). Multiple files are summed so a + sharded suite reports as a single row. + """ + paths = sorted(glob.glob(pattern)) + if not paths: + return {}, 0.0, "no report found" + + counts: dict[str, int] = {} + duration = 0.0 + for p in paths: + try: + data = json.loads(Path(p).read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + return {}, 0.0, f"unreadable report ({type(exc).__name__})" + summary = data.get("summary", {}) + for key in ("passed", "failed", "skipped", "xfailed", "xpassed", "error", "total"): + if key in summary: + counts[key] = counts.get(key, 0) + summary[key] + # `duration` is top-level in pytest-json-report, not inside `summary`. + duration += data.get("duration", 0.0) + return counts, duration, None + + +def _status(counts: dict[str, int], error: str | None) -> str: + if error: + return f"⚠️ {error}" + failed = counts.get("failed", 0) + counts.get("error", 0) + if failed: + return f"❌ {failed} failed" + executed = counts.get("passed", 0) + counts.get("xpassed", 0) + failed + if executed == 0: + # An all-skipped suite is not a pass. This is how an environment outage + # or a missing credential silently reads as green. + return "⚠️ nothing executed" + return "✅ passed" + + +def _fmt_duration(seconds: float) -> str: + if seconds <= 0: + return "—" + if seconds < 60: + return f"{seconds:.0f}s" + m, s = divmod(int(seconds), 60) + return f"{m}m {s}s" + + +def main(argv: list[str]) -> int: + if not argv: + print("usage: suite_summary.py Label=path [Label=path ...]", file=sys.stderr) + return 2 + + rows: list[str] = [] + any_failure = False + for arg in argv: + if "=" not in arg: + print(f"skipping malformed argument: {arg!r}", file=sys.stderr) + continue + label, pattern = arg.split("=", 1) + counts, duration, error = _load(pattern) + status = _status(counts, error) + if status.startswith("❌"): + any_failure = True + + cells = [ + str(counts.get(key, 0)) if counts else "—" + for key in ("passed", "failed", "skipped", "xfailed", "error") + ] + rows.append( + f"| {label} | {status} | " + " | ".join(cells) + f" | {_fmt_duration(duration)} |" + ) + + print("| Suite | Status | ✅ | ❌ | ⏭️ | 🔶 xfail | 💥 | Duration |") + print("|-------|--------|----|----|----|----------|----|----------|") + print("\n".join(rows)) + print() + if any_failure: + print("> One or more suites have failing tests. Job status alone does not") + print("> reflect this — every suite runs with `continue-on-error: true`.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) From cb4e775fbb212eb39f73fdc99d94a90d0a4c9ca7 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 10:32:44 +0530 Subject: [PATCH 04/35] Record the visual baseline audit and two new findings Phase 11 log plus per-baseline outcomes. Files #24 (auditor assignments hangs past 45s on "Loading your assignments...", a route neither #13 nor #14 covered). Flags a caveat worth acting on: #13/#14 did not reproduce this session - both the AI-Maker overview and the evaluations list loaded fully within 45s. That may be the merged backend perf work, or intermittency, but either way the visual suite's evidence for those rows was unreliable because it was capturing at 2.5s. Re-verify before removing their xfails. --- docs/app_bugs.md | 2 ++ docs/visual_diffs.md | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index 9a28d53..3bf757d 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -29,6 +29,7 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 21 | ai-maker / breadcrumb | The deepest breadcrumb crumb on every `/dashboard/ai-maker/{orgId}/*` sub-page (AI Models, Evaluations, Auditors, etc.) shows the organization name or a static **"Dashboard"** fallback — never the actual section name ("Models", "Evaluations", etc.). Root cause confirmed in `app/[locale]/dashboard/ai-maker/[orgId]/layout.tsx:30`: the shared layout's `BreadCrumbs` config hardcodes the last crumb as `label: organization?.name \|\| "Dashboard"`, computed once at the org-layout level — sub-route pages (`ai-models/page.tsx`, etc.) never override or extend this with their own section label. Live repro on `/dashboard/ai-maker/1/ai-models`: last breadcrumb `
  • ` renders literally as `DashboardDashboard` (duplicated because the layout renders both a desktop and a mobile-truncated span for the same crumb). This was already known — flagged in a 2026-06-22 MCP exploration per this test file's own docstring — but never given a ledger row, so the `assert` was a bare failing assertion rather than a proper `xfail`. | open | Log in as `TEST_EMAIL_1` → navigate to `https://dev.parakh.civicdataspace.in/dashboard/ai-maker/1/ai-models`. Run `document.querySelector("[aria-label='breadcrumb'] li:last-child").textContent` → `"Dashboard"` (or the org name), never `"Models"`. Confirmed against `ParakhAI-frontend/app/[locale]/dashboard/ai-maker/[orgId]/layout.tsx:30` 2026-07-31. Two sibling assertions in the same test file (`test_evaluations_breadcrumb_is_not_dashboard`, `test_evaluation_detail_breadcrumb_has_org_name`) pass reliably in isolation — only the AI Models sub-page repros consistently. | `tests/e2e/test_breadcrumb.py::TestBreadcrumbLabels::test_models_breadcrumb_is_not_dashboard` | 2026-06-22 | | 20 | wizard / start-evaluation gating | The **Start Evaluation** button in the "Start an Evaluation" modal (step 2) intermittently never enables despite a fully valid form (evaluator type selected, non-empty objective typed via real keystrokes). First observed live 2026-07-02: identical keystroke input enabled the button in one session and not the next — suspected server-side validation state race, not a client-rendering issue (retyping/re-selecting fields doesn't reliably recover it within a session). This was previously only documented as an inline `xfail` comment in `test_add_evaluation_modal.py` without a numbered ledger row, which violates this file's own "every xfail must reference a row by id" convention — formalized here retroactively since the same defect surfaced independently in `test_evaluations.py` on 2026-07-28. | open | Open "Start an Evaluation" modal → step 2 → select an evaluator type → type an objective via real keystrokes (not `.fill()`) → wait up to 5s. The Start Evaluation button should enable but intermittently stays disabled. No client-side workaround found; re-selecting the evaluator type or retyping the objective does not reliably unstick it. | `tests/e2e/test_add_evaluation_modal.py::TestStartEvaluationValidation::test_start_enables_with_objective_and_disables_when_cleared`; `tests/e2e/test_evaluations.py::TestNewEvaluationModalStep2::test_start_requires_objective_filled` | 2026-07-02 | +| 24 | auditor / assignments hang | `/dashboard/auditor/assignments` hangs indefinitely on "Loading your assignments…" — still curtained past a 45s budget. Found 2026-08-12 by the visual suite's new capture-integrity gate (`utils/visual_guards.py`), which refused to save a baseline of the stuck state. Same family as #13/#14 (a route-level data query that never resolves) but on a route those rows never covered, and notably this one did **not** clear within 45s in the same session where the AI-Maker overview and evaluations list both loaded fully — so it is not merely slow. | open | Log in as `TEST_EMAIL_1` → switch to Evaluator role → navigate to `https://dev.parakh.civicdataspace.in/dashboard/auditor/assignments`. Page renders sidebar/chrome but the content area stays on "Loading your assignments…" past 45s. | `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::test_authenticated_page_desktop[chromium-auditor_assignments]` (skips via the integrity gate rather than baselining the stuck state) | 2026-08-12 | | 23 | auth / backend | ParakhAPI's `DataSpaceAuthMiddleware` (`graphql_api/middleware/dataspace_user.py::process_request`) wraps its entire Bearer-token validation path (DataSpace `/api/auth/user/info/` lookup) in a blanket `except Exception`, and on **any** failure — expired token, invalid token, DataSpace being briefly unreachable — silently sets `request.user = AnonymousUser()` and continues. The failure is only `logger.warning`'d server-side; the client gets a normal `200` response shaped exactly like a legitimate anonymous request (e.g. `audits` returns `{data: [], totalItemsCount: 0}`, no `errors` array, nothing containing "token"/"expired"/401/403). A client has no way to distinguish "you are genuinely unauthenticated" from "your session just died" — it can only infer this indirectly from previously-authenticated-looking context. Root-caused 2026-08-11 while investigating 3 seemingly-flaky `ParakhAI_test` security tests that passed in isolation but failed only when reached ~4-5 minutes into a long suite run; confirmed by reading the middleware source directly. This is very likely the true root cause behind this file's own previously-vague "session expired mid-suite" attributions in the Phase 10 log entries (bug #3/#12 family) — those blamed Keycloak/dev-env instability in general, but the precise mechanism is this middleware's silent-swallow, which a well-behaved API should instead surface as a 401. | open | With a valid Bearer token, wait past the token's TTL (~5 min on dev), then issue any authenticated GraphQL query (e.g. `{ audits { data { id } totalItemsCount } }`) with the now-stale token. Response is `200` with the same shape as an anonymous request, no error. Confirmed against `ParakhAPI` `graphql_api/middleware/dataspace_user.py:99-104` (dev branch) 2026-08-11. | Indirectly, any test using `tests/conftest.py::authenticated_graphql_client` on a long-running suite — see the fixture's own docstring (fixed 2026-08-11 to proactively refresh before this can bite) for the test-side mitigation; this row documents the underlying product-side gap that mitigation works around. | 2026-08-11 | | 22 | audits query / backend perf | Sorting the `audits` GraphQL query by `passed_tests` is measurably slower than the default `created_at` ordering under concurrent load. 5 concurrent authenticated `audits(sortOptions: [{field: "passed_tests", direction: "desc"}])` requests (light field selection: `id status passedTests` only, `limit: 20`) took 9-15s each on the cleanest run and up to 35s under noisier conditions, vs. a 6s budget for the equivalent unsorted 5-concurrent read (`TestConcurrentGraphQLReads`). 100% success, zero errors — not a correctness bug, but the delta suggests the `(organization, -created_at)` index added for this query (commit `4f51ab2`) doesn't help an `ORDER BY passed_tests`, which has no index of its own. Worth a backend follow-up (composite/partial index, or precomputing `passed_tests` ordering) if sort-by-pass-rate becomes a real UI feature rather than just an API capability. | open | Run `pytest tests/load/test_load_graphql.py -m load -v -k TestAuditsSortPerformance` — compare reported latencies in `reports/load_metrics_graphql.json` (`concurrent_audits_sorted_n5`) against the unsorted baseline in the same file. | `tests/load/test_load_graphql.py::TestAuditsSortPerformance::test_5_concurrent_audits_sorted_by_passed_tests` | 2026-08-11 | @@ -66,3 +67,4 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th - Debug scripts used for live repro (`scripts/_dbg_dashboard.py`, `scripts/_dbg_role_and_evals.py`) were throwaway and removed after use, not committed. - Full target list re-run after the xfail changes: 3 passed / 1 skipped / 6 xfailed / 0 failed. - **Phase 9 — CI failure triage: bulk/modal/wizard cluster** (2026-07-31): re-ran all 17 shard-1 failures from this cluster (`test_add_evaluation_bulk.py`, `test_add_evaluation_modal.py`, `test_breadcrumb.py`, `test_bulk_evaluation_flow.py`, `test_add_evaluation_playground.py`) in isolation with `-n 1`, `--reruns 0`, no other heavy suites running concurrently. **16 of 17 were pure CI-concurrency noise** (bug #3 family) — all passed/skipped-as-designed cleanly once the shared dev backend wasn't being hammered by 3 E2E shards + API tests simultaneously; no code changes needed for those. **1 genuine, previously-unlogged bug found**: `test_breadcrumb.py::test_models_breadcrumb_is_not_dashboard` — filed as bug #21 (breadcrumb on every AI-Maker sub-page shows the org name or a static "Dashboard" fallback instead of the actual section name; root-caused to the shared `[orgId]/layout.tsx` breadcrumb config never being overridden per-route). The test file's own docstring already flagged this from a 2026-06-22 MCP exploration but it had never been given a ledger row or `xfail` marker — fixed that gap. Final state: all 17 tests pass/xfail correctly in isolation. +- **Phase 11 — visual baseline integrity audit** (2026-08-12): audited every committed visual baseline rather than adding new visual tests, after noticing the suite was green while covering pages it had never actually captured. **8 of 12 authenticated baselines were invalid** — they encoded a broken or wrong page: `evaluation_detail_completed` was a **404** (the route `/evaluation/288` does not exist; the real ones are `/dashboard/ai-maker/[orgId]/evaluations/[id]` and `/dashboard/auditor/evaluations/[id]`), `ai_maker_dashboard` was a stuck "Loading overview…", `evaluations_list` was a false-empty state, `org_selector` was a bare "Loading" spinner, and four (`auditor_dashboard`, `auditors_management`, `models_list`, `new_evaluation_wizard`) were **pixel-identical to the logged-out public homepage** — bug #15's deep-link fallback frozen into baselines (found by checksum: five files shared one md5 at 348560 bytes). Because most sat under hard pixel-diff assertions, they passed *because* the app was broken; `ai_maker_dashboard` would have gone red the day bug #14 was fixed. Root cause was not the gate's absence but the wait: `_capture_page_masked` settled a flat **2.5s** after `load`, while this repo's own `lessons.md` documents curtains at 15-30s+ — so captures landed mid-curtain by design. Fixes: `wait_for_render_settled()` (45s budget) plus `utils/visual_guards.py`, which refuses to save *or* compare a capture that is a 404, a loading curtain, logged-out chrome on an auth route, or a blank image (it skips with a reason — an invalid capture means "did the pixels change?" was never asked, so the honest result is no-signal, not a pass). 7 baselines also carried the test account's real name/initials; identity masks were verified active *before* regeneration, since the 4 homepage-fallback files were PII-free only by virtue of being logged out and would otherwise have gained PII on re-capture. Outcome: 8 regenerated clean and verified against real content, `auditor_dashboard` correctly refused (bug #15 firing live — the gate caught 6 auth routes serving logged-out chrome in a single run, confirming #15 is active *now*, not historical), and `evaluation_detail_completed` still has no baseline because `completed_eval_id` found no audit meeting its bar and its UI-seeding fallback timed out (pre-existing fixture breakage, not visual). **Caveat worth acting on: bugs #13/#14 did not reproduce in this session** — both the AI-Maker overview and the evaluations list loaded fully within 45s. That may mean the merged backend perf work (PR #95/#96) genuinely fixed them, or that they are intermittent; either way the *visual* suite's evidence for those rows was unreliable, since it was capturing at 2.5s. Re-verify independently before removing their xfails. Filed #24 (auditor assignments genuinely hangs past 45s — a route neither #13 nor #14 covered). Also corrected a repeated claim: `snapshots/` is **gitignored** (`.gitignore:36`), so baselines were never in git — the PII exposure is via the public repo's CI artifacts (30-day retention) and Actions cache, not git history. diff --git a/docs/visual_diffs.md b/docs/visual_diffs.md index 349b542..e1bfc2b 100644 --- a/docs/visual_diffs.md +++ b/docs/visual_diffs.md @@ -103,3 +103,28 @@ The 8 xfails are all under the pre-existing `VISUAL-002` classification (`_NON_D **Follow-up worth doing (not done this run, flagged for review):** the `VISUAL-002` xfail reason string in `tests/visual/test_visual_regression.py` (~line 366) currently blames "model dropdown / live stats / variable list ordering" for all 8 routes — that's accurate for the 3 low-diff routes but understates what's happening on the 5 high-diff ones (see bug #15). Consider splitting the xfail message or diff-magnitude threshold so a ~43% diff (wrong page) is distinguishable in CI output from a ~1% diff (live data noise), since they currently read identically in the test summary. Baselines remain **21 / 21**, unchanged this run. + +### Run 2026-08-12 — Phase 11: baseline integrity audit (env: dev) + +Not a diff-triage run. Audited what the committed baselines actually *contain*, after the suite reported "20 passed / 1 failed" while covering pages it had never captured. + +**8 of the 12 authenticated baselines were invalid.** Full analysis in `docs/app_bugs.md` → Phase 11; inventory impact recorded here. + +| baseline | what it actually contained | outcome | +|---|---|---| +| `auth_evaluation_detail_completed` | Next.js **404** — route `/evaluation/288` doesn't exist | route fixed; **no baseline yet** (see below) | +| `auth_ai_maker_dashboard` | stuck "Loading overview…" (bug #14) | regenerated clean — real stat cards, 6 model cards, populated Recent Evaluations | +| `auth_evaluations_list` | false-empty state | regenerated clean — 10 rows, "Page 1 of 31" | +| `auth_org_selector` | bare "Loading" spinner (bug #19 family) | regenerated clean | +| `auth_auditor_dashboard` | logged-out public homepage (bug #15) | **correctly refused** by the gate — #15 firing live | +| `auth_auditors_management` | logged-out public homepage (bug #15) | regenerated clean | +| `auth_models_list` | logged-out public homepage (bug #15) | regenerated clean | +| `auth_new_evaluation_wizard` | logged-out public homepage (bug #15) | regenerated clean | + +The four homepage clones were found by checksum, not by eye — five files shared one md5 at exactly 348560 bytes, four of them auth routes. **Worth reusing as a technique:** byte-identical baselines across supposedly-different pages is a fast, zero-cost tell for this whole class of defect. + +Also regenerated `auth_auditor_assignments` and `auth_auditor_evaluations` — these were valid, but carried the test account's name/initials and would have xfailed forever once identity masking landed. `auth_dashboard_role_selector` was regenerated separately: its 0.202% diff was a **legitimate frontend copy change** (commit `90f38dc` reworded both role cards — "For people building AI" → "Connect AI models to run evaluations", "For expert as evaluator" → "Evaluate AI Models you've been invited to review"). That one test was the only one in the suite doing its job, precisely because it was one of the few baselines showing a real page. + +**Inventory: 21 → 20.** Missing is `auth_evaluation_detail_completed`; its route is now correct, but `completed_eval_id` found no audit meeting its `progress==100 AND totalTests>0 AND completedAt` bar and the UI-seeding fallback timed out — pre-existing fixture breakage, tracked separately, not a visual problem. + +All baselines now have distinct md5s. Identity masking (header avatar + sidebar identity block) is active and was verified on a real capture *before* any regeneration — necessary ordering, since the four homepage clones were PII-free only by virtue of being logged out and would have *gained* PII on re-capture. From 1ad98fc0de08ad4c70def39a3adfec5024ee1257 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 10:48:19 +0530 Subject: [PATCH 05/35] Share visual masks, and record bug #15 severity update Moves DEFAULT_MASKS / AUTH_PII_MASKS into utils/visual_guards so sibling visual suites use one definition - a new module that forgot the identity masks would quietly start baking the test account's name back into baselines. Also updates app_bugs.md #15: two consecutive runs hit the logged-out fallback on 12 of 24 authenticated captures (~50%, vs the 17-25% recorded in July), and the fallback now renders logged-out chrome (LOGIN / SIGN UP) rather than authenticated shell with wrong content. Both samples are small and conditions differed, so it is noted as materially worse under current conditions rather than a measured regression. --- docs/app_bugs.md | 2 +- tests/visual/test_visual_regression.py | 55 ++++++-------------------- utils/visual_guards.py | 49 +++++++++++++++++++++++ 3 files changed, 61 insertions(+), 45 deletions(-) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index 3bf757d..8f84b0b 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -21,7 +21,7 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 11 | wizard / manual-workspace | The manual-evaluation workspace Test Cases tab fails to render its expected UI elements when reached via the Domain type flow: (a) module cards appear (counter confirmed) but clicking the first card does not open an input/output panel — neither `MANUAL_INPUT_TEXTAREA` nor `MANUAL_CHANGE_MODULE_LINK` becomes visible; (b) the minimum test-cases hint ("Evaluate at least 3 test cases per module…") is not rendered on the Test Cases tab. These were previously skipped with `pytest.skip` guards; they now surface as failures. | open | Log in → AI Maker → CivicDataLab → New Evaluation → Start → select Domain type → fill Objective → click Test Cases tab. Click any module card and observe whether an input panel appears. Also check whether the min-cases hint text is rendered below the module list. | `tests/e2e/test_evaluation_workspace_manual.py::TestManualWorkspaceTestEntry::test_module_click_opens_input_panel`; `tests/e2e/test_evaluation_workspace_manual.py::TestManualWorkspaceTestEntry::test_min_test_cases_note_visible` | 2026-05-23 | | 13 | evaluations list / backend perf | The frontend `GetAudits` query that powers the AI-Maker Evaluations list (`/dashboard/ai-maker/{org}/evaluations`) **hangs and never returns** on dev — the page sits on "Loading evaluations…" indefinitely (still up at 90 s). Root-caused 2026-06-12 via live Playwright + direct GraphQL probing on org 1 (981 total audits): requesting the **light** field set (`id name status modelId`, `limit:100`) returns in ~10.6 s, and `metrics`-only or `modules`-only at `limit:100` each return in ~16–18 s, but the **full field set the frontend actually requests** (`name modelName status modules metrics evaluationMode auditType totalTests passedTests failedTests createdAt startedAt completedAt`) **times out past 30 s even at `limit:5`** — so it is the combined per-row resolver cost (likely N+1 on `metrics`/`modules`/test-count fields), not row volume. Backend should batch/precompute these fields or paginate server-side. Also observed intermittent `ERR_NETWORK_CHANGED` / "Backend server is not available" GraphQL errors on dev during the same session (degraded backend). **Not a test defect** — no client-side timeout bump can help since the query never completes. | open | Log in → AI Maker → CivicDataLab → Evaluations. Page stays on "Loading evaluations…". In DevTools, the `GetAudits` POST to `dev.api.parakh.civicdataspace.in/graphql/` has no response after 90 s. Confirmed via Playwright MCP 2026-06-12: full-field `audits` query aborts at 30 s even with `limit:5`, while light-field returns in ~10 s. | `tests/e2e/test_evaluations.py::TestStatusFilterTabs::*` (all 6 — tabs/column/rows render only after the list query resolves); transitively any test that asserts on rendered evaluations-list content for org 1; `tests/e2e/test_evaluations.py::TestEvaluationsListPage::test_table_column_headers_are_present` (added 2026-07-31 — re-run isolated, no concurrent CI load: page still stuck on "Loading evaluations..." past 40s, confirming the hang reproduces without concurrent-shard traffic) | 2026-06-12 | | 14 | a11y / dashboard heading | The AI Maker Dashboard (`/dashboard/ai-maker/1`) never renders any `

    `/`

    `/`

    ` heading — `document.querySelectorAll('h1,h2,h3').length` is `0` even after a 30s wait. Root cause: the "Overview" widget (which appears to be where the page heading lives) is permanently stuck on "Loading overview..." and never resolves — the same failure shape as bug #13 (backend `GetAudits`/overview resolver hang), just surfacing on the dashboard-home overview query instead of the evaluations-list query. Because the widget never mounts, the page has zero heading elements, violating WCAG 1.3.1 (Info and Relationships) / a broken document outline for screen-reader users. | open | Log in as `TEST_EMAIL_1` → navigate directly to `https://dev.parakh.civicdataspace.in/dashboard/ai-maker/1`. Page loads nav/sidebar/breadcrumbs but the main content area shows only "Loading overview..." indefinitely (confirmed stuck past 30s). Run `document.querySelectorAll('h1,h2,h3').length` → `0`. Confirmed via Playwright 2026-07-10. | `tests/accessibility/test_accessibility_auth.py::TestDashboardAccessibility::test_dashboard_has_exactly_one_h1`; `tests/e2e/test_ai_maker_dashboard.py::TestAIMakerDashboardLoads::test_civicdatalab_name_visible_in_sidebar`, `TestModelCardsonHome::test_add_new_model_button_is_visible`, `TestModelCardsonHome::test_model_cards_are_rendered`, `TestModelCardsonHome::test_text_generation_badge_is_present` (added 2026-07-31 — re-run isolated (no concurrent CI load), direct repro script confirmed the whole home content area, not just the heading, is gated behind the same stuck "Loading overview..." widget: org identity block (`CivicdataLab` text), model cards, `Add A New Model` button, and model-type badges never render because they all read from the same overview payload) | 2026-07-10 | -| 15 | routing / auth deep-link | Direct navigation to authenticated AI-Maker / Auditor sub-routes (`/dashboard/ai-maker/1/ai-models`, `/dashboard/ai-maker/1/evaluations/new`, `/dashboard/ai-maker/1/auditors`, `/dashboard/ai-maker/1/prompt-libraries`, `/dashboard/auditor`) **intermittently renders the public marketing homepage** (hero "Build AI that's trustworthy from day one.", feature tabs, CTA cards, footer) instead of the requested dashboard page, even though the user is authenticated (avatar/session valid) and the URL bar shows the correct path. Root-caused 2026-07-10 via a scripted repro: a fresh browser context seeded with a valid `storage_state` (cookies + localStorage from a real Keycloak login) navigating straight to `/dashboard/ai-maker/1/ai-models` with `wait_until="load"` + 2.5s settle reproduced the homepage fallback in **1 of 6 trials (~17%)**; the same repro against `/dashboard/ai-maker/1/prompt-libraries` reproduced it in **1 of 4 trials (~25%)**, and a further capture landed in a visibly half-transitioned state (both "Prompt Libraries" page chrome and homepage CTA/footer content stacked in the same viewport — see `screenshots/DIFF_auth_prompt_libraries_desktop_1440x900.png`). When it does NOT reproduce, the correct page renders fully and correctly (light theme, correct heading, working sidebar, "Loading AI models…" spinner resolving normally). This is a materially different and more severe symptom than the "non-deterministic async content" (live stats, model dropdown ordering) that `_NON_DETERMINISTIC_VISUAL` in `tests/visual/test_visual_regression.py` currently attributes these routes' diffs to — the wizard/models/auditors/auditor-dashboard visual tests see ~43% pixel diffs because they are sporadically capturing the wrong page entirely, not just varying live data. Likely a client-side routing/auth-guard race on hard navigation (the route guard falls through to the default/public route before the session check resolves), since it only reproduces on deep-link/full-page loads, not in-app client-side navigation. | open | Using a Playwright context seeded with a valid Keycloak `storage_state`, repeatedly `page.goto()` directly to `/dashboard/ai-maker/1/ai-models` (or any route in the list above) with `wait_until="load"` and a 2.5s+ settle delay; repeat ~5-6 times. Roughly 1 in 5-6 loads renders the homepage hero/CTA/footer instead of the dashboard page content. Confirmed via ad hoc Playwright script 2026-07-10 (not yet encoded as a standing test — see `docs/visual_diffs.md` Run 2026-07-10 for the diff images that first surfaced it). | `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::test_authenticated_page_desktop[chromium-models_list]`, `[chromium-new_evaluation_wizard]`, `[chromium-auditors_management]`, `[chromium-auditor_dashboard]`, `[chromium-prompt_libraries]` (all xfail under VISUAL-002 — the xfail reason text should be revisited given this finding); `tests/e2e/test_user_flows.py::TestFlow04_EvaluationDetailAndReport::test_evaluation_detail_shows_full_results` (added 2026-07-13 — deep-link to `/evaluations/{id}` rendered the public homepage; avatar/session chrome still present, confirming auth was intact) | 2026-07-10 | +| 15 | routing / auth deep-link | Direct navigation to authenticated AI-Maker / Auditor sub-routes (`/dashboard/ai-maker/1/ai-models`, `/dashboard/ai-maker/1/evaluations/new`, `/dashboard/ai-maker/1/auditors`, `/dashboard/ai-maker/1/prompt-libraries`, `/dashboard/auditor`) **intermittently renders the public marketing homepage** (hero "Build AI that's trustworthy from day one.", feature tabs, CTA cards, footer) instead of the requested dashboard page, even though the user is authenticated (avatar/session valid) and the URL bar shows the correct path. Root-caused 2026-07-10 via a scripted repro: a fresh browser context seeded with a valid `storage_state` (cookies + localStorage from a real Keycloak login) navigating straight to `/dashboard/ai-maker/1/ai-models` with `wait_until="load"` + 2.5s settle reproduced the homepage fallback in **1 of 6 trials (~17%)**; the same repro against `/dashboard/ai-maker/1/prompt-libraries` reproduced it in **1 of 4 trials (~25%)**, and a further capture landed in a visibly half-transitioned state (both "Prompt Libraries" page chrome and homepage CTA/footer content stacked in the same viewport — see `screenshots/DIFF_auth_prompt_libraries_desktop_1440x900.png`). When it does NOT reproduce, the correct page renders fully and correctly (light theme, correct heading, working sidebar, "Loading AI models…" spinner resolving normally). This is a materially different and more severe symptom than the "non-deterministic async content" (live stats, model dropdown ordering) that `_NON_DETERMINISTIC_VISUAL` in `tests/visual/test_visual_regression.py` currently attributes these routes' diffs to — the wizard/models/auditors/auditor-dashboard visual tests see ~43% pixel diffs because they are sporadically capturing the wrong page entirely, not just varying live data. Likely a client-side routing/auth-guard race on hard navigation (the route guard falls through to the default/public route before the session check resolves), since it only reproduces on deep-link/full-page loads, not in-app client-side navigation. | open | Using a Playwright context seeded with a valid Keycloak `storage_state`, repeatedly `page.goto()` directly to `/dashboard/ai-maker/1/ai-models` (or any route in the list above) with `wait_until="load"` and a 2.5s+ settle delay; repeat ~5-6 times. Roughly 1 in 5-6 loads renders the homepage hero/CTA/footer instead of the dashboard page content. Confirmed via ad hoc Playwright script 2026-07-10 (not yet encoded as a standing test — see `docs/visual_diffs.md` Run 2026-07-10 for the diff images that first surfaced it). | `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::test_authenticated_page_desktop[chromium-models_list]`, `[chromium-new_evaluation_wizard]`, `[chromium-auditors_management]`, `[chromium-auditor_dashboard]`, `[chromium-prompt_libraries]` (all xfail under VISUAL-002 — the xfail reason text should be revisited given this finding); `tests/e2e/test_user_flows.py::TestFlow04_EvaluationDetailAndReport::test_evaluation_detail_shows_full_results` (added 2026-07-13 — deep-link to `/evaluations/{id}` rendered the public homepage; avatar/session chrome still present, confirming auth was intact); `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::*` (2026-08-12 — the new capture-integrity gate in `utils/visual_guards.py` now detects this automatically and skips instead of baselining it) **— SEVERITY UPDATE 2026-08-12: observed at ~50%, far above the 17-25% recorded above.** Two consecutive full visual runs hit the logged-out fallback on **12 of 24 authenticated captures** (5/12 then 7/12). Affected: `/dashboard/ai-maker/1`, `/ai-models`, `/evaluations`, `/evaluations/new`, `/auditors`, `/prompt-libraries`, `/dashboard/auditor`. NOT affected in either run: `/dashboard` (role selector), `/dashboard/ai-maker` (org selector), `/dashboard/auditor/evaluations` — so it is not uniform across depth. New detail vs the 2026-07-10 characterisation: the fallback now renders **logged-out chrome (LOGIN / SIGN UP)**, i.e. the session is not merely failing to apply to the route — the shell itself renders unauthenticated. Caveat on the comparison: both figures are small samples (the original was 1/6 and 1/4 trials) and load conditions differed, so treat this as "materially worse under current conditions" rather than a precise regression measurement. Independent confirmation that it also fired historically: four baselines were byte-identical copies of the logged-out homepage. | 2026-07-10 | | 16 | SEO / homepage content | Homepage `` contains a spelling error: `"Paricipatory AI Evaluation"` (missing the first `t` in "Participatory"). Root-caused in frontend source: `app/layout.tsx` line 180, `export const metadata = { ..., description: "Paricipatory AI Evaluation" }`. One-line content fix. | open | Load `https://dev.parakh.civicdataspace.in/`. Run `document.querySelector('meta[name="description"]').content` → `"Paricipatory AI Evaluation"`. Confirmed against `ParakhAI-frontend/app/layout.tsx:180` 2026-07-13. | `tests/e2e/test_platform_issues.py::TestHomepageSEO::test_meta_description_has_no_typo` | 2026-07-13 | | 17 | SEO / homepage content | Homepage has no Open Graph `` tag (or any other `og:*` tags) — the root `metadata` export in `app/layout.tsx` only sets `title` and `description`, no `openGraph` block. Links to the homepage shared on social media / chat apps will render with no title card. | open | Load `https://dev.parakh.civicdataspace.in/`. Run `document.querySelector('meta[property="og:title"]')` → `null`. Confirmed against `ParakhAI-frontend/app/layout.tsx` (metadata export has no `openGraph` key) 2026-07-13. | `tests/e2e/test_platform_issues.py::TestHomepageSEO::test_og_title_is_present` | 2026-07-13 | | 18 | routing / 404 | Visiting any non-existent URL as an anonymous user does not show a 404/not-found message — it silently redirects to the generic NextAuth "sign in with Keycloak" page instead, with no indication the URL was invalid. Root cause confirmed in `middleware.ts`: `publicPages = ['/']` is the *only* unauthenticated route; the matcher (`'/((?!_next|api|.*\\..*).*)'`) sends every other path — including typos and genuinely nonexistent routes — through `withAuth`, which redirects unauthenticated requests to `/api/auth/signin` before Next.js ever gets a chance to render a not-found page. This is a distinct, more severe variant of the two existing sibling xfails in this file (`test_404_page_has_home_navigation_link` / `test_404_page_has_logo_or_branding`), which assumed a bare-but-present Next.js 404 page — there isn't one for anonymous users at all. | open | As an anonymous user, navigate to `https://dev.parakh.civicdataspace.in/this-page-does-not-exist-xyz-123`. Observe the generic Keycloak/NextAuth sign-in screen, not a 404 page. Confirmed against `ParakhAI-frontend/middleware.ts` (`publicPages = ['/']`) 2026-07-13. | `tests/e2e/test_platform_issues.py::TestNotFoundPage::test_404_page_shows_not_found_message` | 2026-07-13 | diff --git a/tests/visual/test_visual_regression.py b/tests/visual/test_visual_regression.py index 9726d4b..bd002f1 100644 --- a/tests/visual/test_visual_regression.py +++ b/tests/visual/test_visual_regression.py @@ -15,7 +15,12 @@ from playwright.sync_api import Page from utils.config import Config -from utils.visual_guards import capture_integrity_problem, wait_for_render_settled +from utils.visual_guards import ( + AUTH_PII_MASKS, + DEFAULT_MASKS, + capture_integrity_problem, + wait_for_render_settled, +) pytestmark = [pytest.mark.visual] @@ -163,49 +168,11 @@ def _capture_page_masked(page: Page, url: str, masks: list) -> Image.Image: return Image.open(io.BytesIO(raw)) -# Dynamic regions that change every page load — masking stops false diffs on -# nightly visual runs. -_DEFAULT_MASKS = [ - "[class*='timestamp']", - "[class*='last-updated']", - "[class*='activity']", - "time", - "[class*='polling']", -] - -# Personal data rendered by the logged-in shell. Masked on every authenticated -# capture for two reasons: -# 1. Privacy — these baselines embed the real test account's full name -# ("Welcome, ") and avatar initials. `snapshots/` is -# gitignored so they never reach git history, but CI both caches the -# directory and uploads it as a 30-day build artifact -# (.github/workflows/ci.yml), so the images are retrievable by anyone with -# repo access. -# 2. Portability — an unmasked name pins every baseline to one account, so -# re-running the suite as TEST_USER_2 would diff on the sidebar alone. -# Deliberately NOT matching initials literally: locators/workspace_locators.py -# pins `text=MSM` while this account renders "SM", confirming initials vary per -# account. Anchored `^Welcome,` is used rather than -# AIMakerLocators.WELCOME_MESSAGE's bare `text=Welcome`, which would also match -# body copy containing the word. `_capture_page_masked` drops zero-count -# selectors, so listing extras is free. -# Verified against the live DOM 2026-08-12: -#
    ← masked (the whole identity block) -#
    SM
    ← initials, NO avatar class -#

    Welcome, Saqib Manan

    -# Switch Roles -#
    -# The sidebar initials circle is a bare Tailwind div, so `[class*='avatar' i]` -# (which matches only the 2 header avatars) does not cover it. Masking the -# parent block catches the circle and the name together. That also masks the -# "Switch Roles" link — an accepted trade: a small, static control loses pixel -# coverage so no capture carries identity. -_AUTH_PII_MASKS = [ - "[class*='avatar' i]", # header avatar circle(s) rendering initials - ".welcome-text >> xpath=..", # sidebar identity block (circle + name) - "text=/^Welcome,/ >> xpath=..", # fallback if .welcome-text is renamed - "button[aria-label='Open profile']", # per locators/dashboard_locators.py -] +# Mask lists live in utils.visual_guards so sibling visual suites share one +# definition — a new module that forgot _AUTH_PII_MASKS would quietly start +# baking the account name back into baselines. +_DEFAULT_MASKS = DEFAULT_MASKS +_AUTH_PII_MASKS = AUTH_PII_MASKS # Auth routes whose content is genuinely non-deterministic between two captures # against the live/shared dev environment: the New Evaluation wizard's model diff --git a/utils/visual_guards.py b/utils/visual_guards.py index 6e09259..33e15b7 100644 --- a/utils/visual_guards.py +++ b/utils/visual_guards.py @@ -194,3 +194,52 @@ def capture_integrity_problem( return "capture is a blank/near-uniform image — nothing rendered" return None + + +# ── Screenshot masks ───────────────────────────────────────────────────────── +# Shared with every visual test module so a new suite cannot silently omit the +# identity masks and start committing the test account's name to baselines. + +# Dynamic regions that change every page load — masking stops false diffs on +# nightly visual runs. +DEFAULT_MASKS = [ + "[class*='timestamp']", + "[class*='last-updated']", + "[class*='activity']", + "time", + "[class*='polling']", +] + +# Personal data rendered by the logged-in shell. Masked on every authenticated +# capture for two reasons: +# 1. Privacy — these baselines embed the real test account's full name +# ("Welcome, ") and avatar initials. `snapshots/` is +# gitignored so they never reach git history, but CI both caches the +# directory and uploads it as a 30-day build artifact +# (.github/workflows/ci.yml), so the images are retrievable by anyone with +# repo access. +# 2. Portability — an unmasked name pins every baseline to one account, so +# re-running the suite as TEST_USER_2 would diff on the sidebar alone. +# Deliberately NOT matching initials literally: locators/workspace_locators.py +# pins `text=MSM` while this account renders "SM", confirming initials vary per +# account. Anchored `^Welcome,` is used rather than +# AIMakerLocators.WELCOME_MESSAGE's bare `text=Welcome`, which would also match +# body copy containing the word. `_capture_page_masked` drops zero-count +# selectors, so listing extras is free. +# Verified against the live DOM 2026-08-12: +#
    ← masked (the whole identity block) +#
    SM
    ← initials, NO avatar class +#

    Welcome, Saqib Manan

    +# Switch Roles +#
    +# The sidebar initials circle is a bare Tailwind div, so `[class*='avatar' i]` +# (which matches only the 2 header avatars) does not cover it. Masking the +# parent block catches the circle and the name together. That also masks the +# "Switch Roles" link — an accepted trade: a small, static control loses pixel +# coverage so no capture carries identity. +AUTH_PII_MASKS = [ + "[class*='avatar' i]", # header avatar circle(s) rendering initials + ".welcome-text >> xpath=..", # sidebar identity block (circle + name) + "text=/^Welcome,/ >> xpath=..", # fallback if .welcome-text is renamed + "button[aria-label='Open profile']", # per locators/dashboard_locators.py +] From 4beae6ffe98272c41561264392545a2285c91c61 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 11:46:43 +0530 Subject: [PATCH 06/35] Add modal and responsive visual coverage Responsive: mobile/tablet baselines for 4 high-traffic auth routes (role selector, AI-Maker overview, evaluations list, models list) plus a no-horizontal-overflow assertion at mobile width - a deterministic, non-pixel-diff check that catches the most common responsive bug directly. Verified: 10 passed, sensible skip/xfail, all 4 overflow checks passed. Surfaced a real bug: the evaluations table overflows horizontally at 768px, cutting off the Model column and the row-count control. Modal: element-scoped captures for the Know More info dialog (90f38dc) and the Start Evaluation modal's two steps, targeting the ModelSelectionModal refactor (130b5ac). Two interaction steps (evaluation-method radio, evaluator-type radio) intermittently time out against the live dev UI - wrapped to skip with a clear reason rather than fail, same pattern as completed_eval_id's UI-seeding fallback. Masks are shared via utils.visual_guards (DEFAULT_MASKS / AUTH_PII_MASKS) rather than redefined, so this coverage can't accidentally omit the identity masking that visual_regression.py relies on. --- tests/visual/test_visual_modals.py | 217 ++++++++++++++++++++++ tests/visual/test_visual_responsive.py | 248 +++++++++++++++++++++++++ 2 files changed, 465 insertions(+) create mode 100644 tests/visual/test_visual_modals.py create mode 100644 tests/visual/test_visual_responsive.py diff --git a/tests/visual/test_visual_modals.py b/tests/visual/test_visual_modals.py new file mode 100644 index 0000000..efa7ec7 --- /dev/null +++ b/tests/visual/test_visual_modals.py @@ -0,0 +1,217 @@ +""" +Visual regression coverage for modals and overlays. + +The visual suite had zero coverage of any dialog, while the frontend actively +changes them — `90f38dc` ("Added modal with know more") and `130b5ac` +("refactor: update evaluation method handling in ModelSelectionModal") both +landed on `origin/dev` recently. Modals concentrate exactly the defects +full-page captures are worst at spotting: backdrop/z-index, panel width and +overflow, control alignment, disabled-button styling. + +Captures are **element-scoped** (`Locator.screenshot()` on the dialog), not +full-page. Two reasons: + 1. Stability — the pages these dialogs open over (the evaluations list, the + evaluator dashboard) render live, ordering-unstable data. A full-page + capture would diff on the backdrop content, not the modal, and would land + in the same permanently-xfailed bucket as the routes in + `_NON_DETERMINISTIC_VISUAL`. + 2. Signal — a diff here points at the dialog, which is the thing under test. +The trade-off is that the dimmed backdrop itself is not covered; that is +deliberate, since it cannot be captured without also capturing the volatile +page beneath it. + +Determinism: step-1 captures explicitly select the first model and version +rather than trusting whatever the dropdown defaults to — model ordering varies +between loads (the same reason `new_evaluation_wizard` is classified +non-deterministic in the sibling module). The prefilled evaluation name embeds +a live timestamp ("Untitled Evaluation - 10 August 2026 - 2:29PM") and is +masked. + +Markers: visual, auth. +""" + +import io + +import pytest +from PIL import Image +from playwright.sync_api import Page + +from locators.evaluations_locators import EvaluationsLocators +from locators.evaluator_role_locators import EvaluatorRoleLocators +from pages.evaluator_role_page import EvaluatorRolePage +from pages.new_evaluation_page import NewEvaluationPage + +# Imported rather than copied: duplicating the diff maths would let this suite +# drift from the main one (different threshold, different baseline naming), and +# a silently-inconsistent second implementation is precisely the class of bug +# the capture-integrity work was cleaning up. +from tests.visual.test_visual_regression import _compare_or_save_baseline +from utils.visual_guards import ( + AUTH_PII_MASKS, + DEFAULT_MASKS, + capture_integrity_problem, + wait_for_render_settled, +) + +pytestmark = [pytest.mark.visual, pytest.mark.auth] + + +# Volatile content *inside* the dialogs. The evaluation-name field is prefilled +# with a wall-clock timestamp, so without this every run diffs 100% of the time. +_MODAL_VOLATILE_MASKS = [ + EvaluationsLocators.MODAL_EVAL_NAME_INPUT, +] + +_MODAL_MASKS = DEFAULT_MASKS + AUTH_PII_MASKS + _MODAL_VOLATILE_MASKS + + +@pytest.fixture +def page(authenticated_page_fast): + """Override: cached session, and — unlike building a raw context from + `authenticated_storage_state` — this fixture refreshes a stale storage + state before use, so a long suite can't capture a logged-out page.""" + return authenticated_page_fast + + +def _capture_dialog(page: Page, selector: str, snapshot_name: str) -> None: + """Element-scoped capture of a dialog, gated on capture integrity. + + Screenshot first, then gate: `capture_integrity_problem` inspects the whole + page's text (catching a 404 / curtain / logged-out fallback behind the + dialog) and the captured image (catching a blank panel), so it needs both. + """ + wait_for_render_settled(page) + + dialog = page.locator(selector).first + if not dialog.is_visible(): + pytest.skip(f"Dialog not visible for selector {selector!r} — nothing to capture") + + mask_locators = [] + for sel in _MODAL_MASKS: + loc = page.locator(sel) + if loc.count() > 0: + mask_locators.append(loc) + + raw = dialog.screenshot(mask=mask_locators or None) + img = Image.open(io.BytesIO(raw)) + + # expect_auth_route=True: these dialogs only exist behind the auth wall, so + # logged-out chrome underneath means bug #15 served the public homepage and + # whatever we just captured is not the dialog under test. + problem = capture_integrity_problem(page, img, expect_auth_route=True) + if problem: + pytest.skip(f"Capture integrity check failed for {snapshot_name}: {problem}") + + _compare_or_save_baseline(img, snapshot_name) + + +@pytest.fixture +def open_eval_modal(page: Page) -> NewEvaluationPage: + """Open 'Start an Evaluation' via the in-app flow. + + Deep-linking is avoided deliberately (lessons.md: cold deep-links to + protected routes intermittently bounce to sign-in). `click_new_evaluation` + already waits out the modal's own ~20s "Loading models" curtain. + """ + nep = NewEvaluationPage(page) + nep.go_to_evaluations_list() + nep.click_new_evaluation() + if not nep.is_modal_visible(): + pytest.skip("'Start an Evaluation' modal did not open") + return nep + + +# ──────────────────────────────────────────── Start an Evaluation modal + + +class TestStartEvaluationModalVisual: + """The two-step model-selection modal (`130b5ac` touched its method handling).""" + + def test_modal_step_1_desktop(self, open_eval_modal: NewEvaluationPage): + """Step 1: model/version selects, name, and the bulk|playground radios.""" + nep = open_eval_modal + # Pin the selection so the capture doesn't diff on dropdown ordering. + nep.select_first_model_and_version() + _capture_dialog( + nep.page, + EvaluationsLocators.MODAL_STEP_1, + "modal_start_evaluation_step1_desktop_1440x900", + ) + + def test_modal_step_1_playground_method_desktop( + self, open_eval_modal: NewEvaluationPage + ): + """Step 1 with the Playground method selected. + + Targets `130b5ac` ("update evaluation method handling in + ModelSelectionModal") directly: the bulk/playground choice is the part + that changed, and the selected-radio state is what a refactor there is + most likely to break visually. + """ + nep = open_eval_modal + nep.select_first_model_and_version() + try: + nep.select_evaluation_method("manual") + except Exception as exc: # noqa: BLE001 + # Same class of issue as completed_eval_id's UI-seeding fallback + # timing out: an intermittently slow/absent radio control is a + # flow reliability issue, not a visual regression — skip rather + # than fail so this doesn't flap the suite red. + pytest.skip(f"Could not select evaluation method in modal: {exc}") + _capture_dialog( + nep.page, + EvaluationsLocators.MODAL_STEP_1, + "modal_start_evaluation_step1_playground_desktop_1440x900", + ) + + def test_modal_step_2_desktop(self, open_eval_modal: NewEvaluationPage): + """Step 2: evaluator-type radios + objective textarea. + + The objective is left empty on purpose. That keeps the capture + deterministic and captures the *disabled* Start Evaluation button — and + it sidesteps app bug #20, where a filled form intermittently fails to + enable that button, which would make the baseline a coin flip. + """ + nep = open_eval_modal + nep.select_first_model_and_version() + nep.click_modal_next() + if nep.get_modal_step() != "2": + pytest.skip("Modal did not advance to step 2") + try: + nep.select_evaluator_type_in_modal("technical") + except Exception as exc: # noqa: BLE001 + pytest.skip(f"Could not select evaluator type in modal: {exc}") + _capture_dialog( + nep.page, + EvaluationsLocators.MODAL_STEP_2, + "modal_start_evaluation_step2_desktop_1440x900", + ) + + +# ──────────────────────────────────────────── Evaluator "Know More" modal + + +class TestKnowMoreModalVisual: + """The info dialog added by `90f38dc`. + + Only rendered inside the empty pending-invitations state on the evaluator + dashboard, so this skips whenever the account has pending invitations — + same guard the e2e coverage uses (test_evaluator_role.py). + """ + + def test_know_more_dialog_desktop(self, page: Page): + er = EvaluatorRolePage(page) + er.go_to_evaluator_home() + if not er.is_know_more_link_visible(): + pytest.skip( + "'Know More' link not rendered — only present in the empty " + "pending-invitations state" + ) + er.click_know_more() + if not er.is_know_more_dialog_visible(): + pytest.skip("'Know More' dialog did not open") + _capture_dialog( + page, + EvaluatorRoleLocators.KNOW_MORE_DIALOG, + "modal_know_more_desktop_1440x900", + ) diff --git a/tests/visual/test_visual_responsive.py b/tests/visual/test_visual_responsive.py new file mode 100644 index 0000000..d71e6ab --- /dev/null +++ b/tests/visual/test_visual_responsive.py @@ -0,0 +1,248 @@ +""" +Responsive visual regression for authenticated routes (mobile + tablet). + +Why this file exists +-------------------- +The homepage is covered at three viewports, but every one of the twelve +authenticated routes in `test_visual_regression.py` is captured **only** at +desktop 1440x900. The entire logged-in application therefore has zero +responsive visual coverage — sidebar collapse, stat-card reflow, card-grid +wrapping and data-table overflow are all untested, and those are precisely +where responsive regressions live. + +Route selection (the main design decision) +------------------------------------------ +Deliberately NOT the full 12 routes x 2 viewports = 24 tests. That would add +~24 baselines captured against an environment currently failing a majority of +authenticated captures (bug #15), and roughly double visual-suite runtime, for +very low marginal value. Four routes were chosen for genuinely distinct +reflow behaviour: + +* ``dashboard_role_selector`` — two side-by-side cards that must stack. The + simplest, most structurally stable authenticated page (static chrome, no + live data), so it is the one route here kept as a hard pixel assertion. +* ``ai_maker_dashboard`` — the richest reflow surface: a 4-up stat-card + grid plus a 3-column model-card grid, both of which must collapse. +* ``evaluations_list`` — a real data table. Horizontal overflow of a + table is the single most common responsive defect, which is also why the + overflow check below targets these same routes. +* ``models_list`` — 3-column card grid to single column. + +Explicitly excluded, with reasons: + +* ``org_selector`` — `tasks/lessons.md` records a persistent ~0.5% residual + diff on this page from a lazy-loaded placeholder widget that appears + depending on mouse position. It is already over the 0.1% threshold at + desktop; adding two more known-flaky baselines is negative value. +* ``new_evaluation_wizard`` — a ~35s asynchronous model dropdown makes it the + most timing-fragile page in the suite. +* the auditor routes — ``/dashboard/auditor/assignments`` is confirmed hanging + past 45s (app_bugs.md #24) and ``auditor_dashboard`` is currently the route + the capture-integrity gate refuses outright. +* ``evaluation_detail_completed`` — depends on the `completed_eval_id` + fixture, whose seeding fallback is currently intermittent. + +Capture integrity +----------------- +Every capture goes through `utils.visual_guards`. Eight of twelve desktop +baselines previously encoded a 404, a stuck "Loading ..." curtain, or the +logged-out marketing homepage, because captures settled a flat 2.5s while dev +curtains run 15-30s+. New baselines here must not repeat that, so the same +`wait_for_render_settled` + `capture_integrity_problem` pair gates every save +and every compare, and `AUTH_PII_MASKS` keeps the test account's name and +initials out of the images. + +Expect skips while bug #15 is active — a skip means the page never rendered, +so there was no pixel question to answer. That is the correct outcome, not +something to retry around. +""" + +import pytest + +# Helpers are imported from the desktop module rather than copied: duplicating +# the pixel-diff / threshold / baseline-save logic would create a second copy +# that silently drifts from the original (different threshold handling, a +# missed fix). The names are private by convention only — this is a sibling +# module inside the same `tests.visual` package. +from tests.visual.test_visual_regression import ( + _THRESHOLD, + _authenticated_page_at_viewport, + _capture_page_masked, + _compare_or_save_baseline, +) +from utils.config import Config +from utils.visual_guards import ( + AUTH_PII_MASKS, + DEFAULT_MASKS, + capture_integrity_problem, + wait_for_render_settled, +) + +pytestmark = [ + pytest.mark.visual, + pytest.mark.auth, + # Each capture budgets up to 45s waiting out the auth/data curtains, plus + # navigation and settle time. The global pytest timeout is 120s, which is + # uncomfortably close; test_new_evaluation_smoke.py raises it for the same + # reason. + pytest.mark.timeout(180), +] + + +# (path, snapshot name) — see the module docstring for why these four. +RESPONSIVE_ROUTES = [ + ("/dashboard", "dashboard_role_selector"), + ("/dashboard/ai-maker/1", "ai_maker_dashboard"), + ("/dashboard/ai-maker/1/evaluations", "evaluations_list"), + ("/dashboard/ai-maker/1/ai-models", "models_list"), +] + +# (label, width, height) — matches VIEWPORTS in the desktop module. +RESPONSIVE_VIEWPORTS = [ + ("mobile", 390, 844), + ("tablet", 768, 1024), +] + +# Routes whose content is live data and therefore not stably pixel-diffable; a +# diff becomes an xfail rather than a hard failure, mirroring VISUAL-002 in the +# desktop suite. +# +# Note this diverges from the desktop module, which treats `ai_maker_dashboard` +# as a hard assertion. That page now renders a live "Recent Evaluations" table +# and live stat counters; `DEFAULT_MASKS` covers timestamps but not evaluation +# names or counts, and the suite's own `regression_write` tests create +# evaluations. Treating it as deterministic would produce exactly the flaky red +# that erodes trust in a visual suite. Flagged rather than silently differing. +_NON_DETERMINISTIC_RESPONSIVE = { + "ai_maker_dashboard", + "evaluations_list", + "models_list", +} + +# Allowance for sub-pixel layout rounding when comparing scrollWidth against +# innerWidth. Deliberately tiny: a real horizontal-overflow bug overshoots by +# tens or hundreds of pixels, never by one. +_OVERFLOW_TOLERANCE_PX = 2 + + +def _capture_responsive(page, path: str): + """Capture `path`, skipping when the render can't be trusted. + + Returns the PIL image. Skips (never fails) on an untrustworthy capture, for + the reasons in utils/visual_guards: an invalid capture means the "did the + pixels change?" question was never asked. + """ + try: + img = _capture_page_masked( + page, Config.url(path), DEFAULT_MASKS + AUTH_PII_MASKS + ) + except Exception as exc: # noqa: BLE001 — mirrors the desktop suite's handling + pytest.skip( + f"Could not capture {path}: {exc}. " + "Page may be unreachable for this account." + ) + + problem = capture_integrity_problem(page, img, expect_auth_route=True) + if problem: + pytest.skip(f"Capture integrity check failed for {path}: {problem}") + return img + + +class TestResponsiveAuthenticatedVisuals: + """Mobile (390x844) and tablet (768x1024) baselines for four auth routes. + + Snapshot names follow the existing convention + (`auth___x`) so they sort next to their desktop + counterparts in `snapshots/`. + """ + + @pytest.mark.parametrize( + "viewport,width,height", + RESPONSIVE_VIEWPORTS, + ids=[v[0] for v in RESPONSIVE_VIEWPORTS], + ) + @pytest.mark.parametrize( + "path,name", RESPONSIVE_ROUTES, ids=[r[1] for r in RESPONSIVE_ROUTES] + ) + def test_authenticated_page_responsive( + self, browser, authenticated_storage_state, path, name, viewport, width, height + ): + page = _authenticated_page_at_viewport( + browser, authenticated_storage_state, width, height + ) + try: + img = _capture_responsive(page, path) + snapshot = f"auth_{name}_{viewport}_{width}x{height}" + + if name in _NON_DETERMINISTIC_RESPONSIVE: + try: + _compare_or_save_baseline(img, snapshot) + except AssertionError as exc: + pytest.xfail( + f"VISUAL-002: {name} renders live data (stat counters, " + f"evaluation/model lists) and is not stably pixel-diffable " + f"at {_THRESHOLD}%. {exc}" + ) + else: + _compare_or_save_baseline(img, snapshot) + finally: + page.context.close() + + +class TestMobileLayoutIntegrity: + """Horizontal-overflow assertions at mobile width. + + Deliberately NOT a pixel diff. A page wider than its viewport forces the + user to scroll sideways to read content — the most common real responsive + defect — and it is detectable deterministically from layout metrics. That + makes this check immune to the live-data noise that forces the baselines + above into xfail territory, so it keeps working as a hard assertion even on + pages whose pixels can't be trusted. + + A table that scrolls inside its own container is fine and correct; only + overflow of the document itself is a bug, which is what `documentElement` + measures. + """ + + @pytest.mark.mobile + @pytest.mark.parametrize( + "path,name", RESPONSIVE_ROUTES, ids=[r[1] for r in RESPONSIVE_ROUTES] + ) + def test_no_horizontal_overflow_at_mobile( + self, browser, authenticated_storage_state, path, name + ): + page = _authenticated_page_at_viewport( + browser, authenticated_storage_state, 390, 844 + ) + try: + page.goto(Config.url(path), wait_until="load", timeout=30_000) + wait_for_render_settled(page) + page.wait_for_timeout(1_500) + + # Same integrity gate as the pixel tests — asserting layout metrics + # on the logged-out homepage would be a meaningless green. + import io + + from PIL import Image + + img = Image.open(io.BytesIO(page.screenshot())) + problem = capture_integrity_problem(page, img, expect_auth_route=True) + if problem: + pytest.skip(f"Capture integrity check failed for {path}: {problem}") + + metrics = page.evaluate( + """() => ({ + scrollWidth: document.documentElement.scrollWidth, + innerWidth: window.innerWidth, + })""" + ) + overflow = metrics["scrollWidth"] - metrics["innerWidth"] + assert overflow <= _OVERFLOW_TOLERANCE_PX, ( + f"{name} overflows horizontally at 390px: document scrollWidth " + f"{metrics['scrollWidth']}px vs viewport {metrics['innerWidth']}px " + f"({overflow}px too wide). The user has to scroll sideways to read " + f"the page. A wide table should scroll inside its own container " + f"rather than widening the document." + ) + finally: + page.context.close() From 7e003790f6636cdb653cc47c3c805dc0088ecbba Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 11:47:03 +0530 Subject: [PATCH 07/35] File bug #25: evaluations table overflows at tablet width --- docs/app_bugs.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index 8f84b0b..b3e9900 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -29,6 +29,7 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 21 | ai-maker / breadcrumb | The deepest breadcrumb crumb on every `/dashboard/ai-maker/{orgId}/*` sub-page (AI Models, Evaluations, Auditors, etc.) shows the organization name or a static **"Dashboard"** fallback — never the actual section name ("Models", "Evaluations", etc.). Root cause confirmed in `app/[locale]/dashboard/ai-maker/[orgId]/layout.tsx:30`: the shared layout's `BreadCrumbs` config hardcodes the last crumb as `label: organization?.name \|\| "Dashboard"`, computed once at the org-layout level — sub-route pages (`ai-models/page.tsx`, etc.) never override or extend this with their own section label. Live repro on `/dashboard/ai-maker/1/ai-models`: last breadcrumb `
  • ` renders literally as `DashboardDashboard` (duplicated because the layout renders both a desktop and a mobile-truncated span for the same crumb). This was already known — flagged in a 2026-06-22 MCP exploration per this test file's own docstring — but never given a ledger row, so the `assert` was a bare failing assertion rather than a proper `xfail`. | open | Log in as `TEST_EMAIL_1` → navigate to `https://dev.parakh.civicdataspace.in/dashboard/ai-maker/1/ai-models`. Run `document.querySelector("[aria-label='breadcrumb'] li:last-child").textContent` → `"Dashboard"` (or the org name), never `"Models"`. Confirmed against `ParakhAI-frontend/app/[locale]/dashboard/ai-maker/[orgId]/layout.tsx:30` 2026-07-31. Two sibling assertions in the same test file (`test_evaluations_breadcrumb_is_not_dashboard`, `test_evaluation_detail_breadcrumb_has_org_name`) pass reliably in isolation — only the AI Models sub-page repros consistently. | `tests/e2e/test_breadcrumb.py::TestBreadcrumbLabels::test_models_breadcrumb_is_not_dashboard` | 2026-06-22 | | 20 | wizard / start-evaluation gating | The **Start Evaluation** button in the "Start an Evaluation" modal (step 2) intermittently never enables despite a fully valid form (evaluator type selected, non-empty objective typed via real keystrokes). First observed live 2026-07-02: identical keystroke input enabled the button in one session and not the next — suspected server-side validation state race, not a client-rendering issue (retyping/re-selecting fields doesn't reliably recover it within a session). This was previously only documented as an inline `xfail` comment in `test_add_evaluation_modal.py` without a numbered ledger row, which violates this file's own "every xfail must reference a row by id" convention — formalized here retroactively since the same defect surfaced independently in `test_evaluations.py` on 2026-07-28. | open | Open "Start an Evaluation" modal → step 2 → select an evaluator type → type an objective via real keystrokes (not `.fill()`) → wait up to 5s. The Start Evaluation button should enable but intermittently stays disabled. No client-side workaround found; re-selecting the evaluator type or retyping the objective does not reliably unstick it. | `tests/e2e/test_add_evaluation_modal.py::TestStartEvaluationValidation::test_start_enables_with_objective_and_disables_when_cleared`; `tests/e2e/test_evaluations.py::TestNewEvaluationModalStep2::test_start_requires_objective_filled` | 2026-07-02 | +| 25 | evaluations list / responsive | Evaluations table overflows horizontally at tablet width (768px) — the Model column is cut off at the right edge and the "Rows: 10" pagination control is clipped at the left edge. Found by new responsive visual coverage capturing `/dashboard/ai-maker/1/evaluations` at 768×1024. | open | Log in → AI Maker → Evaluations, at 768px viewport width. Table extends past the visible content area on both sides; no horizontal scroll affordance visible. | `tests/visual/test_visual_responsive.py::TestResponsiveAuthenticatedVisuals::test_authenticated_page_responsive[chromium-evaluations_list-tablet]` (baseline `auth_evaluations_list_tablet_768x1024.png` shows the clipped state) | 2026-08-12 | | 24 | auditor / assignments hang | `/dashboard/auditor/assignments` hangs indefinitely on "Loading your assignments…" — still curtained past a 45s budget. Found 2026-08-12 by the visual suite's new capture-integrity gate (`utils/visual_guards.py`), which refused to save a baseline of the stuck state. Same family as #13/#14 (a route-level data query that never resolves) but on a route those rows never covered, and notably this one did **not** clear within 45s in the same session where the AI-Maker overview and evaluations list both loaded fully — so it is not merely slow. | open | Log in as `TEST_EMAIL_1` → switch to Evaluator role → navigate to `https://dev.parakh.civicdataspace.in/dashboard/auditor/assignments`. Page renders sidebar/chrome but the content area stays on "Loading your assignments…" past 45s. | `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::test_authenticated_page_desktop[chromium-auditor_assignments]` (skips via the integrity gate rather than baselining the stuck state) | 2026-08-12 | | 23 | auth / backend | ParakhAPI's `DataSpaceAuthMiddleware` (`graphql_api/middleware/dataspace_user.py::process_request`) wraps its entire Bearer-token validation path (DataSpace `/api/auth/user/info/` lookup) in a blanket `except Exception`, and on **any** failure — expired token, invalid token, DataSpace being briefly unreachable — silently sets `request.user = AnonymousUser()` and continues. The failure is only `logger.warning`'d server-side; the client gets a normal `200` response shaped exactly like a legitimate anonymous request (e.g. `audits` returns `{data: [], totalItemsCount: 0}`, no `errors` array, nothing containing "token"/"expired"/401/403). A client has no way to distinguish "you are genuinely unauthenticated" from "your session just died" — it can only infer this indirectly from previously-authenticated-looking context. Root-caused 2026-08-11 while investigating 3 seemingly-flaky `ParakhAI_test` security tests that passed in isolation but failed only when reached ~4-5 minutes into a long suite run; confirmed by reading the middleware source directly. This is very likely the true root cause behind this file's own previously-vague "session expired mid-suite" attributions in the Phase 10 log entries (bug #3/#12 family) — those blamed Keycloak/dev-env instability in general, but the precise mechanism is this middleware's silent-swallow, which a well-behaved API should instead surface as a 401. | open | With a valid Bearer token, wait past the token's TTL (~5 min on dev), then issue any authenticated GraphQL query (e.g. `{ audits { data { id } totalItemsCount } }`) with the now-stale token. Response is `200` with the same shape as an anonymous request, no error. Confirmed against `ParakhAPI` `graphql_api/middleware/dataspace_user.py:99-104` (dev branch) 2026-08-11. | Indirectly, any test using `tests/conftest.py::authenticated_graphql_client` on a long-running suite — see the fixture's own docstring (fixed 2026-08-11 to proactively refresh before this can bite) for the test-side mitigation; this row documents the underlying product-side gap that mitigation works around. | 2026-08-11 | | 22 | audits query / backend perf | Sorting the `audits` GraphQL query by `passed_tests` is measurably slower than the default `created_at` ordering under concurrent load. 5 concurrent authenticated `audits(sortOptions: [{field: "passed_tests", direction: "desc"}])` requests (light field selection: `id status passedTests` only, `limit: 20`) took 9-15s each on the cleanest run and up to 35s under noisier conditions, vs. a 6s budget for the equivalent unsorted 5-concurrent read (`TestConcurrentGraphQLReads`). 100% success, zero errors — not a correctness bug, but the delta suggests the `(organization, -created_at)` index added for this query (commit `4f51ab2`) doesn't help an `ORDER BY passed_tests`, which has no index of its own. Worth a backend follow-up (composite/partial index, or precomputing `passed_tests` ordering) if sort-by-pass-rate becomes a real UI feature rather than just an API capability. | open | Run `pytest tests/load/test_load_graphql.py -m load -v -k TestAuditsSortPerformance` — compare reported latencies in `reports/load_metrics_graphql.json` (`concurrent_audits_sorted_n5`) against the unsorted baseline in the same file. | `tests/load/test_load_graphql.py::TestAuditsSortPerformance::test_5_concurrent_audits_sorted_by_passed_tests` | 2026-08-11 | From d6775e2532f1e4f7e5229bd169855bb50035ba39 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 13:55:28 +0530 Subject: [PATCH 08/35] Mark bugs #13/#14 fixed after direct re-verification Targeted repro (poll body text until the loading-curtain regex clears or a 60s budget expires - the disambiguation procedure this file already prescribes) shows both routes rendering real content well within budget: the AI-Maker overview in 28.2s with populated stat cards, the evaluations list in 8.1s with real table headers. Second independent confirmation after Phase 11's visual-suite runs also failed to reproduce either hang the same day. Xfails referencing #13/#14 in the e2e/accessibility suites are not yet removed - that cleanup is a follow-up, per this file's own convention of re-running the linked test before dropping the xfail. --- docs/app_bugs.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index b3e9900..a9f781a 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -19,8 +19,8 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 10 | wizard / cancel | The **Cancel Evaluation** button (`"button:has-text('Cancel Evaluation')"` and its fallback selectors) is not found in the wizard after clicking **Start** from the modal. The wizard Configuration tab renders successfully (`WIZARD_TAB_CONFIGURATION` is visible) but the Cancel Evaluation button never appears within 5 seconds. Previously hidden by a `pytest.skip` guard — now surfaces as a failure after the guard was converted to `assert`. Could be a timing issue (the button renders after a further delay) or the button text / element has changed. | open | Log in → AI Maker → CivicDataLab → New Evaluation → Start → wait for wizard. Inspect DOM for `button` elements containing "Cancel" — the button may be absent or have a different label. | `tests/e2e/test_evaluations.py::TestNewEvaluationWizard::test_cancel_evaluation_returns_to_list` | 2026-05-23 | | 12 | auth / keycloak-dev | **Keycloak dev instance rate-limits ≥5 concurrent logins from the same account.** Under a 5-parallel-login load test, 3/5 workers fail with "login form not rendered" even after a 15 s wait — the form simply never appears. The 3-concurrent test (same account) passes consistently. Root causes (any combination): Keycloak brute-force-protection window, single-node session-table lock contention, or the dev nginx upstream thread pool exhaustion. **Not a bug in the Parakh frontend.** Upgrade path: HA Keycloak with session affinity disabled and per-user rate-limit tuning. | open (infra) | Run `pytest tests/load/test_load.py::TestConcurrentAuthentication -v -s` and observe 3/5 workers timing out on the login form. The 3-concurrent variant in the same class passes. | `tests/load/test_load.py::TestConcurrentAuthentication::test_5_concurrent_logins_success_rate` | 2026-05-23 | | 11 | wizard / manual-workspace | The manual-evaluation workspace Test Cases tab fails to render its expected UI elements when reached via the Domain type flow: (a) module cards appear (counter confirmed) but clicking the first card does not open an input/output panel — neither `MANUAL_INPUT_TEXTAREA` nor `MANUAL_CHANGE_MODULE_LINK` becomes visible; (b) the minimum test-cases hint ("Evaluate at least 3 test cases per module…") is not rendered on the Test Cases tab. These were previously skipped with `pytest.skip` guards; they now surface as failures. | open | Log in → AI Maker → CivicDataLab → New Evaluation → Start → select Domain type → fill Objective → click Test Cases tab. Click any module card and observe whether an input panel appears. Also check whether the min-cases hint text is rendered below the module list. | `tests/e2e/test_evaluation_workspace_manual.py::TestManualWorkspaceTestEntry::test_module_click_opens_input_panel`; `tests/e2e/test_evaluation_workspace_manual.py::TestManualWorkspaceTestEntry::test_min_test_cases_note_visible` | 2026-05-23 | -| 13 | evaluations list / backend perf | The frontend `GetAudits` query that powers the AI-Maker Evaluations list (`/dashboard/ai-maker/{org}/evaluations`) **hangs and never returns** on dev — the page sits on "Loading evaluations…" indefinitely (still up at 90 s). Root-caused 2026-06-12 via live Playwright + direct GraphQL probing on org 1 (981 total audits): requesting the **light** field set (`id name status modelId`, `limit:100`) returns in ~10.6 s, and `metrics`-only or `modules`-only at `limit:100` each return in ~16–18 s, but the **full field set the frontend actually requests** (`name modelName status modules metrics evaluationMode auditType totalTests passedTests failedTests createdAt startedAt completedAt`) **times out past 30 s even at `limit:5`** — so it is the combined per-row resolver cost (likely N+1 on `metrics`/`modules`/test-count fields), not row volume. Backend should batch/precompute these fields or paginate server-side. Also observed intermittent `ERR_NETWORK_CHANGED` / "Backend server is not available" GraphQL errors on dev during the same session (degraded backend). **Not a test defect** — no client-side timeout bump can help since the query never completes. | open | Log in → AI Maker → CivicDataLab → Evaluations. Page stays on "Loading evaluations…". In DevTools, the `GetAudits` POST to `dev.api.parakh.civicdataspace.in/graphql/` has no response after 90 s. Confirmed via Playwright MCP 2026-06-12: full-field `audits` query aborts at 30 s even with `limit:5`, while light-field returns in ~10 s. | `tests/e2e/test_evaluations.py::TestStatusFilterTabs::*` (all 6 — tabs/column/rows render only after the list query resolves); transitively any test that asserts on rendered evaluations-list content for org 1; `tests/e2e/test_evaluations.py::TestEvaluationsListPage::test_table_column_headers_are_present` (added 2026-07-31 — re-run isolated, no concurrent CI load: page still stuck on "Loading evaluations..." past 40s, confirming the hang reproduces without concurrent-shard traffic) | 2026-06-12 | -| 14 | a11y / dashboard heading | The AI Maker Dashboard (`/dashboard/ai-maker/1`) never renders any `

    `/`

    `/`

    ` heading — `document.querySelectorAll('h1,h2,h3').length` is `0` even after a 30s wait. Root cause: the "Overview" widget (which appears to be where the page heading lives) is permanently stuck on "Loading overview..." and never resolves — the same failure shape as bug #13 (backend `GetAudits`/overview resolver hang), just surfacing on the dashboard-home overview query instead of the evaluations-list query. Because the widget never mounts, the page has zero heading elements, violating WCAG 1.3.1 (Info and Relationships) / a broken document outline for screen-reader users. | open | Log in as `TEST_EMAIL_1` → navigate directly to `https://dev.parakh.civicdataspace.in/dashboard/ai-maker/1`. Page loads nav/sidebar/breadcrumbs but the main content area shows only "Loading overview..." indefinitely (confirmed stuck past 30s). Run `document.querySelectorAll('h1,h2,h3').length` → `0`. Confirmed via Playwright 2026-07-10. | `tests/accessibility/test_accessibility_auth.py::TestDashboardAccessibility::test_dashboard_has_exactly_one_h1`; `tests/e2e/test_ai_maker_dashboard.py::TestAIMakerDashboardLoads::test_civicdatalab_name_visible_in_sidebar`, `TestModelCardsonHome::test_add_new_model_button_is_visible`, `TestModelCardsonHome::test_model_cards_are_rendered`, `TestModelCardsonHome::test_text_generation_badge_is_present` (added 2026-07-31 — re-run isolated (no concurrent CI load), direct repro script confirmed the whole home content area, not just the heading, is gated behind the same stuck "Loading overview..." widget: org identity block (`CivicdataLab` text), model cards, `Add A New Model` button, and model-type badges never render because they all read from the same overview payload) | 2026-07-10 | +| 13 | evaluations list / backend perf | The frontend `GetAudits` query that powers the AI-Maker Evaluations list (`/dashboard/ai-maker/{org}/evaluations`) **hangs and never returns** on dev — the page sits on "Loading evaluations…" indefinitely (still up at 90 s). Root-caused 2026-06-12 via live Playwright + direct GraphQL probing on org 1 (981 total audits): requesting the **light** field set (`id name status modelId`, `limit:100`) returns in ~10.6 s, and `metrics`-only or `modules`-only at `limit:100` each return in ~16–18 s, but the **full field set the frontend actually requests** (`name modelName status modules metrics evaluationMode auditType totalTests passedTests failedTests createdAt startedAt completedAt`) **times out past 30 s even at `limit:5`** — so it is the combined per-row resolver cost (likely N+1 on `metrics`/`modules`/test-count fields), not row volume. Backend should batch/precompute these fields or paginate server-side. Also observed intermittent `ERR_NETWORK_CHANGED` / "Backend server is not available" GraphQL errors on dev during the same session (degraded backend). **Not a test defect** — no client-side timeout bump can help since the query never completes. | fixed 2026-08-12 (see verification note) | Log in → AI Maker → CivicDataLab → Evaluations. Page stays on "Loading evaluations…". In DevTools, the `GetAudits` POST to `dev.api.parakh.civicdataspace.in/graphql/` has no response after 90 s. Confirmed via Playwright MCP 2026-06-12: full-field `audits` query aborts at 30 s even with `limit:5`, while light-field returns in ~10 s. | `tests/e2e/test_evaluations.py::TestStatusFilterTabs::*` (all 6 — tabs/column/rows render only after the list query resolves); transitively any test that asserts on rendered evaluations-list content for org 1; `tests/e2e/test_evaluations.py::TestEvaluationsListPage::test_table_column_headers_are_present` (added 2026-07-31 — re-run isolated, no concurrent CI load: page still stuck on "Loading evaluations..." past 40s, confirming the hang reproduces without concurrent-shard traffic) | 2026-06-12 | +| 14 | a11y / dashboard heading | The AI Maker Dashboard (`/dashboard/ai-maker/1`) never renders any `

    `/`

    `/`

    ` heading — `document.querySelectorAll('h1,h2,h3').length` is `0` even after a 30s wait. Root cause: the "Overview" widget (which appears to be where the page heading lives) is permanently stuck on "Loading overview..." and never resolves — the same failure shape as bug #13 (backend `GetAudits`/overview resolver hang), just surfacing on the dashboard-home overview query instead of the evaluations-list query. Because the widget never mounts, the page has zero heading elements, violating WCAG 1.3.1 (Info and Relationships) / a broken document outline for screen-reader users. | fixed 2026-08-12 (see verification note) | Log in as `TEST_EMAIL_1` → navigate directly to `https://dev.parakh.civicdataspace.in/dashboard/ai-maker/1`. Page loads nav/sidebar/breadcrumbs but the main content area shows only "Loading overview..." indefinitely (confirmed stuck past 30s). Run `document.querySelectorAll('h1,h2,h3').length` → `0`. Confirmed via Playwright 2026-07-10. | `tests/accessibility/test_accessibility_auth.py::TestDashboardAccessibility::test_dashboard_has_exactly_one_h1`; `tests/e2e/test_ai_maker_dashboard.py::TestAIMakerDashboardLoads::test_civicdatalab_name_visible_in_sidebar`, `TestModelCardsonHome::test_add_new_model_button_is_visible`, `TestModelCardsonHome::test_model_cards_are_rendered`, `TestModelCardsonHome::test_text_generation_badge_is_present` (added 2026-07-31 — re-run isolated (no concurrent CI load), direct repro script confirmed the whole home content area, not just the heading, is gated behind the same stuck "Loading overview..." widget: org identity block (`CivicdataLab` text), model cards, `Add A New Model` button, and model-type badges never render because they all read from the same overview payload) | 2026-07-10 | | 15 | routing / auth deep-link | Direct navigation to authenticated AI-Maker / Auditor sub-routes (`/dashboard/ai-maker/1/ai-models`, `/dashboard/ai-maker/1/evaluations/new`, `/dashboard/ai-maker/1/auditors`, `/dashboard/ai-maker/1/prompt-libraries`, `/dashboard/auditor`) **intermittently renders the public marketing homepage** (hero "Build AI that's trustworthy from day one.", feature tabs, CTA cards, footer) instead of the requested dashboard page, even though the user is authenticated (avatar/session valid) and the URL bar shows the correct path. Root-caused 2026-07-10 via a scripted repro: a fresh browser context seeded with a valid `storage_state` (cookies + localStorage from a real Keycloak login) navigating straight to `/dashboard/ai-maker/1/ai-models` with `wait_until="load"` + 2.5s settle reproduced the homepage fallback in **1 of 6 trials (~17%)**; the same repro against `/dashboard/ai-maker/1/prompt-libraries` reproduced it in **1 of 4 trials (~25%)**, and a further capture landed in a visibly half-transitioned state (both "Prompt Libraries" page chrome and homepage CTA/footer content stacked in the same viewport — see `screenshots/DIFF_auth_prompt_libraries_desktop_1440x900.png`). When it does NOT reproduce, the correct page renders fully and correctly (light theme, correct heading, working sidebar, "Loading AI models…" spinner resolving normally). This is a materially different and more severe symptom than the "non-deterministic async content" (live stats, model dropdown ordering) that `_NON_DETERMINISTIC_VISUAL` in `tests/visual/test_visual_regression.py` currently attributes these routes' diffs to — the wizard/models/auditors/auditor-dashboard visual tests see ~43% pixel diffs because they are sporadically capturing the wrong page entirely, not just varying live data. Likely a client-side routing/auth-guard race on hard navigation (the route guard falls through to the default/public route before the session check resolves), since it only reproduces on deep-link/full-page loads, not in-app client-side navigation. | open | Using a Playwright context seeded with a valid Keycloak `storage_state`, repeatedly `page.goto()` directly to `/dashboard/ai-maker/1/ai-models` (or any route in the list above) with `wait_until="load"` and a 2.5s+ settle delay; repeat ~5-6 times. Roughly 1 in 5-6 loads renders the homepage hero/CTA/footer instead of the dashboard page content. Confirmed via ad hoc Playwright script 2026-07-10 (not yet encoded as a standing test — see `docs/visual_diffs.md` Run 2026-07-10 for the diff images that first surfaced it). | `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::test_authenticated_page_desktop[chromium-models_list]`, `[chromium-new_evaluation_wizard]`, `[chromium-auditors_management]`, `[chromium-auditor_dashboard]`, `[chromium-prompt_libraries]` (all xfail under VISUAL-002 — the xfail reason text should be revisited given this finding); `tests/e2e/test_user_flows.py::TestFlow04_EvaluationDetailAndReport::test_evaluation_detail_shows_full_results` (added 2026-07-13 — deep-link to `/evaluations/{id}` rendered the public homepage; avatar/session chrome still present, confirming auth was intact); `tests/visual/test_visual_regression.py::TestAuthenticatedPageVisuals::*` (2026-08-12 — the new capture-integrity gate in `utils/visual_guards.py` now detects this automatically and skips instead of baselining it) **— SEVERITY UPDATE 2026-08-12: observed at ~50%, far above the 17-25% recorded above.** Two consecutive full visual runs hit the logged-out fallback on **12 of 24 authenticated captures** (5/12 then 7/12). Affected: `/dashboard/ai-maker/1`, `/ai-models`, `/evaluations`, `/evaluations/new`, `/auditors`, `/prompt-libraries`, `/dashboard/auditor`. NOT affected in either run: `/dashboard` (role selector), `/dashboard/ai-maker` (org selector), `/dashboard/auditor/evaluations` — so it is not uniform across depth. New detail vs the 2026-07-10 characterisation: the fallback now renders **logged-out chrome (LOGIN / SIGN UP)**, i.e. the session is not merely failing to apply to the route — the shell itself renders unauthenticated. Caveat on the comparison: both figures are small samples (the original was 1/6 and 1/4 trials) and load conditions differed, so treat this as "materially worse under current conditions" rather than a precise regression measurement. Independent confirmation that it also fired historically: four baselines were byte-identical copies of the logged-out homepage. | 2026-07-10 | | 16 | SEO / homepage content | Homepage `` contains a spelling error: `"Paricipatory AI Evaluation"` (missing the first `t` in "Participatory"). Root-caused in frontend source: `app/layout.tsx` line 180, `export const metadata = { ..., description: "Paricipatory AI Evaluation" }`. One-line content fix. | open | Load `https://dev.parakh.civicdataspace.in/`. Run `document.querySelector('meta[name="description"]').content` → `"Paricipatory AI Evaluation"`. Confirmed against `ParakhAI-frontend/app/layout.tsx:180` 2026-07-13. | `tests/e2e/test_platform_issues.py::TestHomepageSEO::test_meta_description_has_no_typo` | 2026-07-13 | | 17 | SEO / homepage content | Homepage has no Open Graph `` tag (or any other `og:*` tags) — the root `metadata` export in `app/layout.tsx` only sets `title` and `description`, no `openGraph` block. Links to the homepage shared on social media / chat apps will render with no title card. | open | Load `https://dev.parakh.civicdataspace.in/`. Run `document.querySelector('meta[property="og:title"]')` → `null`. Confirmed against `ParakhAI-frontend/app/layout.tsx` (metadata export has no `openGraph` key) 2026-07-13. | `tests/e2e/test_platform_issues.py::TestHomepageSEO::test_og_title_is_present` | 2026-07-13 | @@ -69,3 +69,4 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th - Full target list re-run after the xfail changes: 3 passed / 1 skipped / 6 xfailed / 0 failed. - **Phase 9 — CI failure triage: bulk/modal/wizard cluster** (2026-07-31): re-ran all 17 shard-1 failures from this cluster (`test_add_evaluation_bulk.py`, `test_add_evaluation_modal.py`, `test_breadcrumb.py`, `test_bulk_evaluation_flow.py`, `test_add_evaluation_playground.py`) in isolation with `-n 1`, `--reruns 0`, no other heavy suites running concurrently. **16 of 17 were pure CI-concurrency noise** (bug #3 family) — all passed/skipped-as-designed cleanly once the shared dev backend wasn't being hammered by 3 E2E shards + API tests simultaneously; no code changes needed for those. **1 genuine, previously-unlogged bug found**: `test_breadcrumb.py::test_models_breadcrumb_is_not_dashboard` — filed as bug #21 (breadcrumb on every AI-Maker sub-page shows the org name or a static "Dashboard" fallback instead of the actual section name; root-caused to the shared `[orgId]/layout.tsx` breadcrumb config never being overridden per-route). The test file's own docstring already flagged this from a 2026-06-22 MCP exploration but it had never been given a ledger row or `xfail` marker — fixed that gap. Final state: all 17 tests pass/xfail correctly in isolation. - **Phase 11 — visual baseline integrity audit** (2026-08-12): audited every committed visual baseline rather than adding new visual tests, after noticing the suite was green while covering pages it had never actually captured. **8 of 12 authenticated baselines were invalid** — they encoded a broken or wrong page: `evaluation_detail_completed` was a **404** (the route `/evaluation/288` does not exist; the real ones are `/dashboard/ai-maker/[orgId]/evaluations/[id]` and `/dashboard/auditor/evaluations/[id]`), `ai_maker_dashboard` was a stuck "Loading overview…", `evaluations_list` was a false-empty state, `org_selector` was a bare "Loading" spinner, and four (`auditor_dashboard`, `auditors_management`, `models_list`, `new_evaluation_wizard`) were **pixel-identical to the logged-out public homepage** — bug #15's deep-link fallback frozen into baselines (found by checksum: five files shared one md5 at 348560 bytes). Because most sat under hard pixel-diff assertions, they passed *because* the app was broken; `ai_maker_dashboard` would have gone red the day bug #14 was fixed. Root cause was not the gate's absence but the wait: `_capture_page_masked` settled a flat **2.5s** after `load`, while this repo's own `lessons.md` documents curtains at 15-30s+ — so captures landed mid-curtain by design. Fixes: `wait_for_render_settled()` (45s budget) plus `utils/visual_guards.py`, which refuses to save *or* compare a capture that is a 404, a loading curtain, logged-out chrome on an auth route, or a blank image (it skips with a reason — an invalid capture means "did the pixels change?" was never asked, so the honest result is no-signal, not a pass). 7 baselines also carried the test account's real name/initials; identity masks were verified active *before* regeneration, since the 4 homepage-fallback files were PII-free only by virtue of being logged out and would otherwise have gained PII on re-capture. Outcome: 8 regenerated clean and verified against real content, `auditor_dashboard` correctly refused (bug #15 firing live — the gate caught 6 auth routes serving logged-out chrome in a single run, confirming #15 is active *now*, not historical), and `evaluation_detail_completed` still has no baseline because `completed_eval_id` found no audit meeting its bar and its UI-seeding fallback timed out (pre-existing fixture breakage, not visual). **Caveat worth acting on: bugs #13/#14 did not reproduce in this session** — both the AI-Maker overview and the evaluations list loaded fully within 45s. That may mean the merged backend perf work (PR #95/#96) genuinely fixed them, or that they are intermittent; either way the *visual* suite's evidence for those rows was unreliable, since it was capturing at 2.5s. Re-verify independently before removing their xfails. Filed #24 (auditor assignments genuinely hangs past 45s — a route neither #13 nor #14 covered). Also corrected a repeated claim: `snapshots/` is **gitignored** (`.gitignore:36`), so baselines were never in git — the PII exposure is via the public repo's CI artifacts (30-day retention) and Actions cache, not git history. +- **Phase 12 — direct re-verification of bugs #13/#14** (2026-08-12): both were re-checked with a targeted repro (throwaway script, not committed, per this file's own convention for `_dbg_*` scripts) that logs in fresh, navigates to each route, and polls `document.body.innerText` every 2s against a 60s budget until the loading-curtain regex clears or the budget expires — the exact disambiguation procedure this file already prescribes for "stuck curtain vs slow-but-completes." **Both cleared with real content**, not a curtain: `/dashboard/ai-maker/1` (bug #14) rendered in 28.2s with 3 headings and populated stat cards (Evaluations Completed: 4, Test Cases Evaluated: 59, Models Added: 17); `/dashboard/ai-maker/1/evaluations` (bug #13) rendered in 8.1s with real sortable table headers (Evaluation Name, Mode, ...). This is the second independent confirmation — the visual-suite work earlier this same day (Phase 11) also failed to reproduce either hang across two full runs. Marked `fixed 2026-08-12` on both rows. Plausible cause: the merged backend perf work (dev cutover runserver→gunicorn, Celery containerization — see project memory "ParakhAPI audits perf investigation", PRs #95/#96). **Not yet done: the xfails referencing #13/#14 in the e2e/accessibility suites still need to be removed** — this file's own convention says re-run the linked test and only then drop the xfail; that re-run/cleanup is a follow-up, not done in this phase. Caveat: this is evidence from a shared, sometimes-flaky dev environment on one session — a recurrence should reopen the row rather than be filed as new. From b3791c9ff215d497e29c72a0c94d48c951ee115b Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 15:33:46 +0530 Subject: [PATCH 09/35] Bump CI job timeouts from 30 to 60 minutes Applies to e2e-tests, accessibility-tests, visual-tests. The visual suite alone ran 11.5 min on a cache-cold run today (every baseline saving for the first time under the new v2- cache generation); 30 min was tight headroom for a slow or retry-heavy run. test-summary's separate 10-minute timeout is untouched - it does no browser work. --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd30f27..b1882a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -125,7 +125,7 @@ jobs: # `needs.e2e-tests.result != 'skipped'` and degrade cleanly while this # is off. if: false - timeout-minutes: 30 + timeout-minutes: 60 strategy: fail-fast: false matrix: @@ -179,7 +179,7 @@ jobs: runs-on: ubuntu-latest needs: api-tests if: always() - timeout-minutes: 30 + timeout-minutes: 60 steps: - uses: actions/checkout@v6 @@ -224,7 +224,7 @@ jobs: runs-on: ubuntu-latest needs: accessibility-tests if: always() - timeout-minutes: 30 + timeout-minutes: 60 steps: - uses: actions/checkout@v6 From 278ce2303175c53e773b1f767f441e62e27a15ee Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 16:12:16 +0530 Subject: [PATCH 10/35] Mask ai_maker_dashboard's live counters instead of xfail-ing the page Both visual suites hit the same problem: the AI-Maker overview's stat cards (Evaluations Completed, etc.) and Recent Evaluations table change during normal platform use, so a hard pixel-diff assertion flapped red on ordinary re-runs. The responsive suite had already worked around this by adding the page to its non-deterministic xfail set - which silences diffs for the WHOLE page, including the static layout and model-card grid a real regression would land in. Adds route_masks() to utils.visual_guards: a per-route mask lookup, verified against the live DOM logged in as TEST_EMAIL_2 so the selectors aren't tied to one account's data. Masks only .metric-card-value and the live table rows; the four stat labels, headings, and model cards keep a real hard assertion. Wired into both the desktop and responsive suites, and ai_maker_dashboard is removed from the responsive suite's non-deterministic set now that it's genuinely stable - confirmed 4/4 passed on a same-session re-run with zero skips. --- tests/visual/test_visual_regression.py | 15 +++++----- tests/visual/test_visual_responsive.py | 26 ++++++++--------- utils/visual_guards.py | 40 ++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/tests/visual/test_visual_regression.py b/tests/visual/test_visual_regression.py index bd002f1..5ed4388 100644 --- a/tests/visual/test_visual_regression.py +++ b/tests/visual/test_visual_regression.py @@ -16,9 +16,8 @@ from utils.config import Config from utils.visual_guards import ( - AUTH_PII_MASKS, - DEFAULT_MASKS, capture_integrity_problem, + route_masks, wait_for_render_settled, ) @@ -169,10 +168,9 @@ def _capture_page_masked(page: Page, url: str, masks: list) -> Image.Image: # Mask lists live in utils.visual_guards so sibling visual suites share one -# definition — a new module that forgot _AUTH_PII_MASKS would quietly start -# baking the account name back into baselines. -_DEFAULT_MASKS = DEFAULT_MASKS -_AUTH_PII_MASKS = AUTH_PII_MASKS +# definition — a new module that forgot AUTH_PII_MASKS would quietly start +# baking the account name back into baselines. route_masks(name) layers on +# any route-specific masks (e.g. ai_maker_dashboard's live stat counters). # Auth routes whose content is genuinely non-deterministic between two captures # against the live/shared dev environment: the New Evaluation wizard's model @@ -335,7 +333,8 @@ class TestAuthenticatedPageVisuals: "baseline saved" message. Subsequent runs diff against the cached baseline at Config.VISUAL_THRESHOLD (default 0.1%). - Dynamic regions are masked via _DEFAULT_MASKS to prevent flaky diffs. + Dynamic regions are masked via route_masks(name) (utils.visual_guards) to + prevent flaky diffs. Tests are parametrized by (path, name); each is independent so a failure on one page doesn't mask the others. """ @@ -402,7 +401,7 @@ def test_authenticated_page_desktop( try: try: img = _capture_page_masked( - page, Config.url(path), _DEFAULT_MASKS + _AUTH_PII_MASKS + page, Config.url(path), route_masks(name) ) except Exception as exc: # noqa: BLE001 pytest.skip( diff --git a/tests/visual/test_visual_responsive.py b/tests/visual/test_visual_responsive.py index d71e6ab..8aeb788 100644 --- a/tests/visual/test_visual_responsive.py +++ b/tests/visual/test_visual_responsive.py @@ -72,9 +72,8 @@ ) from utils.config import Config from utils.visual_guards import ( - AUTH_PII_MASKS, - DEFAULT_MASKS, capture_integrity_problem, + route_masks, wait_for_render_settled, ) @@ -107,14 +106,15 @@ # diff becomes an xfail rather than a hard failure, mirroring VISUAL-002 in the # desktop suite. # -# Note this diverges from the desktop module, which treats `ai_maker_dashboard` -# as a hard assertion. That page now renders a live "Recent Evaluations" table -# and live stat counters; `DEFAULT_MASKS` covers timestamps but not evaluation -# names or counts, and the suite's own `regression_write` tests create -# evaluations. Treating it as deterministic would produce exactly the flaky red -# that erodes trust in a visual suite. Flagged rather than silently differing. +# `ai_maker_dashboard` was here too until 2026-08-12, blanket-xfailed for the +# same live-stat-counter reason given below. That silenced diffs across the +# WHOLE page — layout, model cards, structure included — not just the counters. +# Fixed properly instead: `route_masks()` (utils/visual_guards) masks just +# `.metric-card-value` and the live evaluations table, verified against the live +# DOM logged in as TEST_EMAIL_2. It's now a real hard assertion again, matching +# the desktop suite. `evaluations_list` / `models_list` remain here unmasked — +# out of scope for that fix; they'd need the same treatment before removal. _NON_DETERMINISTIC_RESPONSIVE = { - "ai_maker_dashboard", "evaluations_list", "models_list", } @@ -125,7 +125,7 @@ _OVERFLOW_TOLERANCE_PX = 2 -def _capture_responsive(page, path: str): +def _capture_responsive(page, path: str, name: str): """Capture `path`, skipping when the render can't be trusted. Returns the PIL image. Skips (never fails) on an untrustworthy capture, for @@ -133,9 +133,7 @@ def _capture_responsive(page, path: str): pixels change?" question was never asked. """ try: - img = _capture_page_masked( - page, Config.url(path), DEFAULT_MASKS + AUTH_PII_MASKS - ) + img = _capture_page_masked(page, Config.url(path), route_masks(name)) except Exception as exc: # noqa: BLE001 — mirrors the desktop suite's handling pytest.skip( f"Could not capture {path}: {exc}. " @@ -171,7 +169,7 @@ def test_authenticated_page_responsive( browser, authenticated_storage_state, width, height ) try: - img = _capture_responsive(page, path) + img = _capture_responsive(page, path, name) snapshot = f"auth_{name}_{viewport}_{width}x{height}" if name in _NON_DETERMINISTIC_RESPONSIVE: diff --git a/utils/visual_guards.py b/utils/visual_guards.py index 33e15b7..802395a 100644 --- a/utils/visual_guards.py +++ b/utils/visual_guards.py @@ -243,3 +243,43 @@ def capture_integrity_problem( "text=/^Welcome,/ >> xpath=..", # fallback if .welcome-text is renamed "button[aria-label='Open profile']", # per locators/dashboard_locators.py ] + +# Per-route masks for content that is genuinely live (not identity, not a11y +# noise), keyed by the `name` half of AUTH_ROUTES / RESPONSIVE_ROUTES tuples. +# +# ai_maker_dashboard was failing its hard pixel-diff assertion on ordinary re-runs +# purely because its stat counters increment during normal platform use (observed +# 2026-08-12: "Evaluations Completed" 3 -> 4 between two runs) — not a rendering +# bug. The blanket fix would be adding it to _NON_DETERMINISTIC_VISUAL, but that +# silences the diff for the WHOLE page, including the static layout/model-card +# grid a real regression would land in. Masking just the volatile numbers keeps +# everything else — the "Overview" heading, the four *labels*, model cards, +# structure — under a real hard assertion. +# +# Verified against the live DOM 2026-08-12 (logged in as TEST_EMAIL_2, since the +# masks must not be tied to one account's data): +#
    +#
    +#

    Evaluations Completed

    <- static, NOT masked +#

    4

    <- volatile, masked +#
    +# ... (Test Cases Evaluated / Models Added / Issues Flagged, same shape) +#
    +#
    +#

    Recent Evaluations

    See all <- static, NOT masked +# ...
    <- live rows, masked +#
    +# The DataTable-module_* class is an auto-generated CSS-module hash that will +# change on any frontend rebuild — mask by the semantic `table` tag scoped under +# the stable `.audits-section` class instead, not the hashed class. +_ROUTE_EXTRA_MASKS: dict[str, list[str]] = { + "ai_maker_dashboard": [ + "[class*='metric-card-value' i]", + ".audits-section table", + ], +} + + +def route_masks(name: str) -> list[str]: + """DEFAULT_MASKS + AUTH_PII_MASKS + any masks specific to route *name*.""" + return DEFAULT_MASKS + AUTH_PII_MASKS + _ROUTE_EXTRA_MASKS.get(name, []) From 6b3eb9e66d7900ed64392b6012920e62a5cae6d2 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 16:45:50 +0530 Subject: [PATCH 11/35] Fix health endpoint non-GET assertion: 403 (CSRF), not 405 The health endpoints are now deployed to dev, so test_rejects_non_get ran for real for the first time and failed: assumed 405 from @require_GET, but Django's CsrfViewMiddleware intercepts a POST with no CSRF cookie before that decorator ever runs, returning 403 ('CSRF verification failed') instead. Confirmed live via direct curl. Accept either code - the test's concern is that non-GET is rejected, not which middleware rejects it first. --- tests/api/test_health_endpoints.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/api/test_health_endpoints.py b/tests/api/test_health_endpoints.py index 6dd6d17..ad3b1da 100644 --- a/tests/api/test_health_endpoints.py +++ b/tests/api/test_health_endpoints.py @@ -70,9 +70,16 @@ def test_responds_fast(self, api_client): ) def test_rejects_non_get(self, api_client): + """Confirmed live 2026-08-12: Django's CsrfViewMiddleware intercepts a + POST with no CSRF cookie before the view's @require_GET decorator ever + runs, so the real response is 403 ('CSRF verification failed'), not + 405. Both are valid non-GET rejections — accept either rather than + pinning to whichever middleware happens to reject first, since that + ordering isn't this test's concern. + """ resp = api_client.post(_health_url("/health/"), timeout=10) _skip_if_not_deployed(resp) - assert resp.status_code == 405 + assert resp.status_code in (403, 405), resp.text[:300] def test_response_has_no_stack_trace_or_internal_details(self, api_client): """Even a healthy response must not leak framework internals.""" @@ -99,9 +106,11 @@ def test_reports_ok_with_database_check(self, api_client): ) def test_rejects_non_get(self, api_client): + """See TestLiveness.test_rejects_non_get — same CSRF-before-@require_GET + ordering applies here.""" resp = api_client.post(_health_url("/health/ready/"), timeout=10) _skip_if_not_deployed(resp) - assert resp.status_code == 405 + assert resp.status_code in (403, 405), resp.text[:300] def test_response_has_no_stack_trace_or_internal_details(self, api_client): resp = api_client.get(_health_url("/health/ready/"), timeout=10) From 5ac5fa96f5fd1e67decce143585e4f35336a36e0 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 16:49:56 +0530 Subject: [PATCH 12/35] Run api-tests and accessibility-tests concurrently in CI Experiment: both now depend only on lint instead of chaining accessibility-tests after api-tests. The serial chain existed because concurrent load against the shared dev backend used to produce pool exhaustion and widespread ReadTimeouts (docs/app_bugs.md #3/#13/#14). Two things changed since: the backend moved from runserver to Docker, and #13/#14 were independently re-verified fixed today. A local 2-suite concurrency smoke test today was inconclusive - it showed no pool-exhaustion symptoms, but was confounded by a second, unrelated CI run hitting the same backend at the same time (this branch has an open PR into main, so every push auto-triggers a run). Testing it in CI directly avoids that confound. visual-tests and e2e-tests are deliberately not included yet - widen gradually and watch for the ReadTimeout/stuck-curtain signature before going further. --- .github/workflows/ci.yml | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b1882a5..f53941b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,17 +60,25 @@ jobs: # acquisition (scripts/_api_client.get_access_token) and a few # cookie-security tests drive a real headless Chromium via Playwright. # - # api-tests / accessibility-tests / visual-tests / e2e-tests run in a - # serial chain (via `needs:`) rather than all firing at once after lint. - # They all hit the same shared dev backend (dev.parakh.civicdataspace.in), - # which has known slow-query issues (docs/app_bugs.md #13/#14) — running - # them concurrently was overwhelming it and producing widespread transient - # ReadTimeouts / stuck "Loading..." states that looked like test failures - # but were pure infra flakiness (confirmed by re-running the same failures - # in isolation, where they passed cleanly). Lighter/faster suites go first - # so e2e-tests (the heaviest, most concurrency-sensitive suite) gets the - # backend to itself. This trades total pipeline wall-clock time for - # reliability. + # api-tests / accessibility-tests / visual-tests / e2e-tests originally ran + # in a full serial chain (via `needs:`) because they all hit the same + # shared dev backend (dev.parakh.civicdataspace.in), which had known + # slow-query hangs (docs/app_bugs.md #13/#14) that concurrent load turned + # into widespread transient ReadTimeouts / stuck "Loading..." states. + # + # EXPERIMENT started 2026-08-12: api-tests and accessibility-tests now run + # concurrently (both `needs: lint`) instead of serially. Two things changed + # since the serial chain was built — the backend moved from `runserver` to + # a Docker deployment, and #13/#14 were independently re-verified fixed the + # same day (both routes that used to hang now resolve in <30s). A local + # 2-suite concurrency smoke test the same day showed no pool-exhaustion + # symptoms (no "too many clients", no ReadTimeouts) but WAS confounded by a + # second, unrelated CI run hitting the same backend at the same time, so it + # is not treated as conclusive — this in-CI change is the clean test. + # visual-tests / e2e-tests are NOT part of this experiment yet and still + # wait their turn (needs: accessibility-tests / visual-tests) — widen + # gradually, not all at once, and watch a few runs for the ReadTimeout/ + # stuck-curtain signature before going further. api-tests: name: API Tests runs-on: ubuntu-latest @@ -177,7 +185,9 @@ jobs: accessibility-tests: name: Accessibility Tests (axe) runs-on: ubuntu-latest - needs: api-tests + # Runs concurrently with api-tests (both needs: lint) as of 2026-08-12 — + # see the comment above api-tests for why. Was needs: api-tests. + needs: lint if: always() timeout-minutes: 60 steps: From b63d252949a06749a32abba5a8c755811f3425f5 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 17:11:58 +0530 Subject: [PATCH 13/35] Revert api-tests/accessibility-tests to sequential in CI The concurrent experiment (previous commit) started failing in CI within ~13 minutes of the real run. Reverting accessibility-tests to needs: api-tests, restoring the full serial chain. The run was cancelled rather than left to finish, so there's no captured job log of the actual failure mode yet - documented in docs/ci_workflow_notes.md along with what to gather before trying again. --- .github/workflows/ci.yml | 40 +++++++++++++++++++-------------------- docs/ci_workflow_notes.md | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f53941b..48f9538 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,25 +60,25 @@ jobs: # acquisition (scripts/_api_client.get_access_token) and a few # cookie-security tests drive a real headless Chromium via Playwright. # - # api-tests / accessibility-tests / visual-tests / e2e-tests originally ran - # in a full serial chain (via `needs:`) because they all hit the same - # shared dev backend (dev.parakh.civicdataspace.in), which had known - # slow-query hangs (docs/app_bugs.md #13/#14) that concurrent load turned - # into widespread transient ReadTimeouts / stuck "Loading..." states. + # api-tests / accessibility-tests / visual-tests / e2e-tests run in a + # serial chain (via `needs:`) rather than all firing at once after lint. + # They all hit the same shared dev backend (dev.parakh.civicdataspace.in), + # which has known slow-query issues (docs/app_bugs.md #13/#14) — running + # them concurrently was overwhelming it and producing widespread transient + # ReadTimeouts / stuck "Loading..." states that looked like test failures + # but were pure infra flakiness (confirmed by re-running the same failures + # in isolation, where they passed cleanly). Lighter/faster suites go first + # so e2e-tests (the heaviest, most concurrency-sensitive suite) gets the + # backend to itself. This trades total pipeline wall-clock time for + # reliability. # - # EXPERIMENT started 2026-08-12: api-tests and accessibility-tests now run - # concurrently (both `needs: lint`) instead of serially. Two things changed - # since the serial chain was built — the backend moved from `runserver` to - # a Docker deployment, and #13/#14 were independently re-verified fixed the - # same day (both routes that used to hang now resolve in <30s). A local - # 2-suite concurrency smoke test the same day showed no pool-exhaustion - # symptoms (no "too many clients", no ReadTimeouts) but WAS confounded by a - # second, unrelated CI run hitting the same backend at the same time, so it - # is not treated as conclusive — this in-CI change is the clean test. - # visual-tests / e2e-tests are NOT part of this experiment yet and still - # wait their turn (needs: accessibility-tests / visual-tests) — widen - # gradually, not all at once, and watch a few runs for the ReadTimeout/ - # stuck-curtain signature before going further. + # TRIED 2026-08-12: made api-tests + accessibility-tests concurrent (both + # `needs: lint`) on the theory that #13/#14 being independently re-verified + # fixed that same day, plus the backend's runserver -> Docker migration, + # meant the underlying capacity problem was gone. It wasn't (or isn't + # fully) — the concurrent run started failing in CI within ~13 minutes and + # was reverted same-day. Re-attempt only with a real repro of what failed + # (job logs from a completed run, not a cancelled one) in hand first. api-tests: name: API Tests runs-on: ubuntu-latest @@ -185,9 +185,7 @@ jobs: accessibility-tests: name: Accessibility Tests (axe) runs-on: ubuntu-latest - # Runs concurrently with api-tests (both needs: lint) as of 2026-08-12 — - # see the comment above api-tests for why. Was needs: api-tests. - needs: lint + needs: api-tests if: always() timeout-minutes: 60 steps: diff --git a/docs/ci_workflow_notes.md b/docs/ci_workflow_notes.md index 45627b0..99fd1ee 100644 --- a/docs/ci_workflow_notes.md +++ b/docs/ci_workflow_notes.md @@ -75,3 +75,36 @@ code or contort a naming scheme. Note the chain `e2e-tests needs: visual-tests` — resuming `e2e-tests` alone works regardless of `visual-tests`, since the job's own `if:` controls it. + +--- + +## Tried making api-tests + accessibility-tests concurrent — reverted same day (2026-08-12) + +**Status: reverted. `ci.yml` is back to the full serial chain.** + +Theory going in: bug #3's root cause (dev backend overwhelmed under concurrent +load, producing "too many clients"/ReadTimeouts) might no longer apply, since +two things changed the same day — the backend moved from `runserver` to a +Docker deployment, and bugs #13/#14 (routes that used to hang indefinitely) +were independently re-verified fixed (both now resolve in under 30s). + +Changed `accessibility-tests` from `needs: api-tests` to `needs: lint`, so it +would start alongside `api-tests` instead of after it. A local 2-suite +concurrency smoke test beforehand showed no pool-exhaustion symptoms — but +was confounded by an unrelated CI run hitting the same backend at the same +time (this branch has an open PR into `main`, so every push auto-triggers a +run), so it wasn't treated as conclusive on its own. + +The real in-CI test started failing within ~13 minutes. Reverted immediately +on report; the run was cancelled rather than left to finish, so there is +**no captured job log of the actual failure mode** — that's the gap to close +before trying again, not a reason to assume the theory was wrong. Also +observed: because `visual-tests` only depends on `accessibility-tests` (not +`api-tests`), making accessibility fast + independent had a wider blast +radius than intended — `visual-tests` started overlapping with `api-tests` +too, not just accessibility. Any re-attempt should account for that +cascading effect, not just the two jobs whose `needs:` actually changed. + +**Before re-attempting:** get a completed (not cancelled) run's job logs +first, so the failure signature can actually be diagnosed — same-shape +ReadTimeouts as bug #3, or something new post-Docker-migration. From 6eced771ae546f9165896e69dc70a12d9c3cd956 Mon Sep 17 00:00:00 2001 From: Saqib Date: Wed, 12 Aug 2026 17:15:15 +0530 Subject: [PATCH 14/35] Correct the CI concurrency-experiment note with real evidence The prior note said the concurrent run "started failing" without a captured log. Pulled the cancelled API Tests job's partial log (GitHub retains whatever uploaded before cancellation) and found a real, diagnostic failure: tests/api/test_audit_detail_api.py failed in a RERUN/RERUN/FAILED cluster with ~60s between each attempt - a timeout signature matching bug #3's fingerprint, not an assertion bug. Also corrects an overstatement: accessibility-tests and visual-tests both completed successfully in the same run despite running concurrently with the struggling api-tests job, so the slowdown was concentrated in api-tests' fixture-heavy queries, not universal across suites. --- docs/ci_workflow_notes.md | 44 +++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/docs/ci_workflow_notes.md b/docs/ci_workflow_notes.md index 99fd1ee..7a4e677 100644 --- a/docs/ci_workflow_notes.md +++ b/docs/ci_workflow_notes.md @@ -95,16 +95,42 @@ was confounded by an unrelated CI run hitting the same backend at the same time (this branch has an open PR into `main`, so every push auto-triggers a run), so it wasn't treated as conclusive on its own. -The real in-CI test started failing within ~13 minutes. Reverted immediately -on report; the run was cancelled rather than left to finish, so there is -**no captured job log of the actual failure mode** — that's the gap to close -before trying again, not a reason to assume the theory was wrong. Also -observed: because `visual-tests` only depends on `accessibility-tests` (not -`api-tests`), making accessibility fast + independent had a wider blast +The real in-CI test started failing within ~13 minutes. Reverted on report; +`api-tests` was cancelled at 20m29s (vs a 6m31s clean baseline — 3x+), and +`accessibility-tests` + `visual-tests` both actually completed *successfully* +despite running concurrently with it — the slowdown/failures were +concentrated in `api-tests`, not universal. + +**Corrected 2026-08-12, after pulling the cancelled job's partial log** +(`gh api repos/.../actions/jobs//logs` — GitHub retains whatever a job +uploaded before it was killed): there IS a captured failure, and it's +diagnostic. `tests/api/test_audit_detail_api.py` — several tests sharing +fixture-heavy queries — failed together in sequence: + +``` +11:23:01Z RERUN +11:24:04Z RERUN (~63s later) +11:25:05Z FAILED (~61s later) +``` + +~60s between each attempt before it failed/retried is a timeout signature, +not an assertion bug — matches bug #3's fingerprint exactly (dev backend +overwhelmed under concurrent load), reproducing under Docker just as it did +under `runserver`. So: the Docker migration + #13/#14 fixes were NOT +sufficient to make this pairing safe. `accessibility-tests`/`visual-tests` +being fine is consistent with `api-tests`' fixture-heavy queries being the +specific concurrency-sensitive load, not the whole suite. + +Also observed: because `visual-tests` only depends on `accessibility-tests` +(not `api-tests`), making accessibility fast + independent had a wider blast radius than intended — `visual-tests` started overlapping with `api-tests` too, not just accessibility. Any re-attempt should account for that cascading effect, not just the two jobs whose `needs:` actually changed. -**Before re-attempting:** get a completed (not cancelled) run's job logs -first, so the failure signature can actually be diagnosed — same-shape -ReadTimeouts as bug #3, or something new post-Docker-migration. +**Before re-attempting:** the failure signature is now known (timeout-driven +RERUN/RERUN/FAILED clusters on fixture-heavy `tests/api/test_audit_detail_api.py` +queries, ~60s per stalled attempt). A re-attempt should watch specifically for +recurrence of that exact pattern rather than treating a generic "it failed" +as sufficient — and should isolate whether it's `api-tests`' fixture load +specifically, since `accessibility-tests`/`visual-tests` showed no such +symptom running concurrently with it in this same run. From f303d0f4444c1727e656e9507b73c4af196d53e2 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 12:06:58 +0530 Subject: [PATCH 15/35] Fix wrong artifact path for the visual suite summary row Visual Regression has always shown "no report found" regardless of whether the run passed - the summary script was looking for suite-artifacts/visual.json, but that artifact's upload step spans three top-level folders (reports/, screenshots/, snapshots/), so GitHub doesn't strip the reports/ prefix the way it does for api-test-report and accessibility-report (each a single common folder). Confirmed by downloading the actual artifact and inspecting its real layout: reports/visual.json. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 48f9538..f6e93f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -364,7 +364,7 @@ jobs: "API=suite-artifacts/api.json" \ "E2E=shard-artifacts/*/reports/e2e_shard_*.json" \ "Accessibility=suite-artifacts/accessibility.json" \ - "Visual Regression=suite-artifacts/visual.json" + "Visual Regression=suite-artifacts/reports/visual.json" echo "" echo "**Platform:** ${{ env.BASE_URL }}" echo "" From 7da8da8eaa2ea446792c10a7848e4f6f328361f3 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 12:22:39 +0530 Subject: [PATCH 16/35] Add concurrency group so CI runs stop stacking Rapid pushes to an open PR were each auto-triggering a full run, and every run shares one dev backend - overlapping runs were contending with each other (api-tests hit 42m26s vs a 6m31s baseline while two runs overlapped, docs/ci_workflow_notes.md). Groups by PR number (or ref for push/workflow_dispatch), cancel-in-progress so a new push supersedes a stale run's results instead of letting it keep running against the shared backend. --- .github/workflows/ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f6e93f4..a2ac19d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,24 @@ on: branches: [main] workflow_dispatch: +# One run at a time per PR (or per branch/ref for push/workflow_dispatch). +# Added 2026-08-12: rapid pushes to an open PR were each auto-triggering a +# full run, and because this workflow shares one dev backend across every +# run (not just within a run), overlapping runs contended with each other - +# api-tests took 42m26s against a 6m31s baseline while two runs overlapped +# (see docs/ci_workflow_notes.md). cancel-in-progress means a new push +# supersedes the old run's results immediately rather than letting a stale +# commit's run keep consuming the shared backend. +# +# Caveat worth watching: jobs using `if: always()` (accessibility-tests, +# visual-tests, test-summary) are documented to sometimes ignore workflow +# cancellation and start anyway even after the run is marked cancelled - +# they're already paired with explicit timeout-minutes as the backstop for +# that, but verify a real cancelled run to confirm behavior, don't assume. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + permissions: contents: read From f1179237fcabbb1afce6f123f7c97037e77caf74 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 12:35:23 +0530 Subject: [PATCH 17/35] Remove if: always() from test jobs, keep it only on test-summary Verified live: a superseded run's api-tests correctly cancelled via the new concurrency group, but accessibility-tests (if: always()) started right after anyway and kept running against the shared backend, ignoring the cancellation entirely - the exact caveat flagged when the concurrency group was added. Removed always() from accessibility-tests and visual-tests. Every test step already uses continue-on-error, so the upstream job's conclusion is "success" even when its tests fail - default success()-gating only actually changes behavior on a genuine infra failure or cancellation, both cases where skipping is correct. test-summary keeps always() (paired with !cancelled()) since it's a reporting job that must tolerate e2e-tests' deliberate skip while paused, not a test job. Not pushed yet - holding per instruction until current runs are stopped or told to push. --- .github/workflows/ci.yml | 46 +++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2ac19d..d7e053f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,11 +16,15 @@ on: # supersedes the old run's results immediately rather than letting a stale # commit's run keep consuming the shared backend. # -# Caveat worth watching: jobs using `if: always()` (accessibility-tests, -# visual-tests, test-summary) are documented to sometimes ignore workflow -# cancellation and start anyway even after the run is marked cancelled - -# they're already paired with explicit timeout-minutes as the backstop for -# that, but verify a real cancelled run to confirm behavior, don't assume. +# Verified live 2026-08-12 that `if: always()` on a job ignores workflow +# cancellation and lets it start anyway even after the run is marked +# cancelled: api-tests correctly cancelled, but accessibility-tests (then +# `if: always()`) started right after and kept running against the shared +# backend regardless. Removed always() from the actual test jobs +# (accessibility-tests, visual-tests) so they respect cancellation via +# default success()-gating. test-summary still needs always() (it must +# tolerate e2e-tests' deliberate skip while paused) - paired with +# !cancelled() there instead of removing it outright. concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true @@ -146,10 +150,13 @@ jobs: name: E2E Tests (shard ${{ matrix.shard }}/${{ strategy.job-total }}) runs-on: ubuntu-latest needs: visual-tests - # PAUSED 2026-08-11 — remove this line (restoring `if: always()`) to - # resume. test-summary's E2E-report steps are guarded on - # `needs.e2e-tests.result != 'skipped'` and degrade cleanly while this - # is off. + # PAUSED 2026-08-11 — remove this line to resume. test-summary's + # E2E-report steps are guarded on `needs.e2e-tests.result != 'skipped'` + # and degrade cleanly while this is off. Do NOT restore `if: always()` + # (that was the original value) — removed the same pattern from + # accessibility-tests/visual-tests 2026-08-12 after verifying live that + # it ignores workflow cancellation; default success()-gating is correct + # here for the same reason. if: false timeout-minutes: 60 strategy: @@ -204,7 +211,14 @@ jobs: name: Accessibility Tests (axe) runs-on: ubuntu-latest needs: api-tests - if: always() + # No if: always() here on purpose (removed 2026-08-12) - it was letting + # this job start even after the run was cancelled (verified live: a + # superseded run's api-tests correctly cancelled, but this job ignored + # that and kept running against the shared backend for up to its + # timeout). Every test step already uses continue-on-error, so + # api-tests' job conclusion is "success" even when its tests fail - + # default gating only actually skips this job on a genuine infra + # failure or cancellation, both cases where skipping is correct. timeout-minutes: 60 steps: - uses: actions/checkout@v6 @@ -249,7 +263,8 @@ jobs: name: Visual Regression Tests runs-on: ubuntu-latest needs: accessibility-tests - if: always() + # No if: always() here either - see the comment on accessibility-tests' + # if: line above; same reasoning applies. timeout-minutes: 60 steps: - uses: actions/checkout@v6 @@ -320,7 +335,14 @@ jobs: name: Test Summary runs-on: ubuntu-latest needs: [api-tests, e2e-tests, accessibility-tests, visual-tests] - if: always() + # Kept always() here (unlike the test jobs above) because this is the + # reporting job, not a test job - it must still run while e2e-tests is + # deliberately paused (if: false -> always "skipped", and default + # success() gating treats a skipped dependency as not-satisfied, which + # would stop this job from running at all). Paired with !cancelled() so + # it still respects a real concurrency-group cancellation rather than + # repeating the bug just fixed on the test jobs. + if: always() && !cancelled() timeout-minutes: 10 steps: - uses: actions/checkout@v6 From 0e1da81d18e9623b4c0ff7003a2f24d79c264455 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 13:20:30 +0530 Subject: [PATCH 18/35] Fix evaluator-type radio locator drift (Technical -> TECHNICAL_AUDIT) DOM value attrs on the step-2 evaluator-type radios changed from plain Technical/Domain/Cultural to the backend enum form (TECHNICAL_AUDIT/ DOMAIN_AUDIT/CULTURAL_AUDIT), so select_evaluator_type_in_modal timed out 30s on every call. Updated the locators and made get_checked_evaluator_type() map the raw enum back to the display label so existing assertions keep reading Technical/Domain/Cultural. --- locators/evaluations_locators.py | 15 ++++++++++----- pages/new_evaluation_page.py | 14 ++++++++++++-- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/locators/evaluations_locators.py b/locators/evaluations_locators.py index 674c2da..c1301b9 100644 --- a/locators/evaluations_locators.py +++ b/locators/evaluations_locators.py @@ -96,12 +96,17 @@ class EvaluationsLocators: ) MODAL_LOADING_MODELS = "[role='dialog'] :text('Loading models')" - # Step-2 evaluator-type radios (values: Technical / Domain / Cultural; - # Technical is checked by default). + # Step-2 evaluator-type radios. DOM `value`s are the backend enum + # (TECHNICAL_AUDIT / DOMAIN_AUDIT / CULTURAL_AUDIT), not the display + # label — confirmed via live DOM dump 2026-08-13 (previously plain + # 'Technical' / 'Domain' / 'Cultural', now drifted to the _AUDIT suffix + # form). TECHNICAL_AUDIT is checked by default. See + # NewEvaluationPage.get_checked_evaluator_type() for the reverse mapping + # back to the human-readable label tests assert against. MODAL_EVALUATOR_TYPE_RADIO = "input[name='evaluatorType']" - MODAL_EVALUATOR_TECHNICAL = "input[name='evaluatorType'][value='Technical']" - MODAL_EVALUATOR_DOMAIN = "input[name='evaluatorType'][value='Domain']" - MODAL_EVALUATOR_CULTURAL = "input[name='evaluatorType'][value='Cultural']" + MODAL_EVALUATOR_TECHNICAL = "input[name='evaluatorType'][value='TECHNICAL_AUDIT']" + MODAL_EVALUATOR_DOMAIN = "input[name='evaluatorType'][value='DOMAIN_AUDIT']" + MODAL_EVALUATOR_CULTURAL = "input[name='evaluatorType'][value='CULTURAL_AUDIT']" # Step-2 objective textarea — required; Start Evaluation stays disabled # while it is empty. MODAL_OBJECTIVE_TEXTAREA = "[role='dialog'] textarea" diff --git a/pages/new_evaluation_page.py b/pages/new_evaluation_page.py index 7fd4a4f..cd7e4a6 100644 --- a/pages/new_evaluation_page.py +++ b/pages/new_evaluation_page.py @@ -269,12 +269,22 @@ def select_evaluator_type_in_modal(self, eval_type: str = "technical") -> None: sel = value_map.get(eval_type.lower(), value_map["technical"]) self.page.locator(sel).first.check() + # DOM `value`s are the backend enum (TECHNICAL_AUDIT/DOMAIN_AUDIT/ + # CULTURAL_AUDIT); map back to the human-readable label the UI shows + # and tests assert against. See locators/evaluations_locators.py. + _EVALUATOR_TYPE_LABELS = { + "TECHNICAL_AUDIT": "Technical", + "DOMAIN_AUDIT": "Domain", + "CULTURAL_AUDIT": "Cultural", + } + def get_checked_evaluator_type(self) -> str | None: - """Return the value (Technical/Domain/Cultural) of the checked step-2 radio.""" + """Return the label (Technical/Domain/Cultural) of the checked step-2 radio.""" radios = self.page.locator(EvaluationsLocators.MODAL_EVALUATOR_TYPE_RADIO).all() for r in radios: if r.is_checked(): - return r.get_attribute("value") + raw = r.get_attribute("value") + return self._EVALUATOR_TYPE_LABELS.get(raw, raw) return None def fill_modal_objective(self, objective: str) -> None: From 0c4810ffb5cd10f77bf437cc55a5eac74ecd0b79 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 13:20:34 +0530 Subject: [PATCH 19/35] File bug #26: Evaluator overview field always shows 'Evaluator' Root-caused in ParakhAI-frontend (origin/dev, NewEvaluationContent.tsx getEvaluatorLabel): the switch compares the raw AUDIT_TYPE enum against AUDIT_TYPE_LABELS values, which can never match, so every selection falls through to the default label. Introduced in commit 34f221c. xfail the three e2e assertions that hit it. --- docs/app_bugs.md | 2 ++ tests/e2e/test_add_evaluation_bulk.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index a9f781a..cf59f37 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -34,6 +34,8 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 23 | auth / backend | ParakhAPI's `DataSpaceAuthMiddleware` (`graphql_api/middleware/dataspace_user.py::process_request`) wraps its entire Bearer-token validation path (DataSpace `/api/auth/user/info/` lookup) in a blanket `except Exception`, and on **any** failure — expired token, invalid token, DataSpace being briefly unreachable — silently sets `request.user = AnonymousUser()` and continues. The failure is only `logger.warning`'d server-side; the client gets a normal `200` response shaped exactly like a legitimate anonymous request (e.g. `audits` returns `{data: [], totalItemsCount: 0}`, no `errors` array, nothing containing "token"/"expired"/401/403). A client has no way to distinguish "you are genuinely unauthenticated" from "your session just died" — it can only infer this indirectly from previously-authenticated-looking context. Root-caused 2026-08-11 while investigating 3 seemingly-flaky `ParakhAI_test` security tests that passed in isolation but failed only when reached ~4-5 minutes into a long suite run; confirmed by reading the middleware source directly. This is very likely the true root cause behind this file's own previously-vague "session expired mid-suite" attributions in the Phase 10 log entries (bug #3/#12 family) — those blamed Keycloak/dev-env instability in general, but the precise mechanism is this middleware's silent-swallow, which a well-behaved API should instead surface as a 401. | open | With a valid Bearer token, wait past the token's TTL (~5 min on dev), then issue any authenticated GraphQL query (e.g. `{ audits { data { id } totalItemsCount } }`) with the now-stale token. Response is `200` with the same shape as an anonymous request, no error. Confirmed against `ParakhAPI` `graphql_api/middleware/dataspace_user.py:99-104` (dev branch) 2026-08-11. | Indirectly, any test using `tests/conftest.py::authenticated_graphql_client` on a long-running suite — see the fixture's own docstring (fixed 2026-08-11 to proactively refresh before this can bite) for the test-side mitigation; this row documents the underlying product-side gap that mitigation works around. | 2026-08-11 | | 22 | audits query / backend perf | Sorting the `audits` GraphQL query by `passed_tests` is measurably slower than the default `created_at` ordering under concurrent load. 5 concurrent authenticated `audits(sortOptions: [{field: "passed_tests", direction: "desc"}])` requests (light field selection: `id status passedTests` only, `limit: 20`) took 9-15s each on the cleanest run and up to 35s under noisier conditions, vs. a 6s budget for the equivalent unsorted 5-concurrent read (`TestConcurrentGraphQLReads`). 100% success, zero errors — not a correctness bug, but the delta suggests the `(organization, -created_at)` index added for this query (commit `4f51ab2`) doesn't help an `ORDER BY passed_tests`, which has no index of its own. Worth a backend follow-up (composite/partial index, or precomputing `passed_tests` ordering) if sort-by-pass-rate becomes a real UI feature rather than just an API capability. | open | Run `pytest tests/load/test_load_graphql.py -m load -v -k TestAuditsSortPerformance` — compare reported latencies in `reports/load_metrics_graphql.json` (`concurrent_audits_sorted_n5`) against the unsorted baseline in the same file. | `tests/load/test_load_graphql.py::TestAuditsSortPerformance::test_5_concurrent_audits_sorted_by_passed_tests` | 2026-08-11 | +| 26 | wizard / evaluator-type label | The wizard's Evaluation Overview card renders the "Evaluator" field as the literal string `Evaluator` instead of the selected evaluator type ("Technical"/"Domain"/"Cultural"), for all three evaluator-type selections. Root-caused in frontend source (`ParakhAI-frontend`, `dev` branch @ `a1c5b84` — note the local checkout used for earlier bug rows was several commits behind live `origin/dev` at investigation time; always diff against `origin/dev`, not a stale local clone, when re-verifying this row): `getEvaluatorLabel()` in `app/[locale]/dashboard/ai-maker/[orgId]/evaluations/components/NewEvaluationContent.tsx:1958-1969` switches on `auditType` (a raw backend enum, e.g. `AUDIT_TYPE.TECHNICAL_AUDIT` = `'TECHNICAL_AUDIT'`) but its `case` labels compare against `AUDIT_TYPE_LABELS.TECHNICAL_AUDIT` (the *display* label, `'Technical'`) — an enum-vs-label comparison that can never match, so every selection falls through to `default: return 'Evaluator'`. Introduced in commit `34f221c` (2026-08-03, "refactor: update audit type handling in evaluation components"), which correctly migrated the `auditType` state to hold the raw enum but missed updating this one downstream consumer to compare against `AUDIT_TYPE.*` instead of `AUDIT_TYPE_LABELS.*`. Same-shaped drift also broke this repo's Playwright locator for the step-2 radio inputs (`input[name='evaluatorType'][value='Technical']` → DOM now emits `[value='TECHNICAL_AUDIT']`) — fixed test-side in this session (`locators/evaluations_locators.py`, `pages/new_evaluation_page.py::get_checked_evaluator_type`), since that half was a pure test-locator drift, not a product defect. | open | Log in as `TEST_EMAIL_1` → AI Maker → CivicDataLab → New Evaluation → select any model → Bulk method → Next → leave evaluator type on the default "a technical evaluator" (or pick Domain/Cultural) → fill Objective → Start Evaluation. On the wizard's Evaluation Overview card, the "Evaluator" row reads `Evaluator : Evaluator` regardless of which type was selected (Scope/Mode/Objective all render correctly in the same card). Confirmed via a live scripted repro 2026-08-13 (draft audit id 1911, since cancelled via `scripts/cleanup_drafts.py`) — full page-body dump showed `Evaluator : Evaluator`. | `tests/e2e/test_add_evaluation_bulk.py::TestBulkDraftCreation::test_overview_reflects_modal_inputs`; `tests/e2e/test_add_evaluation_bulk.py::TestEvaluatorTypeVariants::test_evaluator_type_reflected_in_overview[domain-Domain]`, `[cultural-Cultural]` | 2026-08-13 | + ## Conventions - One row per distinct bug. If two tests fail because of the same backend behaviour, list both in `related test(s)`. diff --git a/tests/e2e/test_add_evaluation_bulk.py b/tests/e2e/test_add_evaluation_bulk.py index 9a78384..efd1a2f 100644 --- a/tests/e2e/test_add_evaluation_bulk.py +++ b/tests/e2e/test_add_evaluation_bulk.py @@ -97,6 +97,8 @@ def test_overview_reflects_modal_inputs( f"Overview Mode must read 'Bulk Evaluation'; got {nep.get_overview_field('Mode')!r}" ) evaluator = nep.get_overview_field("Evaluator") or "" + if "Technical" not in evaluator: + pytest.xfail("App bug #26 — see docs/app_bugs.md") assert "Technical" in evaluator, ( f"Overview Evaluator must reflect the Technical selection; got {evaluator!r}" ) @@ -274,6 +276,8 @@ def test_evaluator_type_reflected_in_overview( eval_type=eval_type, ) evaluator = nep.get_overview_field("Evaluator") or "" + if expected not in evaluator: + pytest.xfail("App bug #26 — see docs/app_bugs.md") assert expected in evaluator, ( f"Overview Evaluator must reflect the {eval_type} selection; got {evaluator!r}" ) From 4383906d57c08cfbdf0676ef1e3abe80effefa3e Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 14:02:39 +0530 Subject: [PATCH 20/35] Fix playground evaluation-method locator drift (manual -> playground) DOM value attr for the Playground radio changed from 'manual' to 'playground' (bulk unchanged). select_evaluation_method/is_method_selected kept the public 'bulk'/'manual' API used across ~20 call sites and translate internally to the real DOM values. --- locators/evaluations_locators.py | 4 +++- pages/new_evaluation_page.py | 13 +++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/locators/evaluations_locators.py b/locators/evaluations_locators.py index c1301b9..d3171c6 100644 --- a/locators/evaluations_locators.py +++ b/locators/evaluations_locators.py @@ -85,8 +85,10 @@ class EvaluationsLocators: MODAL_MODEL_DROPDOWN = "select[name='modelSelect']" MODAL_VERSION_DROPDOWN = "select[name='versionSelect']" MODAL_EVAL_NAME_INPUT = "input[name='evaluationName']" + # Playground radio's DOM value drifted to 'playground' (was 'manual'); + # confirmed via origin/dev source 2026-08-13. Bulk unchanged. MODAL_EVAL_METHOD_BULK = "input[name='evaluationMethod'][value='bulk']" - MODAL_EVAL_METHOD_PLAYGROUND = "input[name='evaluationMethod'][value='manual']" + MODAL_EVAL_METHOD_PLAYGROUND = "input[name='evaluationMethod'][value='playground']" MODAL_NEXT_BUTTON = "[role='dialog'] button:has-text('Next')" MODAL_BACK_BUTTON = "[role='dialog'] button:has-text('Back')" MODAL_START_BUTTON = "[role='dialog'] button:has-text('Start Evaluation')" diff --git a/pages/new_evaluation_page.py b/pages/new_evaluation_page.py index cd7e4a6..462fd83 100644 --- a/pages/new_evaluation_page.py +++ b/pages/new_evaluation_page.py @@ -226,16 +226,25 @@ def set_modal_eval_name(self, name: str) -> None: loc.wait_for(state="visible", timeout=self.timeout) loc.fill(name) + # Callers use 'bulk'/'manual' as the public method name; the DOM `value` + # attribute for the Playground radio drifted to 'playground' (confirmed + # via origin/dev source 2026-08-13 — 'bulk' is unchanged). Translate here + # so the public 'bulk'/'manual' API doesn't need touching at ~20 call + # sites across the suite. + _EVAL_METHOD_DOM_VALUES = {"bulk": "bulk", "manual": "playground"} + def select_evaluation_method(self, method: str = "bulk") -> None: """Select the evaluation-method radio in step 1: 'bulk' or 'manual' (Playground).""" + dom_value = self._EVAL_METHOD_DOM_VALUES.get(method, method) self.page.locator( - f"input[name='evaluationMethod'][value='{method}']" + f"input[name='evaluationMethod'][value='{dom_value}']" ).click() def is_method_selected(self, method: str) -> bool: """Return True if the given evaluation-method radio is checked.""" + dom_value = self._EVAL_METHOD_DOM_VALUES.get(method, method) return self.page.locator( - f"input[name='evaluationMethod'][value='{method}']" + f"input[name='evaluationMethod'][value='{dom_value}']" ).is_checked() def click_modal_next(self) -> NewEvaluationPage: From fa08d7a71623247bdc0969126af0a38e708185b4 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 14:08:50 +0530 Subject: [PATCH 21/35] Skip CDS-001 tests when anonymous fixture never reaches the CDS editor Both tests navigate with the unauthenticated page fixture, which redirects to Keycloak login before the editor's JS bundle (and its SyntaxError) ever loads -- so they were passing without observing anything, which fails loudly under xfail(strict=True) as a false XPASS. Added the same reachability skip guard the CDS-005 tests in this file already use, keyed on landing on the opub-kc auth domain. --- tests/e2e/test_add_model_flow.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/e2e/test_add_model_flow.py b/tests/e2e/test_add_model_flow.py index 252b57d..bbc77f3 100644 --- a/tests/e2e/test_add_model_flow.py +++ b/tests/e2e/test_add_model_flow.py @@ -117,6 +117,11 @@ def test_cds001_no_js_syntax_error_on_editor_page_load(self, page: Page): page.on("pageerror", lambda e: errors.append(str(e))) page.goto(Config.cds_url("/en/manage/ai-models"), wait_until="domcontentloaded", timeout=20000) page.wait_for_timeout(3000) + if "opub-kc" in page.url or "auth/realms" in page.url: + pytest.skip( + "CDS editor not reached — page (uses anonymous `page` fixture, no CDS " + f"auth) redirected to Keycloak login before the editor loaded: {page.url}" + ) syntax_errors = [e for e in errors if "SyntaxError" in e or "appendChild" in e] assert not syntax_errors, ( "CDS-001: JS SyntaxError(s) on editor page load:\n" + "\n".join(syntax_errors) @@ -139,6 +144,11 @@ def test_cds001_editor_has_no_console_errors_on_load(self, page: Page): page.on("pageerror", lambda e: console_errors.append(str(e))) page.goto(Config.cds_url("/en/manage/ai-models"), wait_until="domcontentloaded", timeout=20000) page.wait_for_timeout(3000) + if "opub-kc" in page.url or "auth/realms" in page.url: + pytest.skip( + "CDS editor not reached — page (uses anonymous `page` fixture, no CDS " + f"auth) redirected to Keycloak login before the editor loaded: {page.url}" + ) assert not console_errors, f"Console errors on CDS editor load: {console_errors}" @pytest.mark.xfail(reason="CDS-004: ?tab=registered param silently ignored on CivicDataSpace AI Models — known bug") From f9cf356da4c79aa874b6b62248016883609925f9 Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 13 Aug 2026 14:26:16 +0530 Subject: [PATCH 22/35] Migrate status-filter tests off the removed StatusFilterTabs bar The tab bar was replaced by a per-column 'Filter Status' popover on the DataTable in the evaluation-table-listing redesign (~2026-08); all STATUS_TAB_* locators now permanently match 0 elements. Added the new FILTER_STATUS_BUTTON/FILTER_DIALOG locators and rewrote EvaluationsPage.click_status_tab/get_status_filter_options/ is_status_filter_available to drive the popover. Updated all of TestStatusFilterTabs plus the two pagination tests that referenced the old tabs. Also filed bug #27: the Pending Review filter option shows its chip as applied but doesn't actually filter rows (Draft/Completed filtering works correctly), and xfailed the one test that catches it. --- docs/app_bugs.md | 2 + locators/evaluations_locators.py | 22 ++++++-- pages/evaluations_page.py | 58 +++++++++++++------- tests/e2e/test_evaluations.py | 91 ++++++++++++++++---------------- 4 files changed, 105 insertions(+), 68 deletions(-) diff --git a/docs/app_bugs.md b/docs/app_bugs.md index cf59f37..6bb5566 100644 --- a/docs/app_bugs.md +++ b/docs/app_bugs.md @@ -36,6 +36,8 @@ Format: append-only. When a bug is fixed in the app, mark `status: fixed` and th | 26 | wizard / evaluator-type label | The wizard's Evaluation Overview card renders the "Evaluator" field as the literal string `Evaluator` instead of the selected evaluator type ("Technical"/"Domain"/"Cultural"), for all three evaluator-type selections. Root-caused in frontend source (`ParakhAI-frontend`, `dev` branch @ `a1c5b84` — note the local checkout used for earlier bug rows was several commits behind live `origin/dev` at investigation time; always diff against `origin/dev`, not a stale local clone, when re-verifying this row): `getEvaluatorLabel()` in `app/[locale]/dashboard/ai-maker/[orgId]/evaluations/components/NewEvaluationContent.tsx:1958-1969` switches on `auditType` (a raw backend enum, e.g. `AUDIT_TYPE.TECHNICAL_AUDIT` = `'TECHNICAL_AUDIT'`) but its `case` labels compare against `AUDIT_TYPE_LABELS.TECHNICAL_AUDIT` (the *display* label, `'Technical'`) — an enum-vs-label comparison that can never match, so every selection falls through to `default: return 'Evaluator'`. Introduced in commit `34f221c` (2026-08-03, "refactor: update audit type handling in evaluation components"), which correctly migrated the `auditType` state to hold the raw enum but missed updating this one downstream consumer to compare against `AUDIT_TYPE.*` instead of `AUDIT_TYPE_LABELS.*`. Same-shaped drift also broke this repo's Playwright locator for the step-2 radio inputs (`input[name='evaluatorType'][value='Technical']` → DOM now emits `[value='TECHNICAL_AUDIT']`) — fixed test-side in this session (`locators/evaluations_locators.py`, `pages/new_evaluation_page.py::get_checked_evaluator_type`), since that half was a pure test-locator drift, not a product defect. | open | Log in as `TEST_EMAIL_1` → AI Maker → CivicDataLab → New Evaluation → select any model → Bulk method → Next → leave evaluator type on the default "a technical evaluator" (or pick Domain/Cultural) → fill Objective → Start Evaluation. On the wizard's Evaluation Overview card, the "Evaluator" row reads `Evaluator : Evaluator` regardless of which type was selected (Scope/Mode/Objective all render correctly in the same card). Confirmed via a live scripted repro 2026-08-13 (draft audit id 1911, since cancelled via `scripts/cleanup_drafts.py`) — full page-body dump showed `Evaluator : Evaluator`. | `tests/e2e/test_add_evaluation_bulk.py::TestBulkDraftCreation::test_overview_reflects_modal_inputs`; `tests/e2e/test_add_evaluation_bulk.py::TestEvaluatorTypeVariants::test_evaluator_type_reflected_in_overview[domain-Domain]`, `[cultural-Cultural]` | 2026-08-13 | +| 27 | evaluations list / filtering | The evaluations list's "Filter Status" column-filter popover (part of the ~2026-08 evaluation-table-listing redesign, which replaced the old StatusFilterTabs bar) applies correctly for "Completed" and "Draft" — the table only shows matching rows — but selecting "Pending Review" and clicking Apply shows the `Status: Pending Review` filter chip as active while the table rows remain **completely unfiltered** (DRAFT/CANCELLED/etc. rows still visible, same row count and same "Page 1 of 4" as the unfiltered view). Confirmed live 2026-08-13: applying the filter via the popover, the chip renders correctly but `td :has-text('DRAFT'), td :has-text('COMPLETED'), td :has-text('IN_PROGRESS')` still matches 12 rows immediately after. Not yet isolated to frontend vs. backend (`PENDING_REVIEW` is a real, valid `AuditStatus` enum value confirmed in `ParakhAPI/apps/model_audits/models/audit.py`, and the frontend's `EVALUATION_STATUS_FILTER_OPTIONS`/`AUDIT_FILTER_FIELD_MAP` plumbing for `status` looks structurally identical to the working Draft/Completed path) — worth a follow-up with a network-level capture of the `audits` GraphQL request's `filters` payload for this specific value to see whether the wrong value is sent or the backend silently drops/ignores it. | open | Log in as `TEST_EMAIL_1` → AI Maker → CivicDataLab → Evaluations. Click the "Filter Status" icon button on the Status column header, check "Pending Review", click Apply. The `Status: Pending Review` chip appears above the table, but rows with DRAFT/CANCELLED/etc. status remain visible and the row/page count is unchanged from the unfiltered view. Repeating the same steps with "Completed" or "Draft" correctly filters to only matching rows. | `tests/e2e/test_evaluations.py::TestStatusFilterTabs::test_pending_review_tab_filters_correctly` | 2026-08-13 | + ## Conventions - One row per distinct bug. If two tests fail because of the same backend behaviour, list both in `related test(s)`. diff --git a/locators/evaluations_locators.py b/locators/evaluations_locators.py index d3171c6..132d8f6 100644 --- a/locators/evaluations_locators.py +++ b/locators/evaluations_locators.py @@ -18,13 +18,14 @@ class EvaluationsLocators: EVAL_COMPLETED_COL = "th:text('Completed'), :text('Completed')" # ── Status filter tabs (StatusFilterTabs component — Jun 2026) ─────────── - # As of late Jun 2026 the component renders 9 tabs: - # All | Draft | Queued | Running | In Progress | Pending Review | Completed | Failed | Cancelled - # Use :has-text for substring match so count badges ("Draft(40)") still match. + # REMOVED in the evaluation-table-listing redesign (~2026-08): the tab bar + # is gone. Status filtering now lives in a per-column filter popover on + # the DataTable (see FILTER_STATUS_BUTTON below) — confirmed live + # 2026-08-13 (button/[role='tab'] counts are 0 for all of these on both a + # fresh nav and a post-navigation SPA transition). Kept as dead constants + # only so any external reference doesn't hard-crash; do not use in new code. STATUS_TAB_ALL = "button:has-text('All'), [role='tab']:has-text('All')" STATUS_TAB_DRAFT = "button:has-text('Draft'), [role='tab']:has-text('Draft')" - # "Pending" is now split into "Queued" and "Pending Review" — keep old selector - # as a broad fallback and add the specific new ones. STATUS_TAB_PENDING = "button:has-text('Pending'), [role='tab']:has-text('Pending')" STATUS_TAB_QUEUED = "button:has-text('Queued'), [role='tab']:has-text('Queued')" STATUS_TAB_IN_PROGRESS = "button:has-text('In Progress'), [role='tab']:has-text('In Progress')" @@ -34,6 +35,17 @@ class EvaluationsLocators: STATUS_TAB_FAILED = "button:has-text('Failed'), [role='tab']:has-text('Failed')" STATUS_TAB_CANCELLED = "button:has-text('Cancelled'), [role='tab']:has-text('Cancelled')" + # ── Status column filter popover (replaces StatusFilterTabs, ~2026-08) ─── + # DataTable columns each get a filter icon button (aria-label="Filter + # "); clicking it opens a [role='dialog'] with a checkbox per + # option (real