Stop test realm setup importing card-api when it has nothing to prepare - #6016
Open
backspace wants to merge 3 commits into
Open
Stop test realm setup importing card-api when it has nothing to prepare#6016backspace wants to merge 3 commits into
backspace wants to merge 3 commits into
Conversation
`TestRealmAdapter.prepareInstances` imports `@cardstack/base/card-api` before looping over the objects it needs to mark saved or shim, but the loop is what uses it. The loader it imports through is constructed per test realm, so its module cache is empty and the import evaluates card-api and its whole graph. Profiled across a host shard: `adapter.ready`, which that import gates, is 59s of the 223s that per-test realm setup costs — 26.5% — at a ~280ms median, and it costs the same whether the realm has contents or none. Forty-four of 220 setups pass no contents at all and prepare nothing; realms given their contents as source strings likewise populate nothing. Guarding the import on there being work to do leaves behaviour unchanged. Also threads the `skipBootIndex` option Realm already accepts through `setupIntegrationTestRealm` and `setupTestRealm`. A realm's boot index costs ~676ms whatever it has to index — 146 of those 220 setups pay it to index a median of one file — so a test that never reads the search index need not. It is opt-in and off by default, because a test that queries, or reads a card by id through the store, needs the index and fails without it. Two modules take it as a pilot: `computed-test` and `enum-field-test` construct cards in memory and render them, and reference no search API. Twelve calls, which is too few to show against the suite's ±106s run-to-run variance — the point is to establish that lazy definition resolution carries these tests before the option is applied anywhere it matters.
Contributor
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 1h 56m 52s ⏱️ - 17m 8s Results for commit d3bfd91. ± Comparison against earlier commit 76639fd. Realm Server Test Results 1 files ±0 190 suites ±0 1h 8m 14s ⏱️ + 2m 39s Results for commit d3bfd91. ± Comparison against earlier commit 76639fd. |
The pilot answered its question: `computed-test`'s 15 tests pass with the boot index skipped, so definitions do resolve lazily through the prerenderer for tests that build cards and render them. Serialization is where the calls concentrate — 109 setups in one module, more than twice the next — and almost none of them need an index: the tests construct a card, serialize or deserialize it, and assert on the payload. Three tests do read the index, around the query-backed relationship cases, and those keep their boot index; the conversion partitions the file into test blocks and skips any block referencing a search API. 106 converted, 3 skipped. If any of the 106 turns out to need the index it fails loudly and names itself, which is a cheaper audit than reading 98 tests.
backspace
marked this pull request as ready for review
September 4, 2026 14:30
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 76639fdc0f
ℹ️ 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".
`Realm` takes its behavioural options in a second constructor argument. The
option was spread into the first one, which destructures a fixed set of fields
and never names `skipBootIndex`, so every caller that asked to skip the boot
index still ran it.
TypeScript did not catch this: object spreads bypass excess-property checking,
so `{ ...(cond ? { skipBootIndex } : {}) }` inside the first argument is well
typed and silently discarded.
This means the option had no effect at all in its first two commits here, and
any timing attributed to it needs remeasuring from this commit forward.
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
1 participant
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.
TestRealmAdapter.prepareInstancesimports@cardstack/base/card-apibefore looping over the objects it needs to mark saved or shim, but the loop is what uses it. The loader it imports through is constructed per test realm, so its module cache is empty and the import evaluates card-api and its whole graph.Profiled across a host shard:
adapter.ready, which that import gates, is 59s of the 223s that per-test realm setup costs — 26.5% — at a ~280ms median, and it costs the same whether the realm has contents or none. Forty-four of 220 setups pass no contents at all and prepare nothing; realms given their contents as source strings likewise populate nothing. Guarding the import on there being work to do leaves behaviour unchanged.Also threads the
skipBootIndexoption Realm already accepts throughsetupIntegrationTestRealmandsetupTestRealm. A realm's boot index costs ~676ms whatever it has to index — 146 of those 220 setups pay it to index a median of one file — so a test that never reads the search index need not. It is opt-in and off by default, because a test that queries, or reads a card by id through the store, needs the index and fails without it.118 call sites take it, across
serialization-test,computed-testandenum-field-test: they construct cards in memory and render or serialize them, and reference no search API. The suite's remaining setups concentrate inrealm-test,realm-indexing-testandrich-markdown-field-test, which all read the index and so are ineligible on merit.What it is worth
Summed shard time cannot measure this. Two runs on this branch that differed only by a change with no effect still differed by 612s, so anything below that is unresolvable at that grain.
Normalizing within a single run does work: the merged junit report carries per-test durations, so the untouched tests in the same run calibrate that run's speed.
The unconverted tests ran 18.6% slower, so the converted modules should have taken 251.7s. They took 165.2s — −86.5s, −34.4%, about −555ms per test net of the runner penalty, consistent with skipping a boot index measured at a ~676ms median.
That is roughly 1% of total suite time, because only 118 of ~220 setups per shard are eligible and the eligible ones are the cheap ones. Small, but contained and verified: all 16 shards pass with the option active.