Fix flaky Hash Algorithm select wait condition#455
Conversation
The Integrity Check select test waited with not.toHaveValue("") for the
config to arrive via SSE, but Angular renders selects bound to a null
model with a non-empty "? null:null ?" placeholder option. That caused
the assertion to pass while the model was still null, after which
toBeEnabled would deterministically fail because value() === null still
disables the select.
Wait on a real algorithm value (md5|sha1|sha256) instead. The
toBeEnabled check is left in place as an explicit confirmation of the
gate the comment describes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA test assertion in the "Hash Algorithm" select test is strengthened to verify the configured algorithm matches an expected value ( ChangesTest Assertion Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/e2e-playwright/tests/settings.spec.ts (1)
515-518: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding an explicit timeout to guard against the same SSE-loading race.
After
settings.goto()(line 516) the Angular app must receive the config via SSE before the Log Level select reflects the persisted value. The assertion at line 518 relies on Playwright's default 5 s timeout with nowaitForStreamcall, which is the same pattern that caused the Hash Algorithm flakiness described in this PR. Log Level has no disable condition so the failure mode is less severe, but the race is structurally identical.🔧 Suggested fix for consistency
await settings.goto(); + await waitForStream(page); const reloadedSelect = settings.getSelect("Log Level"); - await expect(reloadedSelect).toHaveValue(newValue); + await expect(reloadedSelect).toHaveValue(newValue, { timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/settings.spec.ts` around lines 515 - 518, The test can race with SSE loading after calling settings.goto(), so before asserting on the value returned by settings.getSelect("Log Level") you should wait explicitly for the config to arrive; update the test to await a stable condition (for example call the existing waitForStream helper or use Playwright's waitForSelector/state with an extended timeout) right after settings.goto() and before reading reloadedSelect, then keep the expect(reloadedSelect).toHaveValue(newValue) as-is; reference the settings.goto(), settings.getSelect("Log Level"), reloadedSelect and the project's waitForStream helper (or equivalent waitForSelector) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 515-518: The test can race with SSE loading after calling
settings.goto(), so before asserting on the value returned by
settings.getSelect("Log Level") you should wait explicitly for the config to
arrive; update the test to await a stable condition (for example call the
existing waitForStream helper or use Playwright's waitForSelector/state with an
extended timeout) right after settings.goto() and before reading reloadedSelect,
then keep the expect(reloadedSelect).toHaveValue(newValue) as-is; reference the
settings.goto(), settings.getSelect("Log Level"), reloadedSelect and the
project's waitForStream helper (or equivalent waitForSelector) when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3ce2007-02d9-4160-b929-83353c3e19ba
📒 Files selected for processing (1)
src/e2e-playwright/tests/settings.spec.ts
After reloading the settings page, the test asserted on the Log Level select's value with only the default 5 s expect timeout. Recent CI runs showed this test finishing at ~4.2 s, leaving little headroom — and the margin shrinks once Playwright workers go up. settings.goto() only waits for Angular's sidebar render (before SSE config delivery), so toBeEnabled is the right gate: the disable condition is `value() === null || value() === undefined`, so an enabled select implies the model has received the SSE value. This mirrors the pattern used in the Hash Algorithm test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The original setup set validate.xfer_verify = True under the assumption that this enabled the algorithm select. It does not. The disable rule lives in buildValidateContext() in settings-page.component.ts and is gated on config.validate.enabled (post-download validation), not on xfer_verify (inline transfer verification). So the select stayed disabled regardless of the SSE-delivered model value, and toBeEnabled deterministically failed. Switch the setup to toggle validate.enabled, and reorder the assertions so toBeEnabled (the slower wait, since it depends on both SSE delivery AND the dynamic context rebuild) gates the toHaveValue check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
not.toHaveValue(\"\")withtoHaveValue(/^(md5|sha1|sha256)\$/)in the Integrity Check select test (src/e2e-playwright/tests/settings.spec.ts:392)Why
This test was added in #442 and has been failing 100% of the time on every PR run since #442 merged. Develop's CI has been showing green only because the merge run for #442 itself skipped `Build and Test (amd64)` — the failure was first surfaced by #454 (an unrelated docs PR).
The Hash Algorithm select has a single disable gate: `value() === null || value() === undefined` (option.component.html). The test correctly identified this and waited for the SSE config update with `not.toHaveValue("")` — but Angular renders selects bound to a null model with a synthetic `"? null:null ?"` placeholder option that has a non-empty value. So `not.toHaveValue("")` returns true while the model is still null, and the next line's `toBeEnabled` immediately fails with `unexpected value "disabled"`.
Asserting on a real algorithm value forces the wait to settle on actual SSE-delivered data.
Failure trace from #454 (this fix's parent symptom)
```
✘ tests/settings.spec.ts:369:7 › Settings — Integrity Check › hash algorithm select changes and saves to backend
Error: expect(locator).toBeEnabled() failed
- unexpected value "disabled"
> 393 | await expect(select).toBeEnabled({ timeout: 5_000 });
```
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit