Tests: centralize contract coverage follow-ups#48751
Tests: centralize contract coverage follow-ups#48751vincentkoc merged 10 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
channel: discord
Greptile SummaryThis PR centralizes the contract test infrastructure for both channel plugins and provider plugins — moving helper files from Key issues found:
Confidence Score: 2/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| await processDiscordMessage(messageCtx); | ||
|
|
||
| expect(inboundCtxCapture.ctx).toBeTruthy(); | ||
| expectChannelInboundContextContract(inboundCtxCapture.ctx!); | ||
| }); | ||
|
|
||
| it("keeps Signal inbound context finalized", async () => { | ||
| const handler = createSignalEventHandler( |
There was a problem hiding this comment.
vi.mock() for the same dispatch module
This test file imports inboundCtxCapture from ./inbound-contract-dispatch-mock.js (line 169), which itself registers vi.mock("../../../auto-reply/dispatch.js", ...) that captures context into inboundCtxCapture.ctx. The same file then registers a second, conflicting vi.mock() for the same resolved path (src/auto-reply/dispatch.js) here, which captures into signalCapture.ctx instead.
Only one mock can be active at a time. If the test file's direct mock wins (which is likely since Vitest hoists vi.mock before imports are resolved), then:
- The Discord test at line 341 – which asserts
expect(inboundCtxCapture.ctx).toBeTruthy()– will always fail becauseinboundCtxCapture.ctxis never populated by the winning mock. - The Signal test (line 354) will pass, while the Discord test silently catches a wrong invariant.
The PR description acknowledges "the full Vitest contract lanes did not complete in this checkout", so this conflict may not have been caught locally.
One way to resolve this would be to drop the import of inboundCtxCapture/the helper's vi.mock and instead update the Discord test to use signalCapture.ctx (populated by the local mock), or vice versa – consolidate all channels onto a single capture variable fed by the single local vi.mock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/channels/plugins/contracts/inbound.contract.test.ts
Line: 189-197
Comment:
**Competing `vi.mock()` for the same dispatch module**
This test file imports `inboundCtxCapture` from `./inbound-contract-dispatch-mock.js` (line 169), which itself registers `vi.mock("../../../auto-reply/dispatch.js", ...)` that captures context into `inboundCtxCapture.ctx`. The same file then registers a second, conflicting `vi.mock()` for the same resolved path (`src/auto-reply/dispatch.js`) here, which captures into `signalCapture.ctx` instead.
Only one mock can be active at a time. If the test file's direct mock wins (which is likely since Vitest hoists `vi.mock` before imports are resolved), then:
- The Discord test at line 341 – which asserts `expect(inboundCtxCapture.ctx).toBeTruthy()` – will always fail because `inboundCtxCapture.ctx` is never populated by the winning mock.
- The Signal test (line 354) will pass, while the Discord test silently catches a wrong invariant.
The PR description acknowledges "the full Vitest contract lanes did not complete in this checkout", so this conflict may not have been caught locally.
One way to resolve this would be to drop the import of `inboundCtxCapture`/the helper's `vi.mock` and instead update the Discord test to use `signalCapture.ctx` (populated by the local mock), or vice versa – consolidate all channels onto a single capture variable fed by the single local `vi.mock`.
How can I resolve this? If you propose a fix, please make it concise.| it("maps provider-plugin choices through the shared preferred-provider fallback resolver", async () => { | ||
| const pluginFallbackScenarios = [ | ||
| "github-copilot", | ||
| "qwen-portal", | ||
| "minimax-portal", | ||
| "modelstudio", | ||
| "ollama", | ||
| ].map((providerId) => { | ||
| const provider = requireProviderContractProvider(providerId); | ||
| return { | ||
| authChoice: buildProviderPluginMethodChoice(provider.id, provider.auth[0]?.id ?? "default"), | ||
| expectedProvider: provider.id, | ||
| }; | ||
| }); | ||
|
|
||
| for (const scenario of scenarios) { | ||
| for (const scenario of pluginFallbackScenarios) { | ||
| resolvePreferredProviderPluginProvidersMock.mockClear(); | ||
| await expect( | ||
| resolvePreferredProviderForAuthChoice({ choice: scenario.authChoice }), | ||
| resolvePreferredProviderForAuthChoice({ choice: scenario.authChoice as AuthChoice }), | ||
| ).resolves.toBe(scenario.expectedProvider); | ||
| expect(resolvePreferredProviderPluginProvidersMock).toHaveBeenCalled(); | ||
| } | ||
|
|
||
| resolvePreferredProviderPluginProvidersMock.mockClear(); | ||
| await expect( | ||
| resolvePreferredProviderForAuthChoice({ choice: "unknown" as AuthChoice }), | ||
| ).resolves.toBe(undefined); | ||
| expect(resolvePreferredProviderPluginProvidersMock).toHaveBeenCalled(); |
There was a problem hiding this comment.
The old test explicitly verified that specific auth-choice strings route to the correct provider, e.g.:
{ authChoice: "minimax-global-oauth" as const, expectedProvider: "minimax-portal" },The new test constructs auth choices dynamically via buildProviderPluginMethodChoice(provider.id, provider.auth[0]?.id ?? "default"), which verifies only that a round-trip through the provider's own first auth method resolves back to itself. This no longer catches regressions where a concrete auth-choice string (like "minimax-global-oauth") silently stops resolving to the expected provider ("minimax-portal").
If the intent is to remove the over-specification of individual auth-choice strings (as the PR description suggests), this is acceptable as a deliberate scope reduction. However, it would be worth a comment in the test acknowledging the narrowed contract scope so future contributors understand why the concrete string mapping is no longer asserted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/contracts/auth-choice.contract.test.ts
Line: 121-148
Comment:
**Auth-choice test now only verifies round-trip, not concrete string mapping**
The old test explicitly verified that specific auth-choice strings route to the correct provider, e.g.:
```ts
{ authChoice: "minimax-global-oauth" as const, expectedProvider: "minimax-portal" },
```
The new test constructs auth choices dynamically via `buildProviderPluginMethodChoice(provider.id, provider.auth[0]?.id ?? "default")`, which verifies only that a round-trip through the provider's own first auth method resolves back to itself. This no longer catches regressions where a concrete auth-choice string (like `"minimax-global-oauth"`) silently stops resolving to the expected provider (`"minimax-portal"`).
If the intent is to remove the over-specification of individual auth-choice strings (as the PR description suggests), this is acceptable as a deliberate scope reduction. However, it would be worth a comment in the test acknowledging the narrowed contract scope so future contributors understand why the concrete string mapping is no longer asserted.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77493753a4
ℹ️ 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".
| vi.mock("../../../auto-reply/dispatch.js", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("../../../auto-reply/dispatch.js")>(); | ||
| return { | ||
| ...actual, | ||
| dispatchInboundMessage: dispatchInboundMessageMock, |
There was a problem hiding this comment.
This file imports inboundCtxCapture from inbound-contract-dispatch-mock, which already mocks src/auto-reply/dispatch, but then immediately remocks the same module here and routes all dispatch calls only through dispatchInboundMessageMock. In the Discord contract case, processDiscordMessage uses that remocked dispatch path, so inboundCtxCapture.ctx is not populated and the Discord assertion becomes order-dependent or fails outright. Use one shared mock (or set both capture sinks in this mock) so Discord and Signal captures stay deterministic.
Useful? React with 👍 / 👎.
* Plugins: harden global contract coverage * Channels: tighten global contract coverage * Channels: centralize inbound contract coverage * Channels: move inbound contract helpers into core * Tests: rename local inbound context checks * Tests: stabilize contract runner profile * Tests: split scoped contract lanes * Channels: move inbound dispatch testkit into contracts * Plugins: share provider contract registry helpers * Plugins: reuse provider contract registry helpers
* Plugins: harden global contract coverage * Channels: tighten global contract coverage * Channels: centralize inbound contract coverage * Channels: move inbound contract helpers into core * Tests: rename local inbound context checks * Tests: stabilize contract runner profile * Tests: split scoped contract lanes * Channels: move inbound dispatch testkit into contracts * Plugins: share provider contract registry helpers * Plugins: reuse provider contract registry helpers
1 participant
Summary
CONTRIBUTING.md.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
pnpm test:contracts,pnpm test:contracts:channels, andpnpm test:contracts:pluginslanes documented inCONTRIBUTING.md.Security Impact (required)
Repro + Verification
Environment
Steps
origin/main.Expected
main.Actual
mainand keeps the stricter provider fallback and contract-registry assertions.oxlintpasses on the touched contract files.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
origin/main; resolved the provider auth-choice fallback contract and contract registry conflicts; ranpnpm exec oxlinton the touched contract files.mainand dropped them during rebase.pnpm test:contractsrun from this branch.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Failure Recovery (if this breaks)
src/channels/plugins/contracts/andsrc/plugins/contracts/.Risks and Mitigations