Repository navigation
🧪 fix: Delete the USER Profile the Config Shell Spec Creates and Unbreak the Live Profiles and Import Specs - #201
Conversation
…eak the Live Profiles and Import Specs e2e/config-shell.spec.ts (live) created the USER role's profile on a clean database to test Back from a profile and never deleted it, so later runs and other specs started from a different state. The test now records when it created the profile, and an afterEach deletes it through the scope selector, as an admin would, while it still has no overrides. A profile the test found is left alone. The selector helper waits for the profile list to load before deciding whether USER has a profile, so that decision is never made on an empty, still-loading list. After confirming the delete, the cleanup waits for the profile list to come back before checking that USER is gone: the confirm view lists no options and stays up until the delete succeeds, so checking only that the row is gone would pass at once, even when the delete failed. e2e/profiles.spec.ts: the Registration section of a user profile shows two notes, the scope gate and the read-only note for the settings LibreChat sets at startup, so `#section-registration [role=note]` was a strict-mode violation. The locator now targets the scope note (`data-read-only=scope`). e2e/import.spec.ts: ensureUserProfile counted the "Create profile" button once after networkidle, before the roles query had rendered it, so on a fresh database it skipped creating the USER profile and "imports into an existing profile" timed out. It now waits until the page shows either the open profile or the Create profile button, and creates the profile in the second case.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79a953ab66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # e2e/live.ts
Pull Request Template
Summary
Three live e2e specs (
E2E_LIVE=1) left data behind or failed onmain. The specs that use the USER role's profile now also take turns, so that the new cleanup can't delete that profile while another spec is using it. This PR changes only files ine2e/.e2e/config-shell.spec.tsleft a USER role profile behind. On a clean database, "Back after selecting a profile restores Base" creates the USER role's profile so it has a profile to select, and nothing deleted it. After one run, every later run and every other spec started from a different database. For example,import.spec.tswould then pass "imports into an existing profile" because the profile was already there.afterEachdeletes it through the scope selector (Delete profile, then Delete configuration), the same way an admin would. TheafterEachalso runs when the test fails.The specs that use the USER role's profile now take turns. The cleanup above made a race possible.
playwright.config.tssetsfullyParallel, and local runs use several workers. Soimport.spec.tscould open the empty USER profile thatconfig-shell.spec.tshad just created, and the cleanup then deleted it while the import wizard was still using it. The import could also save its value between the "No overrides" check and the delete, and the delete then removed the value along with the profile. An override count can't tell which test owns the profile. LibreChat has only two built-in roles, ADMIN and USER, andimport.spec.tswrites to both, so movingconfig-shell.spec.tsto the other role would only move the race.e2e/live.tsaddslockUserProfile(cleanup?). Every test in a describe that calls it holds one shared lock for the whole test, across workers and spec files. AbeforeEachwaits for the lock. TheafterEachruns the cleanup first and then releases the lock, also when the test failed or timed out. A processexithandler releases it as well.config-shell.spec.ts, with the profile delete as its cleanup; "imports into an existing profile" inimport.spec.ts; and "a profile shows the specs it inherits" inspecs.spec.ts. "Back after selecting a profile" now has its own describe, so the other two profile-scope tests don't wait for the lock.mkdir, which is atomic, in the OS temp directory. Its name comes fromE2E_BASE_URL's host, port and path, with127.0.0.1,[::1]and a trailing slash read as the same panel, so runs against different panels never wait on each other and two spellings of one panel's URL still share the lock. The holder writes its process id into it.e2e/profiles.spec.tshit a strict-mode error. In a user profile, the Registration section now shows two notes: the scope gate ("Registration: applies to everyone") and the read-only note for settings LibreChat sets at startup.#section-registration [role=note]therefore matched both elements. The locator now targets the scope note:[role=note][data-read-only=scope].e2e/import.spec.tsraced the roles query.ensureUserProfilecounted the "Create profile" button once, right afternetworkidle. On a fresh database the button only appears once the roles query has returned, which can be a few seconds later. So the helper often found no button, skipped creating the profile, and "imports into an existing profile" timed out waiting for Import YAML. The helper now waits until the page shows either the open profile or the Create profile button, and clicks Create only in the second case.Not in this PR
import.spec.tsstill leaves its imports in the database: a Basebalance.startBalance, values in the USER and ADMIN profiles, and one newImported <timestamp>role each run. Its tests pick fresh values, so it re-runs fine anyway.profiles.spec.tsfails before its delete step, the profile forE2E_PROFILE_USERstays behind. A re-run then can't offer that user under Create.specs.spec.ts"a profile shows the specs it inherits" skips without a USER role profile. Before this PR it usually had one, left behind byconfig-shell.spec.ts. On a clean database it now skips unlessimport.spec.tshas run first.specs.spec.ts"a new spec asks for Endpoint and Model up front" fails even when it runs alone on a clean database: after Add spec, the new card's Endpoint select is not focused. This PR changes neither that test nor the panel, somainfails the same way. The rest of that serial describe then doesn't run, so the race runs below include only the USER profile test fromspecs.spec.ts.Change Type
Testing
All runs were live (
E2E_LIVE=1) against a local LibreChat backend on a fresh database, with the worker counts given below. Between runs, config overrides and profiles were reset where noted.mainconfig-shell.spec.tsrole / USERconfig document with no overrides.profiles.spec.ts#section-registration [role=note]resolved to 2 elements,data-read-only="scope"anddata-read-only="yamlOnly".import.spec.tsThis branch
config-shell.spec.ts, twice in a rowconfig-shell.spec.ts_id) was still there afterwards.config-shell.spec.ts, with a deliberate failure added right after the profile is created (not committed)afterEachstill deleted the profile.config-shell.spec.ts, with the delete request aborted in the browser (not committed)afterEachfailed: the profile list never came back within 10 s. The profile was left behind, and the run reported it. A cleanup that only checked for the USER row passed in the same setup and left the same profile.profiles.spec.ts, twice in a rowimport.spec.ts, twice in a rowE2E_LIVE)The USER profile race (
--workers=4, starting from a clean database each time; a change stream on the config collection and a watcher on the lock directory recorded what happened)--repeat-each=3, three runsNot an input element). In 3 the profile was gone before the wizard opened, and the test timed out waiting for Import YAML. In 1 the import never reported success. In every run, the "Back" cleanup deleted the USER profile about 0.4 s after an import had savedbalance.startBalanceto it. One "Back" cleanup failed because another copy had already created USER again. Two of the three runs ended with no USER profile at all.config-shell.spec.ts,import.spec.tsand thespecs.spec.tsUSER profile test,--repeat-each=3, three runsimport.spec.tsis known to leave (see Not in this PR). No empty USER profile and no lock directory were left.config-shell.spec.tsand the specs test,--repeat-each=3Gave up after 10 s waiting for the USER role profile lock <dir>, held by pid <pid>. The holder's lock was left in place.--workers=2afterEachstill deleted the profile and released the lock. The import test, which had been waiting, then took the lock, created its own profile and passed.The runs in the This branch table above were made before the lock was added.
At the head of this PR,
npx tsc --noEmit(which coverse2e/),npx eslint src/ e2e/ --max-warnings 0and prettier on the changed files pass.bun run test:lint-rules,npx vitest run(154 files, 3338 tests) andbun run buildpassed before the lock was added. The lock changes only files ine2e/, which vitest excludes.Test Configuration:
E2E_LIVE=1 E2E_BASE_URL=<panel> E2E_ADMIN_EMAIL=<admin> E2E_ADMIN_PASSWORD=<password> npx playwright test e2e/config-shell.spec.ts e2e/profiles.spec.ts e2e/import.spec.ts --workers=2profiles.spec.tsalso needsE2E_PROFILE_USER, set to the email of a user that has no profile.... npx playwright test e2e/config-shell.spec.ts e2e/import.spec.ts e2e/specs.spec.ts:182 --workers=4 --repeat-each=3. The before-and-after runs of the three USER profile tests usede2e/specs.spec.tswith--grep "Back after selecting|imports into an existing profile|inherits read-only".devbackend.Checklist