fix(backend-tests): un-skip api/ and admin/ subdirectory specs#7789
Merged
Conversation
The pnpm test script's glob `tests/backend/specs/**.ts` only matched files at the top level of tests/backend/specs/. Every spec under tests/backend/specs/api/ (14 files) and tests/backend/specs/admin/ (2 files) has been silently skipped by CI — including the failing tests reported in #7785, #7786, #7787, #7788. Switch to passing the directories with `--extension ts --recursive`, which makes mocha walk the tree the way --recursive is documented to. Local run after this change picks up 16 additional spec files and surfaces 6 newly-visible failures: 4 already filed (#7785–#7788) plus one that wasn't yet filed (appendTextAuthor.ts: "appendText without authorId does not attribute to any author"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoFix backend test glob to discover nested spec directories
WalkthroughsDescription• Fix glob pattern to include nested test directories - Changed from tests/backend/specs/**.ts to tests/backend/specs with `--extension ts --recursive` - Mocha now discovers 74 spec files instead of 53 (16 additional files) • Surfaces 6 previously hidden failing tests in api/ and admin/ subdirectories - Tests in pad.ts, importexportGetPost.ts, and appendTextAuthor.ts now visible • Enables proper recursive test discovery for plugin specs - Plugin specs in ../node_modules/ep_*/static/tests/backend/specs also benefit from recursive flag Diagramflowchart LR
A["Old glob pattern<br/>tests/backend/specs/**.ts"] -->|"Only matches<br/>top-level files"| B["53 spec files<br/>discovered"]
C["New glob pattern<br/>tests/backend/specs<br/>with --recursive"] -->|"Walks directory tree<br/>recursively"| D["74 spec files<br/>discovered"]
B -->|"Skips"| E["api/ subdirectory<br/>14 files hidden"]
B -->|"Skips"| F["admin/ subdirectory<br/>2 files hidden"]
D -->|"Includes"| E
D -->|"Includes"| F
E -->|"Surfaces"| G["6 failing tests<br/>now visible"]
File Changes1. src/package.json
|
Code Review by Qodo
Context used 1. No regression test for pnpm test
|
This was referenced May 17, 2026
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
…iscovery Read the pnpm test script from src/package.json, hand mocha the same arguments under --dry-run --list-files, and assert that a representative spec from tests/backend/specs/api/ and tests/backend/specs/admin/ appears in the discovered list. Locks in the glob fix from this PR: if anyone re-narrows the script back to the previous tests/backend/specs/**.ts pattern (which only matches depth 1), this vitest fails with a clear "glob missed ..." message instead of letting the affected specs silently drop out of CI. Verified the test FAILS when the script is reverted to the broken glob. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
added a commit
that referenced
this pull request
May 17, 2026
#7796) * test(admin): skip anonymizeAuthorSocket suite when ep_hash_auth is installed #7789 un-hid this suite from CI and immediately surfaced a 14-minute stall on every with-plugins matrix run (Linux + Windows + the 'Upgrade from latest release' workflow). Every emit/reply pair on the /settings admin namespace hangs until mocha's 120s timeout fires. Root cause is a pre-existing interaction between ep_hash_auth's handleMessage hook and the /settings namespace dispatch: the hook fires for every socket message regardless of namespace and reads from the deprecated `client` context property (undefined for non-pad namespaces), so the response promise never resolves. Tracked separately in #7795. Until that lands, gate the suite on require.resolve('ep_hash_auth'). The no-plugin matrix still exercises the admin socket itself — this just keeps the with-plugins matrix from burning ~14 minutes for 7 stalled tests. Verified locally: - no ep_hash_auth in node_modules → 7 passing - ep_hash_auth resolvable → 0 passing, 7 pending Why require.resolve and not pluginDefs.plugins[...]: Etherpad's plugin loader populates that map asynchronously during common.init. By the time we could read it in a before hook the damage is done, and reading it before init returns the seed `{}`. Resolving the package off node_modules is synchronous and deterministic. Refs #7795 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): probe at the application layer; restore settings safely on skip Two Qodo follow-ups on this PR: 1) Replace the static `require.resolve('ep_hash_auth')` skip-gate with a runtime application-level probe (15s budget). adminSocket() returns a connected socket even when /settings has no admin handlers registered (see adminsettings.ts:25 — non-admin sockets exit early without binding listeners). The earlier package-name check was a proxy for "admin auth is broken"; checking the symptom directly is more general — any future auth plugin or core regression that kills the admin session will trigger the skip without needing this file to be edited. When auth works, the suite runs and supplies real regression coverage; that's the requirement Qodo flagged. 2) Guard after() with a setupCompleted flag. The skip-via-this.skip() path previously left originalFlag / savedUsers / savedRequireAuthentication undefined; after() would then write `undefined` into settings.gdprAuthorErasure.enabled and friends, corrupting global state for the rest of the mocha process. Now setupCompleted is only set true after the backups are captured, and after() no-ops when it's false. Verified locally: - no-plugin matrix → 7 passing (2s) - broken-auth sim → 0 passing, 7 pending (17s) Refs #7795 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pnpm test(insrc/) has been silently skipping every backend spec undertests/backend/specs/api/andtests/backend/specs/admin/. The globtests/backend/specs/**.tsonly resolves to files at depth 1 —**without a slash separator is treated like*by both bash and minimatch, and the shell pre-expands the pattern before mocha sees it, so--recursivehas nothing to recurse into.Confirmed against the most recent green CI run on develop (b96e262, run id 25970235389):
pad.ts,importexportGetPost.ts,listAuthorsOfPad,copyPadWithoutHistory, or any other test name from the api/admin subdirs.tests/backend/specs/api/**either.The fix passes the directories to mocha with
--extension ts --recursiveso mocha does its own walk.What this surfaces
After the fix, mocha picks up 74 spec files instead of 53. The 16 newly-discovered files contain 6 failing tests:
tests/backend/specs/api/pad.tsGet Authors of the Padtests/backend/specs/api/pad.tsPad with complex nested lists of different typestests/backend/specs/api/pad.tscopyPadWithoutHistory > creates a new pad with the same content as the source padtests/backend/specs/api/importexportGetPost.tstxt request rev test1 is 403tests/backend/specs/api/importexportGetPost.tshtml request rev test1 results in 500 responsetests/backend/specs/api/appendTextAuthor.tsappendText without authorId does not attribute to any authorAll
admin/specs pass.Two tests listed in #7786's body (
ugly HTML,white space between list items) and one in #7787 (attribute pools are independent) do not reproduce as failures on develop HEAD; the issue bodies are slightly stale there.Trade-off
This change will make the next CI run red until #7785–#7788 (and the new
appendTextAuthor.tsfailure) are fixed. That's the point — those tests have been silently broken. Merging this is what makes them visible.Test plan
🤖 Generated with Claude Code