Run the integration tests in CI, and make the fixture check real (LC-605) #561

Merged

longjacksonle merged 1 commit from fix/lc605-ci-runs-integration-tests into main 2026-07-19 03:56:40 +02:00

Conversation 1 Commits 1 Files changed 7 +453 -401

longjacksonle commented 2026-07-19 03:44:41 +02:00

Owner

Copy link

Closes LC-605.

CI ran cargo test --lib -p lets-chat-server, executing only the 266 in-crate unit tests. The 177 integration binaries under server/tests/ were compiled by the --all-targets build and clippy steps but never run - so roughly 770 tests, about 74% of the 1036 that pass locally, could not fail the pipeline.

Compiling them is exactly what made this invisible: a broken integration test still went green, and even a non-compiling one was caught, so the job looked like it covered them. Three long-lived breakages reached main this way (LC-600, and the two below).

Dropping --lib runs both sets. The marginal cost is modest because the earlier steps already compile these binaries - locally the full suite adds ~100s wall-clock over --lib on a warm cache (18s vs 119s).

Two blockers had to be fixed first

Enabling this without them would just have turned CI red.

1. routes_drafts draft-indicator tests

They matched the literal ✎. Commit 06913c8 ("draft indicator is an SVG pencil, not a tofu dingbat") replaced that entity and did not update the tests.

• sidebar_shows_draft_pencil_for_room_with_draft had been failing on main ever since.
• sidebar_omits_draft_pencil_when_no_draft kept passing while asserting the absence of a string that no longer appears anywhere - which is the worse failure of the two, because it read as coverage and was not.

Both now assert on the badge's structure (populated indicator vs empty OOB-target span) through a small draft_badge helper, so the next icon change does not break them.

2. The lc77 fixture check was inert locally

I reported this on the ticket as "auto-rewrites on mismatch". That was wrong, and the real cause is worth stating precisely:

if std::env::var("FIXTURE_WRITE").is_ok() {   // <-- present-but-empty counts as opt-in

dev/cargo forwards -e "FIXTURE_WRITE=${FIXTURE_WRITE:-}", which sets the variable present and empty. env::var returns Ok(""), so the write branch fired on every local run through the standard wrapper: the golden files were silently regenerated and the test always passed. The assertion only worked under a bare cargo test - i.e. nowhere anyone actually ran it, and (until this PR) nowhere in CI either.

The predicate now requires a meaningful value, so the default is to assert. Verified both directions: with stale fixtures the tests fail and leave the files untouched; with FIXTURE_WRITE=1 they regenerate.

The four fixtures are regenerated here to match current templates. The drift is the LC-553 reaction button and the LC-598 menu grouping - both intentional, neither previously captured.

Verification

• Full suite green; just check green.
• Fixture gate confirmed in both directions (see above).
• Suite passes with server/assets/tailwind-built.css removed, confirming CI needs no build-css step for it - compute_asset_version falls back and tests construct AppState with asset_version: "test".

What this does not do

I have not seen this job run on the runner. The remaining risk is wall-clock and any runner-specific environment difference; the local evidence above is the best proxy I can produce from here. If the added time is a problem, the natural follow-up is splitting the integration run into its own parallel job rather than reverting to --lib.

🤖 Generated with Claude Code

https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

Closes LC-605. CI ran `cargo test --lib -p lets-chat-server`, executing only the **266** in-crate unit tests. The **177** integration binaries under `server/tests/` were compiled by the `--all-targets` build and clippy steps but never run - so roughly **770 tests, about 74%** of the 1036 that pass locally, could not fail the pipeline. Compiling them is exactly what made this invisible: a broken integration test still went green, and even a non-compiling one was caught, so the job looked like it covered them. Three long-lived breakages reached `main` this way (LC-600, and the two below). Dropping `--lib` runs both sets. The marginal cost is modest because the earlier steps already compile these binaries - locally the full suite adds **~100s** wall-clock over `--lib` on a warm cache (18s vs 119s). ## Two blockers had to be fixed first Enabling this without them would just have turned CI red. ### 1. `routes_drafts` draft-indicator tests They matched the literal `&#9998;`. Commit `06913c8` ("draft indicator is an SVG pencil, not a tofu dingbat") replaced that entity and did not update the tests. - `sidebar_shows_draft_pencil_for_room_with_draft` had been **failing on main** ever since. - `sidebar_omits_draft_pencil_when_no_draft` kept **passing while asserting the absence of a string that no longer appears anywhere** - which is the worse failure of the two, because it read as coverage and was not. Both now assert on the badge's structure (populated indicator vs empty OOB-target span) through a small `draft_badge` helper, so the next icon change does not break them. ### 2. The `lc77` fixture check was inert locally I reported this on the ticket as "auto-rewrites on mismatch". That was wrong, and the real cause is worth stating precisely: ```rust if std::env::var("FIXTURE_WRITE").is_ok() { // <-- present-but-empty counts as opt-in ``` `dev/cargo` forwards `-e "FIXTURE_WRITE=${FIXTURE_WRITE:-}"`, which sets the variable **present and empty**. `env::var` returns `Ok("")`, so the write branch fired on *every* local run through the standard wrapper: the golden files were silently regenerated and the test always passed. The assertion only worked under a bare `cargo test` - i.e. nowhere anyone actually ran it, and (until this PR) nowhere in CI either. The predicate now requires a meaningful value, so the default is to assert. Verified both directions: with stale fixtures the tests **fail and leave the files untouched**; with `FIXTURE_WRITE=1` they regenerate. The four fixtures are regenerated here to match current templates. The drift is the LC-553 reaction button and the LC-598 menu grouping - both intentional, neither previously captured. ## Verification - Full suite green; `just check` green. - Fixture gate confirmed in both directions (see above). - Suite passes with `server/assets/tailwind-built.css` **removed**, confirming CI needs no `build-css` step for it - `compute_asset_version` falls back and tests construct `AppState` with `asset_version: "test"`. ## What this does not do I have not seen this job run on the runner. The remaining risk is wall-clock and any runner-specific environment difference; the local evidence above is the best proxy I can produce from here. If the added time is a problem, the natural follow-up is splitting the integration run into its own parallel job rather than reverting to `--lib`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

longjacksonle added 1 commit 2026-07-19 03:44:41 +02:00

ci: run the integration tests, and make the fixture check real ... b242f17951

CI ran `cargo test --lib -p lets-chat-server`, which executes only the 266 in-crate unit tests. The 177 integration binaries under server/tests/ were compiled by the `--all-targets` build and clippy steps but never executed, so roughly 770 tests - about 74% of the 1036 that pass locally - could not fail the pipeline. Compiling them is what made the gap invisible: a broken integration test still went green, and even a non-compiling one was caught, so the job looked like it covered them. Three long-lived breakages reached main this way.

Dropping `--lib` runs both sets. The marginal cost is modest because the earlier steps already compile these binaries; locally the full suite adds ~100s wall-clock over `--lib` on a warm cache.

Two things had to be fixed first, or enabling this would simply have turned CI red.

routes_drafts: the sidebar draft-indicator tests matched the literal `✎`. Commit 06913c8 replaced that entity with an inline SVG pencil and did not update them. The positive test had been failing on main ever since; the negative test kept passing while asserting the absence of a string that no longer appears anywhere, which is worse - it looked like coverage and was not. Both now assert on the badge's structure (populated vs empty target span) via a small helper, which survives the next icon change.

lc77 render fixtures: `assert_fixture_matches` gated regeneration on `env::var("FIXTURE_WRITE").is_ok()`, which treats a variable that is present but empty as opt-in. dev/cargo forwards `-e "FIXTURE_WRITE=${FIXTURE_WRITE:-}"`, so under the standard local wrapper the variable was always present and always empty: every local run silently rewrote the golden files and passed, and the check only did its job under a bare `cargo test`, which is to say nowhere anyone ran it. The predicate now requires a meaningful value, so the default is to assert. The four fixtures are regenerated in this commit to match current templates - the drift is the LC-553 reaction button and the LC-598 menu grouping, both intentional.

Verified: full suite green, `just check` green, and the suite passes with server/assets/tailwind-built.css removed, confirming CI does not need a build-css step for it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2Lh6DKB15inZTb5z8miu7

longjacksonle commented 2026-07-19 03:55:29 +02:00

Author

Owner

Copy link

CI has now run this on the runner, so the "I have not seen this job run" caveat above is resolved.

Check / clippy + fmt + tests (pull_request) -> success, "Successful in 5m25s", combined status success.

That is the whole 1036-test suite executing on the runner rather than my local proxy for it. 5m25s is the total job wall-clock (fmt, two clippy passes, two builds, then the tests), so the added time is comfortably absorbed - no need for the split-into-a-parallel-job follow-up I suggested unless it grows later.

Worth stating plainly: this run is also the first time the integration tests have ever gated a PR in this repo.

**CI has now run this on the runner, so the "I have not seen this job run" caveat above is resolved.** `Check / clippy + fmt + tests (pull_request)` -> **success**, *"Successful in 5m25s"*, combined status **success**. That is the whole 1036-test suite executing on the runner rather than my local proxy for it. 5m25s is the total job wall-clock (fmt, two clippy passes, two builds, then the tests), so the added time is comfortably absorbed - no need for the split-into-a-parallel-job follow-up I suggested unless it grows later. Worth stating plainly: this run is also the first time the integration tests have ever gated a PR in this repo.

longjacksonle merged commit c4c649383b into main 2026-07-19 03:56:40 +02:00

longjacksonle deleted branch fix/lc605-ci-runs-integration-tests 2026-07-19 03:56:41 +02:00

longjacksonle referenced this pull request from a commit 2026-07-19 03:56:43 +02:00

Merge pull request 'Run the integration tests in CI, and make the fixture check real (LC-605)' (#561) from fix/lc605-ci-runs-integration-tests into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/lets-chat!561

No description provided.