Tests: stabilize Windows ACL tests + command auth registry#9493
Tests: stabilize Windows ACL tests + command auth registry#9493
Conversation
PR SummaryLow Risk Overview Adjusts auto-reply command authorization tests to register a minimal Written by Cursor Bugbot for commit daebb74. This will update automatically on new commits. Configure here. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| beforeEach(() => { | ||
| setActivePluginRegistry(createTestRegistry([])); | ||
| setActivePluginRegistry(createRegistry()); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| setActivePluginRegistry(createTestRegistry([])); | ||
| setActivePluginRegistry(createRegistry()); | ||
| }); |
There was a problem hiding this comment.
Global plugin registry not restored
setActivePluginRegistry writes to a global singleton (src/plugins/runtime.ts:19-46), so leaving a non-empty registry configured in afterEach means subsequent test files can observe a lingering discord plugin even if they expect the default empty registry. This can create order-dependent failures when the full suite runs in a single worker/process. Consider restoring the previous registry state (save/restore) or resetting to an empty registry in afterEach instead of re-applying createRegistry().
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/command-control.test.ts
Line: 21:27
Comment:
**Global plugin registry not restored**
`setActivePluginRegistry` writes to a global singleton (`src/plugins/runtime.ts:19-46`), so leaving a non-empty registry configured in `afterEach` means subsequent test files can observe a lingering `discord` plugin even if they expect the default empty registry. This can create order-dependent failures when the full suite runs in a single worker/process. Consider restoring the previous registry state (save/restore) or resetting to an empty registry in `afterEach` instead of re-applying `createRegistry()`.
How can I resolve this? If you propose a fix, please make it concise.| const { | ||
| createIcaclsResetCommand, | ||
| formatIcaclsResetCommand, | ||
| formatWindowsAclSummary, | ||
| inspectWindowsAcl, | ||
| parseIcaclsOutput, | ||
| resolveWindowsUserPrincipal, | ||
| summarizeWindowsAcl, | ||
| type WindowsAclEntry, | ||
| type WindowsAclSummary, | ||
| } from "./windows-acl.js"; | ||
| } = await import("./windows-acl.js"); |
There was a problem hiding this comment.
Top-level await in tests
This file uses a top-level await import("./windows-acl.js"). If the test runner/transpilation target doesn’t support top-level await for .test.ts in all environments this repo runs in, this will fail to load the test file entirely (syntax/runtime error before any test executes). If the intention is only to ensure vi.mock("node:os") runs before importing windows-acl, it’s safer to do the dynamic import inside a beforeAll/beforeEach (or otherwise ensure the repo explicitly supports TLA in Vitest across Node versions).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/windows-acl.test.ts
Line: 11:19
Comment:
**Top-level await in tests**
This file uses a top-level `await import("./windows-acl.js")`. If the test runner/transpilation target doesn’t support top-level await for `.test.ts` in all environments this repo runs in, this will fail to load the test file entirely (syntax/runtime error before any test executes). If the intention is only to ensure `vi.mock("node:os")` runs before importing `windows-acl`, it’s safer to do the dynamic import inside a `beforeAll`/`beforeEach` (or otherwise ensure the repo explicitly supports TLA in Vitest across Node versions).
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Testing
Greptile Overview
Greptile Summary
This PR updates test-only code to reduce flakiness on Windows by making the current-username source deterministic and by ensuring the command authorization code can normalize provider-prefixed allowlist entries (e.g.
discord:123) during tests.Changes include:
src/security/windows-acl.test.ts: mocksnode:os.userInfo()and switches to a dynamic import ofwindows-aclso the mock is applied before the module is loaded; assertions are tightened to check the mocked username is used.src/auto-reply/command-control.test.ts: sets up a test plugin registry containing adiscordplugin so channel/owner allowlist normalization behaves consistently.CHANGELOG.md: adds an entry documenting the test stabilization.Confidence Score: 3/5