fix(test): de-flake mass transfer NIfTI conversion acceptance test

[samuelvkwong]

Summary

• test_mass_transfer_to_folder_with_nifti_conversion[chromium-c-move] was flaking ~5–10% of runs with AssertionError: No NIfTI files were generated.
• Root cause: the form's Convert to NIfTI checkbox is reactively disabled by Alpine.js (x-effect) until isDestinationFolder updates after the destination select changes. The original click(force=True) bypassed Playwright's actionability checks, but the browser silently drops clicks on disabled checkboxes — and disabled fields are stripped from the POST. So the job was submitted with convert_to_nifti=False, the processor ran the non-NIfTI export path, reported success, and the file-existence assertion failed.
• Fix: bypass the Alpine.js timing window entirely by setting disabled=false; checked=true on the input via page.evaluate right before form submission.

Test plan

• Stress test the previously flaky case 100x — 100 passed, 0 failed
• Run the full 12-test acceptance matrix (4 tests × 3 protocols: c-move, c-get, dicomweb) — all pass

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved test reliability for the mass transfer form submission flow by making the test interact with the "Convert to NIfTI" option in a way that avoids timing/race issues with the reactive UI, reducing flaky failures during destination changes while preserving the overall form-fill and job submission behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 70ea5ff6-cb5f-4a0c-9f5a-3817aec9d176

📥 Commits

Reviewing files that changed from the base of the PR and between 347c337 and 62ccb7d.

📒 Files selected for processing (1)

• adit/mass_transfer/tests/acceptance/test_mass_transfer.py

🚧 Files skipped from review as they are similar to previous changes (1)

• adit/mass_transfer/tests/acceptance/test_mass_transfer.py

---

📝 Walkthrough

Walkthrough

The acceptance test helper _fill_mass_transfer_form now calls Playwright's page.get_by_label(...).set_checked(True) to enable and check the "Convert to NIfTI" checkbox, avoiding races where Alpine.js temporarily disables the input during destination changes.

Changes

Mass Transfer Form Submission Test

Layer / File(s)	Summary
Test Helper Update adit/mass_transfer/tests/acceptance/test_mass_transfer.py	_fill_mass_transfer_form replaces a forced click with page.get_by_label(...).set_checked(True) and adds comments about avoiding a race where Alpine.js may disable the checkbox during destination changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I hopped through tests to find a stray tick,
Alpine would hide it—oh, what a trick!
With set_checked's nudge the box stays bright,
Now forms submit clean in the soft moonlight. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing a flaky acceptance test for mass transfer NIfTI conversion by using set_checked(True) to avoid Alpine.js race conditions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/flaky-mass-transfer-nifti-test

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request modifies the mass transfer acceptance tests to handle a race condition where Alpine.js might disable the 'Convert to NIfTI' checkbox during form submission. The current implementation uses page.evaluate to force-enable the field via JavaScript. Feedback suggests replacing this with Playwright's idiomatic .set_checked(True) method, which is less brittle and automatically waits for the element to become enabled, ensuring the test remains stable without bypassing the application's reactive logic.

[coderabbitai]

🧹 Nitpick comments (1)

adit/mass_transfer/tests/acceptance/test_mass_transfer.py (1)

41-45: ⚡ Quick win

Consider applying the same page.evaluate pattern to the pseudonymize checkbox.

The pseudonymize_checkbox.click(force=True) calls use the same approach that was identified as the root cause of the NIfTI flakiness. If the Pseudonymize checkbox ever gains a reactive disabled binding in Alpine.js (e.g., tied to destination type or permissions), it will exhibit the same silent-drop behaviour. Replacing it now would keep the two checkbox interactions consistent and preempt a future flake.

♻️ Proposed refactor

-    pseudonymize_checkbox = page.get_by_label("Pseudonymize")
-    if pseudonymize and not pseudonymize_checkbox.is_checked():
-        pseudonymize_checkbox.click(force=True)
-    elif not pseudonymize and pseudonymize_checkbox.is_checked():
-        pseudonymize_checkbox.click(force=True)
+    page.evaluate(
+        """(checked) => {
+            const cb = document.getElementById('id_pseudonymize');
+            if (cb) { cb.disabled = false; cb.checked = checked; }
+        }""",
+        pseudonymize,
+    )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@adit/mass_transfer/tests/acceptance/test_mass_transfer.py` around lines 41 -
45, The pseudonymize checkbox click currently uses
pseudonymize_checkbox.click(force=True) which can silently fail if the element
becomes disabled; replace that interaction with the same page.evaluate pattern
used elsewhere: locate the element (pseudonymize_checkbox) and run page.evaluate
in the page context to set its checked state to the desired boolean
(pseudonymize) and dispatch appropriate input/change events so Alpine/reactive
bindings update (e.g., element.checked = ...; element.dispatchEvent(new
Event('input', { bubbles: true })); element.dispatchEvent(new Event('change', {
bubbles: true }))); keep the conditional logic using the pseudonymize variable
to avoid unnecessary DOM ops.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@adit/mass_transfer/tests/acceptance/test_mass_transfer.py`:
- Around line 41-45: The pseudonymize checkbox click currently uses
pseudonymize_checkbox.click(force=True) which can silently fail if the element
becomes disabled; replace that interaction with the same page.evaluate pattern
used elsewhere: locate the element (pseudonymize_checkbox) and run page.evaluate
in the page context to set its checked state to the desired boolean
(pseudonymize) and dispatch appropriate input/change events so Alpine/reactive
bindings update (e.g., element.checked = ...; element.dispatchEvent(new
Event('input', { bubbles: true })); element.dispatchEvent(new Event('change', {
bubbles: true }))); keep the conditional logic using the pseudonymize variable
to avoid unnecessary DOM ops.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7bebf201-ef49-419c-9235-a53226d2a66b

📥 Commits

Reviewing files that changed from the base of the PR and between 6e0b9b6 and 347c337.

📒 Files selected for processing (1)

• adit/mass_transfer/tests/acceptance/test_mass_transfer.py