Skip to content

test: remove weak tests and restore missing CI coverage - #641

Merged
Mathih13 merged 1 commit into
mainfrom
t3/audit-tests
Oct 10, 2026
Merged

Mathih13 merged 1 commit into
mainfrom
t3/audit-tests

Conversation

@Mathih13

@Mathih13 Mathih13 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Some tests only construct rows or copy production logic. Other tests share the constants they should verify, and CI omits useful suites.

This removes four setup-only tests, 15 copied-model tests and three duplicates. It adds four net scanner tests and one test of production subscription queries, for 17 fewer tests against current main. The overlapping source-snapshot removals have already merged in #640.

Raw table-read checks now exclude comments, literals and test-only code. The shared scanner preserves the documented Package API body-helper behavior, including literal values and exact function signatures. Importer fixtures use independent column offsets. Datascript determinism checks require successful emissions and real output files, and the refused-script fixture writes synchronously.

CI gains 359 existing tests across Package Delta, Package API and Datascript suites. The newly enabled tests also exposed a stale quest fixture, which now supplies quest type and checks the exact missing fields. Testing docs and the PR checklist match CI.

All eight findings and their status are in the working report.

What you verified

At f794e576, rebased onto a4ea7c74:

  • Rust workspace tests with Module debug reducers: 4,643 passed, 0 failed, 275 ignored.
  • Bun: 45 passed, 0 failed. Datascript typechecking passed.
  • Workspace Clippy with all targets and features, warnings denied; formatting; diff whitespace; Module print-macro check.
  • Independent adversarial Standards and Spec reviews found no remaining actionable issues. Scanner regressions cover comment and literal decoys, test-only code, generic signatures, final fields and signatures containing array lengths.
  • Earlier targeted mutations of three importer offsets and a failing emitter were rejected.
  • Hosted CI passed all nine checks at f794e576, including all three durable shards and all 45 Bun tests. The Bun job passed after restarting an install that had stalled for nearly 58 minutes.

What you did not verify

Durable and client-data tests were not run locally. Hosted CI passed the durable tier at the final head. Client-data tests remain unverified.

The setup-only, copied-model and duplicate removals add no new durable coverage. Source scanning is lexical and does not prove runtime control flow, expand macros or process inner cfg attributes.

Risk

Changes affect tests, test support, CI and documentation. No gameplay logic or schema changes. The rebase preserves #640's source-check removals, new durable tests and Package API helper contract.

Made with GPT-6 Astra in T3 Code through Codex; adversarial review used GPT-6.1 Sol and GPT-6 Astra.

@Mathih13 Mathih13 self-assigned this Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Two-axis review (Standards / Spec) at 001bbe6

There is no linked issue, so the Spec axis uses the PR description as the spec. I could not reach the external working report.

Standards

No P1 findings. CI, docs/testing.md and the PR template list the same commands. The gateway and module test_scan.rs now call the shared scanner instead of keeping their own copies.

crates/lyracore-test-support/src/source_scan.rs

  • P2, correctness: without_test_code can mask production code. The loop that finds the end of a test-only item (around lines 122–140) stops at ;, a top-level , or a {…} group. It never stops at an unmatched }. If the last field, variant or match arm is #[cfg(test)] with no trailing comma, the masking continues into the next item. For example, in struct S { #[cfg(test)] a: u8 } fn prod() { x.unwrap(); } the : sets field_type, so fn is ignored and the whole body of prod is blanked. The architecture checks then pass without scanning it. This conflicts with CODING_STANDARDS "Tests": keep the scanner narrow and prove it detects the forbidden mutation. Fix: break on an unmatched } and add a test for it.
  • P3: Inner #[cfg(test)] is not matched. This is a safe miss, because the code stays in the scan. Say so in the doc comment.
  • P3: pub fn strip_comments is used only by this file's tests ("Interface depth": don't expose things only for tests). gateway/src/config.rs still has its own strip_comments, which makes two ways to do one thing. Either share it or make it private.
  • P3: in_comment and in_literal re-lex the whole file on every call. The callers in mount.rs, vmap.rs and tripwires.rs make these calls inside loops, so the cost is quadratic. Tests only.
  • P3, judgement call: rustc_lexer = "0.1.0" is a 2019 release and is unmaintained. ra_ap_rustc_lexer is maintained. Newer syntax, such as C-string literals, can mis-lex.

module/src/test_scan.rs

  • P3, possible Mysterious Name: The aliases in_comment as on_comment_line and in_literal as in_string_literal no longer describe what the functions do. The checks are not line-based, and in_literal also matches number and char literals.
  • P3: The "Shared engine…" comment still refers to body_of/code_of "above" and to comment-line filtering. Both are gone from this file (CODING_STANDARDS: update nearby comments).

gateway/src/test_scan.rs, gateway/src/main.rs release_code

  • P3, possible Middle Man: One is a one-line re-export and the other only calls code_only. Callers can import lyracore_test_support::source_scan directly.

Spec

The claims check out: 41 tests removed and 9 added, package_api_lint and lyracore-package-delta added to rust.yml, the full bun test in runtime-scripts.yml, and a real behaviour test for the subscription query lists in connection.rs. Outside test modules, the only production-file changes are to comments.

(c) Looks implemented but looks wrong

  • P2: "Source checks now exclude … text literals". Because literals are now masked, some assertions were weakened or removed:

    • module/src/mount.rs: the Gate test no longer checks the riding-training, dead, combat, indoors or deep-water refusals. Only is_engaged and GHOST are left, so deleting a refusal branch still passes.
    • gateway/src/stdb/reducers.rs (about lines 3890–3928): the three routing tests now match only call_reducer!(realm.0.call_pipe().conn.reducers,. They no longer check which reducer is called (realm_chat, realm_whisper or realm_channel_op), so a swapped reducer name passes.
    • module/src/auction.rs: now accepts any Err(. Minor.

    Suggested fix: check these with a literal-aware match, such as code_of plus the raw body for the string needles, or move them to behaviour tests.

  • P2: "Removed setup, copied-model and snapshot tests gain no new durable coverage". Some of the deleted tests are regression guards, not weak tests:

    • chat.rs apply_send_chat_to_gates_on_is_supported_chat_type_alone (the EMOTE admission bug)
    • vmap.rs the_indoor_query_pre_rejects_before_it_raycasts (fail-open when there is no active generation or the kill switch is off)
    • world_store.rs delete_character_dispatches_through_the_resolved_shard_not_always_self (a past data-loss bug)

    Keep a reduced form of each, or name them in the PR as accepted coverage loss.

(a) Missing or partial

  • P3: "CI gains 358 existing tests". A static count finds about 315 (289 Rust and 26 bun). The difference may come from tests generated in loops. Please confirm the number against a CI run.

(b) Scope creep

  • P3: The PR description does not mention these changes. Each looks justified:
    • the quest_identifiers.rs and quest_type fixture rewrite
    • the tripwires.rs budget entry that was dropped
    • the prose rewrites in transfer/mod.rs, ctx.rs and realm_core.rs
    • the Bun.write → writeFileSync race fix in emit.test.ts

Summary: Standards has 9 findings, and the worst is the P2 without_test_code overrun past an unmatched }, which can hide production code from the source checks. Spec has 7 findings, and the worst is the P2 loss of the mount-refusal and reducer-name assertions when literals were masked.


Generated by Claude Code

@Mathih13

Copy link
Copy Markdown
Contributor Author

Review follow-up at 8f4cc01f:

  • Fixed the scanner overrun. It stops at unmatched closing delimiters and preserves them, while removing the matching delimiters of a complete test-only block. The existing scanner test now covers final fields, variants and match arms without commas, plus body extraction after test-only blocks and nested functions. The reproducer failed before the fix and passes now; independent re-review found no remaining issue in the correction.
  • Removed the test-only public strip_comments helper and the Gateway forwarding wrapper. Updated the stale scanner comment and documented that inner #![cfg(...)] attributes remain in the scan. Kept the crate-local re-exports and legacy aliases to avoid unrelated caller churn. Per-offset re-lexing and a newer lexer remain possible follow-ups; neither is needed by the current source checks. The Module unit suite completes in 0.69 seconds.
  • Reducer selection is still checked. In call_reducer!, the string is $what, a diagnostic label; $method selects the reducer. All three tests still assert their respective realm_chat_then, realm_whisper_then or realm_channel_op_then calls. Restoring the string needles would pin diagnostic wording. The auction test still requires the initial identity comparison and immediate Refusal.
  • Kept the authorized snapshot and message-census removals. The PR now explicitly names the coverage gaps for chat composition, the indoor query, Coordinator delete dispatch and the five mount Refusal branches. These are accepted source-check removals, with no claim of replacement durable coverage.
  • Corrected the CI count to 359. The completed Rust job reports 289 Package Delta tests and 44 Package API tests. The Bun job reports 45 tests, including 19 in the previously enabled Runtime Script suite: 289 + 44 + (45 - 19) = 359. The PR also describes the quest fixture, obsolete exception, stale comments and synchronous file-write changes.

The failed durable job could not connect to its private Standalone during the initial publish. Its log stopped before the listening message; it had not reached gameplay assertions. Startup, publish, the failing test and its workflow are unchanged by this PR. The new head reruns CI without weakening the test or changing retry behavior.

Local verification after the correction: 4,874 Rust tests passed, 260 ignored; workspace Clippy, formatting and diff checks passed. CI must pass before merge.

@Mathih13
Mathih13 merged commit ee4d292 into main Oct 10, 2026
9 of 10 checks passed
@Mathih13
Mathih13 deleted the t3/audit-tests branch October 11, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant