Skip to content

fix: make two CI tests hermetic to the wall-clock window - #673

Merged
johanzander merged 1 commit into
mainfrom
fix/ci-flake-time-window
Aug 22, 2026
Merged

fix: make two CI tests hermetic to the wall-clock window#673
johanzander merged 1 commit into
mainfrom
fix/ci-flake-time-window

Conversation

@johanzander

Copy link
Copy Markdown
Owner

Summary

  • Fix a Fast-tests flake: test_app_startup.py loads the real app.py, whose
    BESSController.__init__ calls set_timezone("UTC") from the mocked HA
    config — leaking the shared module-global time_utils.TIMEZONE into the
    rest of the test session. At UTC hour 0 the historical-data endpoint then
    computes zero "missing" hours, so test_dismiss_persists_across_requests
    always gets dismissed: False. Restore TIMEZONE on teardown.
  • Fix an E2E flake: the vpp_power schedule spec mocks a 00:00–01:00
    period group, which the page treats as "current" whenever the real wall
    clock falls in that window — the Current Strategy card then renders a
    second "+35% (Remote)" and the strict getByText('+35%') resolves to 2
    elements. Scope the assertion to the schedule table's VPP cell.

Root cause

Both are the same class: tests not hermetic to the wall clock, plus CI runs
landing in the 00:00–01:00 UTC window. Neither is caused by #671 or #672
both PRs hit the identical failures on their own CI runs.

Verification

  • pytest backend/tests/test_app_startup.py backend/tests/test_dashboard_api.py::TestHistoricalDataStatus
    failed before (1 failed) and passes after (8 passed).
  • Full pytest -m "not slow": 2225 passed / 0 failed.
  • E2E fix verified by locator analysis (single cell match); full E2E suite
    requires the mock-HA/podman stack and runs on CI.

🤖 Generated with Claude Code

test_dismiss_persists_across_requests fails when the suite runs in the
00:00 UTC hour: test_app_startup's real app.py load calls
set_timezone("UTC") from the mocked HA config, leaking the shared
module-global TIMEZONE; at UTC hour 0 the historical-data endpoint then
sees zero "missing" hours so the dismissed flag is always False. Restore
TIMEZONE on teardown so the leak stops at the file boundary.

The inverter-schedule-control-model VPP spec hits the same time-window
class: the mocked 00:00-01:00 period group becomes "current" on the real
clock inside that window, so the Current Strategy card renders a second
"+35%" and the strict getByText() resolves to 2 elements. Scope the
assertion to the schedule table's VPP cell instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@bess-agent

Copy link
Copy Markdown
Collaborator

@claude-bot review

@bess-manager-claude-bot bess-manager-claude-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: PR #673 — fix: make two CI tests hermetic to the wall-clock window

Verdict: APPROVE

No linked issue — this is a self-contained CI-flake fix (PR body references #671/#672 only to show the flake isn't caused by them).

Fix 1 — backend/tests/test_app_startup.py (+8/-0)

Root cause matches the diagnosis exactly. Verified in core/bess/time_utils.py:

def set_timezone(tz_name: str) -> None:
    global TIMEZONE
    TIMEZONE = ZoneInfo(tz_name)
...
TIMEZONE = ZoneInfo("Europe/Stockholm")

TIMEZONE is a genuine module-global mutated in place by set_timezone. _load_real_app_module execs the real app.py, whose BESSController.__init__ calls set_timezone("UTC") from the mocked HA config (get_ha_config{"time_zone": "UTC"}), permanently flipping the shared global for the rest of the session — exactly the leak the PR describes.

The fix:

import core.bess.time_utils as time_utils
monkeypatch.setattr(time_utils, "TIMEZONE", time_utils.TIMEZONE, raising=False)

This is a correct, idiomatic use of monkeypatch: it snapshots the pre-exec value of TIMEZONE before exec_module runs set_timezone("UTC"), and pytest restores that snapshot at the function-scoped monkeypatch fixture's teardown — regardless of how many times the global gets reassigned during the test body. Since each test function in this file takes its own monkeypatch, the restore happens at the end of every test that calls _load_real_app_module, which stops the leak at the file boundary as claimed. No workaround shape here (no new flag/param/second construction site) — this fixes the actual leak at its source using the tool built for exactly this.

Minor nit: raising=False is redundant since TIMEZONE is unconditionally defined at module scope — harmless, not worth a changed request.

Fix 2 — e2e/tests/inverter-schedule-control-model.spec.ts (+5/-2)

Confirmed against frontend/src/components/InverterStatusDashboard.tsx: the "Current Strategy" card (line 637, a StatusCard) renders +35% (Remote) as a keyValue/metric string — not a table cell — whenever dashboardControlModel === 'vpp_power' and the wall clock falls inside the mocked 00:00–01:00 "current" period group. The schedule table's VPP column cell (line 916) independently renders +35% inside a <td>. The old page.getByText('+35%') (strict mode) would match both nodes whenever a CI run landed in that UTC hour — exactly the flake described. Scoping to page.getByRole('cell', { name: '+35%' }) correctly excludes the card (non-table markup) and, since the mocked periodGroups array has exactly one entry, uniquely matches the one table cell. This is a real fix, not a loosened assertion — it still asserts the signed-percent rendering, just via a selector immune to the day's actual wall-clock hour.

Checklist

  • Matches root cause: yes, for both fixes — verified against the actual time_utils.py global and the actual card/table markup, not just taken on faith.
  • Would the "test" catch a regression: yes — the existing tests these fixes touch (test_dismiss_persists_across_requests and the vpp_power E2E spec) already assert real behavior (dismissed-flag correctness under real "missing hours" math; correct VPP-column rendering). Nothing was weakened to make the flake go away.
  • Minimal / no scope creep: yes — only the two files needed are touched, no production code changed, no unrelated cleanup.
  • Rule violations: none found. No Optional[x], no hasattr/getattr fallbacks, no new classes, no ad-hoc API dicts, no string-matched exceptions, no hardcoded entity IDs — none of these apply since this is test-only. Comments added explain non-obvious WHY (the leak mechanism / card-vs-cell ambiguity), consistent with the comments rule.
  • CI on this PR is fully green (Fast tests, E2E tests, Code quality, Docker build & boot), which is itself evidence the E2E fix works against the real rendered page, not just locator analysis.

One process nit (not a blocker): per rules.md Debugging Protocol step 9, the PR description doesn't explicitly label this as a "local fix" — but the diff itself makes that unambiguous (test-only, no new method responsibilities, no routing-around-a-problem shape), so I'm not treating the omission as a rule violation here.

@johanzander
johanzander marked this pull request as ready for review August 22, 2026 08:39
@johanzander
johanzander merged commit 13b1f8e into main Aug 22, 2026
8 checks passed
@johanzander
johanzander deleted the fix/ci-flake-time-window branch August 22, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants