test(admin): cover saveSettings round-trip + restart persistence (#7819)

[JohnMcLear]

Summary

Closes the test gap exposed by #7819:

• Backend (src/tests/backend/specs/admin/adminSettingsSave.ts, new) — 3 specs covering the saveSettings socket on /settings, which previously had zero direct test coverage. Asserts the payload is written byte-for-byte to settings.settingsFilename, augmenting existing JSON with a new top-level block round-trips through the next load reply, and /* */ comments survive the write path.
• E2E (src/tests/frontend-new/admin-spec/adminsettings.spec.ts) — adds #7819 custom top-level block survives server restart. The pre-existing restart works test never sets a custom value before restart, so a deployment that resets settings.json on restart (Docker layer wipe, init-container template render, snap reseed) would pass it. The new test mirrors the workflow Can't add Github's settings for the plugin ep_oauth v11.0.28 - Etherpad 3.1.0 #7819 describes: open Raw, prepend an ep_oauth-shaped block, save, restartEtherpad(), re-login, confirm the block is still in Raw and surfaces in Form view as its own Ep oauth section.

No production code changes — tests only.

Why the gap was there

The save path is dead simple — fsp.writeFile(settings.settingsFilename, newSettings) — so it has felt "obviously correct" enough to skip. But that simplicity hides the actual #7819 failure mode: the deployment layer (not Etherpad) resets the file on restart, and the existing tests only restart the server without first putting a recoverable marker into the file. Now they do.

Test plan

• pnpm run ts-check (passes; no other lint job in CI)
• pnpm test targeting tests/backend/specs/admin/ — 35 passing, including the 3 new ones
• CI runs chromium-admin Playwright project — the new e2e test exercises the restart path under the same conditions as the existing restart works test it sits beside

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Add admin settings persistence tests for issue #7819

🧪 Tests

Walkthroughs

Description

• Add backend specs for saveSettings socket covering round-trip persistence
• Verify payload writes byte-for-byte to disk and survives server restart
• Add e2e test mirroring #7819 workflow: add plugin block, save, restart, confirm persistence
• Ensure comments and custom top-level blocks survive the write path

Diagram

flowchart LR
  A["Admin saveSettings socket"] -->|"Backend specs"| B["Payload write verification"]
  A -->|"Backend specs"| C["Round-trip JSON augmentation"]
  A -->|"Backend specs"| D["Comment preservation"]
  E["E2E restart workflow"] -->|"Add plugin block"| F["Save settings"]
  F -->|"Restart server"| G["Verify persistence"]
  G -->|"Check Raw view"| H["Confirm block survives"]
  G -->|"Check Form view"| I["Verify humanized section"]

Loading

File Changes

1. src/tests/backend/specs/admin/adminSettingsSave.ts 🧪 Tests +220/-0

Backend specs for saveSettings socket persistence

• New file with 3 backend specs for saveSettings socket covering zero-coverage gap
• Spec 1: Verifies payload writes byte-for-byte to settings.settingsFilename without
 transformation
• Spec 2: Tests augmenting existing JSON with new top-level plugin block (ep_oauth) round-trips
 through subsequent load reply
• Spec 3: Confirms /* */ comments survive the write path and are not stripped or normalized
• Includes helper functions adminSocket() and adminSocketWithProbe() for authenticated socket
 connection with timeout handling

src/tests/backend/specs/admin/adminSettingsSave.ts

---

2. src/tests/frontend-new/admin-spec/adminsettings.spec.ts 🧪 Tests +60/-0

E2E test for settings persistence across restart

• Add new e2e test #7819 custom top-level block survives server restart to regression coverage
• Test mirrors user workflow: open Raw view, inject ep_oauth plugin block with marker, save,
 restart, re-login
• Verify injected block persists in Raw view after restart and surfaces in Form view as humanized
 section
• Include cleanup step to restore original settings and prevent poisoning subsequent tests

src/tests/frontend-new/admin-spec/adminsettings.spec.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 Can't add Github's settings for the plugin ep_oauth v11.0.28 - Etherpad 3.1.0

1. Missing settings cleanup guard 🐞 Bug ☼ Reliability

Description

The new Playwright restart-persistence test restores the original raw settings only at the end of
the test body; if any assertion fails before the restore, the injected ep_oauth block remains
persisted and can break subsequent serial admin settings tests that assume a clean settings file.

Code

src/tests/frontend-new/admin-spec/adminsettings.spec.ts[R179-185]

+    // Restore — DO NOT rely on the next test's cleanup; the file is
+    // shared across the serial run and a custom ep_oauth block could
+    // poison anything that asserts the exact length of settings.
+    await page.getByTestId('mode-toggle-raw').click();
+    await rawAfter.fill(original);
+    await saveSettings(page);
+  });

Evidence

The suite explicitly runs serially and the new test acknowledges the settings file is shared, but
the restore sequence is not protected by a finally/afterEach, so it will be skipped on any mid-test
failure.

src/tests/frontend-new/admin-spec/adminsettings.spec.ts[4-6]
src/tests/frontend-new/admin-spec/adminsettings.spec.ts[127-185]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test `#7819 custom top-level block survives server restart` mutates the shared settings file and only restores it at the very end. If the test fails (assertion/timeout/restart issues) before the restore block, the suite remains polluted and later tests in the same serial run can fail.

### Issue Context
This suite is configured to run serially, and the test itself notes the settings file is shared across the serial run.

### Fix Focus Areas
- src/tests/frontend-new/admin-spec/adminsettings.spec.ts[134-185]

### Suggested fix
Wrap the mutation + assertions in a `try { ... } finally { ... }` and move the restore logic into the `finally` so it executes even on failure.

Example sketch:
```ts
const original = await raw.inputValue();
let mutated = false;
try {
 const augmented = ...;
 mutated = true;
 ... assertions ...
} finally {
 if (mutated) {
   // best-effort restore
   await page.goto('http://localhost:9001/admin/settings');
   await page.getByTestId('mode-toggle-raw').click();
   await page.getByTestId('settings-raw-textarea').fill(original);
   await saveSettings(page);
 }
}
```
(You can keep it minimal by reusing existing locators; the key is ensuring restore runs even when the test fails.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

1. Missing settings cleanup guard 🐞 Bug ☼ Reliability

The new Playwright restart-persistence test restores the original raw settings only at the end of
the test body; if any assertion fails before the restore, the injected ep_oauth block remains
persisted and can break subsequent serial admin settings tests that assume a clean settings file.

Agent Prompt

### Issue description
The test `#7819 custom top-level block survives server restart` mutates the shared settings file and only restores it at the very end. If the test fails (assertion/timeout/restart issues) before the restore block, the suite remains polluted and later tests in the same serial run can fail.

### Issue Context
This suite is configured to run serially, and the test itself notes the settings file is shared across the serial run.

### Fix Focus Areas
- src/tests/frontend-new/admin-spec/adminsettings.spec.ts[134-185]

### Suggested fix
Wrap the mutation + assertions in a `try { ... } finally { ... }` and move the restore logic into the `finally` so it executes even on failure.

Example sketch:
```ts
const original = await raw.inputValue();
let mutated = false;
try {
  const augmented = ...;
  mutated = true;
  ... assertions ...
} finally {
  if (mutated) {
    // best-effort restore
    await page.goto('http://localhost:9001/admin/settings');
    await page.getByTestId('mode-toggle-raw').click();
    await page.getByTestId('settings-raw-textarea').fill(original);
    await saveSettings(page);
  }
}
```
(You can keep it minimal by reusing existing locators; the key is ensuring restore runs even when the test fails.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools