test: pin the config-validation contract and restore head-isolation coverage - #3294
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens integration test coverage by asserting the existing config-validation error contract (slug, message prefix, and structured context) and by restoring meaningful multi-tenant head-collector isolation tests that deterministically interleave concurrent requests.
Changes:
- Pin
config-validation-failederror contract assertions across integration config-loader tests (slug,Invalid veryfront.config at <field>:prefix, andcontext.field/context.expected). - Re-enable and rewrite head-collector multi-tenant isolation tests to be deterministic via a barrier (instead of timing-based sleeps) and to assert exact collected head output.
- Tighten a test server helper return shape and lower the skipped-tests baseline to reflect re-enabled coverage.
Verification (reviewer-run):
- Not run in this review environment.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/critical-bugs/multi-tenant-isolation.test.ts | Restores non-vacuous, deterministic head-collector isolation coverage under concurrency. |
| tests/integration/core/loader.test.ts | Asserts config validation error slug/context for unknown keys and pins CORS error prefix. |
| tests/integration/core/config-schema.test.ts | Pins config-validation-failed contract for schema validation failures. |
| tests/integration/core/config-loader-edge-cases.test.ts | Adds a shared helper to assert the full config-validation error contract across edge cases. |
| tests/_helpers/server.ts | Avoids spreading ServerHandle into the test server shape by explicitly mapping fields. |
| scripts/lint/check-skipped-tests-baseline.ts | Lowers skipped-test baseline to match re-enabled tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kojiwakayama
force-pushed
the
test/validation-contract-and-deflake
branch
from
August 3, 2026 00:27
5b770ac to
f30f4ac
Compare
The config validation tests matched loose message substrings such as "security.cors" or "Unrecognized keys". That form passes even if the loader degrades to a bare Error carrying no slug and no machine-readable context, so it could not catch a regression in the structured half of the contract that callers actually consume. Assert all three parts instead: the `config-validation-failed` registry slug, the `Invalid veryfront.config at <field>:` message prefix, and the `context.field` / `context.expected` pair. Table-drive the non-object export cases to cover empty string, false and zero alongside string, null and undefined. Verified against the current loader: mutating `field: path` to a constant in config.schema.ts turns 21 previously-green steps red across the three files.
Both head-collector isolation tests were disabled with it.ignore. Re-enabling them as written would not have helped: they called collectHead outside any runWithHeadCollector scope, so the AsyncLocalStorage store was undefined, every write was a no-op and flushHeadCollector returned an empty head. Their assertions were all of the "not contaminated" shape, which an empty head satisfies, so they would have passed while testing nothing. Scope each simulated request with runWithHeadCollector and replace the randomised sleeps with a barrier that releases only once every participant has arrived. Randomised sleeps make the concurrent overlap merely probable, so a run whose timers failed to align would pass even with head state fully shared; the barrier forces the interleaving on every run. Assertions now pin exact titles, descriptions and meta arrays rather than absence of contamination. Also build the production test-server adapter explicitly instead of spreading the handle, and lower the skipped-test baseline from 20 to 18. Verified: sharing one head across runWithHeadCollector calls turns both restored tests red; 12 consecutive runs pass with no flake.
kojiwakayama
force-pushed
the
test/validation-contract-and-deflake
branch
from
August 3, 2026 00:36
f30f4ac to
3005c14
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/integration/core/config-loader-edge-cases.test.ts:86
- Redundant type assertions:
assertEquals(typeof contextExpected, "string")andassert(typeof contextExpected === "string")both check the same condition. Keeping a singleassert(...)is enough and also provides TS narrowing.
assertEquals(typeof contextExpected, "string");
assert(typeof contextExpected === "string");
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
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.
Ported from
codex/module-reconcile-20260723with per-assertion verification against current main; two branch hunks rejected as stale (one would have regressed coverage instreaming.test.ts; the other targeted aServerHandleshape main doesn't have).error.slug === "config-validation-failed", theInvalid veryfront.config at <field>:prefix, and structuredcontext.field/context.expected; non-object-export cases table-driven 3 → 6. Mutation-checked (constant-folding the field path turns 21 steps red).collectHeadoutside anyrunWithHeadCollectorscope: every write was a no-op, so an empty head satisfied their "does not contain" assertions — simply un-ignoring them would have passed while testing nothing (probed empirically). Rewritten to scope each request, assert exact titles/metas, and force the interleaving with a barrier instead ofdelayRandom. 12/12 consecutive runs green; mutation-checked (sharing one head object turns both red). Skipped-tests baseline lowered 20 → 18.Loose end for a future cleanup:
createTestProductionServerhas zero callers repo-wide.Verification: 5 suites, 70 steps, 0 failed (baseline 65 with 2 ignored); fmt/lint/check clean on all six touched files.
test:unitin 4 unrelated react files — merge #3291 first.