Refactor iModel.test.ts to avoid tests leaking into each other

[MichaelSwigerAtBentley]

Refactors the monolith iModel test file into 8 separate files:

• IModelECSql  (7)
• IModelElements  (18)
• IModelGeolocation  (8)
• IModelLifecycle  (15)
• IModelMetadata  (10)
• IModelModels  (9)
• IModelRelationships  (8)
• IModelViews  (5)
• IModelTestFixtures.ts  (shared helpers)

Previously, iModel test reused 5 opened-once imodels accross all of the tests, leading to cases where write tests could bleed into other tests causing failures if tests were ran in isolation. This refactors the test file such that:
• Read-only tests share one per-file read-only snapshot that is only opened once
• write tests each get a fresh writable copy

Tests were not changed, just the before and after, and the file structure. A pass should probably be done on the tests as well, but for now I will save that for another PR.

Separating the tests like this only leads to a ~5% performance loss (extra time to copy the baselines), in my testing test times went from avg 20s to 21s

[Copilot]

Pull request overview

This PR refactors the previously monolithic iModel backend unit tests into multiple focused test files and introduces shared fixture helpers to reduce test-state leakage between read/write tests. The refactor aims to ensure read-only tests share stable read-only snapshots while write tests operate on fresh writable copies.

Changes:

• Split iModel tests into dedicated suites (ECSql, Elements, Geolocation, Lifecycle, Metadata, Models, Relationships, Views).
• Added shared test-fixture helpers for creating isolated writable snapshots and common error/assert utilities.
• Updated per-suite setup/teardown patterns to close mutable iModels after each test and keep shared read-only snapshots open once per file.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
core/backend/src/test/imodel/IModelECSql.test.ts	ECSQL/SQL tests moved into their own suite with shared read-only fixtures and per-test writable snapshots where needed.
core/backend/src/test/imodel/IModelElements.test.ts	Element CRUD/behavior tests isolated via per-test writable snapshots and shared read-only snapshots.
core/backend/src/test/imodel/IModelGeolocation.test.ts	Geolocation tests isolated and retain backend restart behavior for GCS workspace loading.
core/backend/src/test/imodel/IModelLifecycle.test.ts	Lifecycle tests moved into dedicated suite using shared fixture helpers for error assertions.
core/backend/src/test/imodel/IModelMetadata.test.ts	Metadata/schema tests split out and updated to use shared fixture helpers and snapshot lifecycle management.
core/backend/src/test/imodel/IModelModels.test.ts	Model tests split out and updated to use shared fixture helpers and snapshot lifecycle management.
core/backend/src/test/imodel/IModelRelationships.test.ts	Relationship tests split out; writable-seed tests use per-test generated snapshots for isolation.
core/backend/src/test/imodel/IModelViews.test.ts	View tests split out; write tests use fresh writable copies while read-only tests share a read-only snapshot.
core/backend/src/test/imodel/IModelTestFixtures.ts	New shared helper module for snapshot creation, error assertion helpers, and JSON roundtrip helper.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

[hl662]

seems like you need a rush change, its refactoring tests so you can leave an empty changelog message.

( You can resolve my comments yourself when you're done)

[MichaelSwigerAtBentley]

Code Review: #9543 — Refactor iModel.test.ts to avoid tests leaking into each other

TL;DR / Verdict

Request changes (minor scope). The direction is right and overdue: a 3,728-line monolith with 5 process-wide shared iModels becomes 8 focused files with per-test isolation, and all 79 tests survive (verified by title + body diff). But the execution leaves a 14-line fixture block copy-pasted into 5 files, ships a dead exported helper, and the PR description ("Tests were not changed") is false — at least three test bodies changed semantics, one of which is an undisclosed assertion fix and one of which makes a "random" test not random.

---

Architecture

Pros

• Kills the shared-mutable-fixture anti-pattern. Read-only tests share a snapshot; mutating tests get a fresh copy. This is the correct model.
• Deletes a 3,728-line file; every new file is ≤1,083 lines. Real maintainability win.
• Split boundaries (Elements / Models / Views / ECSql / Metadata / Relationships / Geolocation / Lifecycle) are cohesive and map to real domains.
• Output filenames are namespaced per file (elements-, metadata-, …) — I checked for cross-file collisions and found none.
• GCS backend restart is correctly localized to IModelGeolocation.test.ts:26-27 / 43-44 instead of applying to the whole suite.

Cons

• [High] The read-only fixture boilerplate is duplicated verbatim 5×. IModelECSql.test.ts:19-40, IModelElements.test.ts:39-63, IModelMetadata.test.ts:22-42, IModelModels.test.ts:26-46, IModelViews.test.ts:22-38 all contain the same create-writable → capture pathName → close() → SnapshotDb.openFile() dance plus the same guarded after teardown. This is exactly the canonical-helper duplication the fixtures file was created to prevent. Collapse into one helper, e.g.:

  export async function openReadonlySeedCopy(target: string, seed: string, opts?: { importTestBim?: boolean }): Promise<SnapshotDb>

  and a closeIfOpen(...dbs) teardown. That deletes ~60 lines and makes the isolation policy live in one place instead of five.

• [High] trackMutableIModel + mutableIModels is duplicated 4× — IModelElements.test.ts:37,72, IModelMetadata.test.ts:20,51, IModelModels.test.ts:24,59, IModelViews.test.ts:20,48. This is the single most important mechanism in the whole PR (it's what actually prevents leaks) and it lives in four places. It belongs in IModelTestFixtures.ts as a small factory: const { track, closeTracked } = createIModelTracker();.

• [Medium] openSeedReadonly (IModelTestFixtures.ts:64-66) is exported and never called. Dead on arrival. Either use it — IModelGeolocation.test.ts:31-34 copies mirukuru.ibim only to reopen it read-only, which is what this helper is for — or delete it.

• [Medium] generateTestSnapshot and createIModelFromSeed are the same function. IModelTestFixtures.ts:44-52 vs 56-61 differ only by the TestBim schema import. One function with { importTestBim?: boolean } (or an explicit importTestBim(db)) removes the near-duplicate pair and the naming confusion — nothing about "generate" vs "createFrom" tells a reader which one imports the schema.

• [Low] IModelElements.test.ts is still 1,083 lines. It's the only file left over 1k. codeValue behaviors (959-1080) and the auto-handled-array tests are natural seams if you want to finish the job.

• [Nit] Inconsistent teardown. IModelMetadata.test.ts:37-42 closes only the read-only fixtures in after, while Elements/Models/Views also drain mutableIModels there. Since afterEach already drains, the after drain in the other three is dead code. Pick one and apply it uniformly — a shared helper makes this moot.

---

Security

No issues found. This is test-only code: no untrusted input, no new deps, no secrets. process.env is saved/restored in IModelGeolocation.test.ts:24,38 and IModelLifecycle.test.ts:26,36, which is carried over from the original.

---

TypeScript

• [Medium] let originalEnv: any; — IModelGeolocation.test.ts:21. The sibling file gets this right with NodeJS.ProcessEnv (IModelLifecycle.test.ts:26). Fix the any.
• [Low] Type lies in the fixture declarations. let testBimReadonly: SnapshotDb; (IModelECSql.test.ts:16, and the equivalent in Elements/Metadata/Models/Views) is declared non-nullable, then after guards with testBimReadonly !== undefined. If it can be undefined when before throws — and it can — declare it SnapshotDb | undefined. Otherwise the guard is unreachable per the type system and readers can't tell which is true.
• [Nit] after(async () => { ... }) in IModelECSql.test.ts:34, IModelElements.test.ts:54, IModelMetadata.test.ts:37, IModelModels.test.ts:41, IModelViews.test.ts:32 contains no await. Drop async.
• IModelTestFixtures.ts:34 new (entity1.constructor as any)(...) and :27 error! are carried over verbatim from the original — not introduced here, so not blocking, but this is the moment to type it properly since it's now a shared exported helper rather than a local.

---

Correctness — undisclosed behavior changes

The description says "Tests were not changed, just the before and after, and the file structure." I diffed all 79 bodies with mechanical renames normalized. Most changes are indeed pure rebinding, but these are not:

1. [Blocker — description, not code] deleteInstances for random relationship instances (IModelRelationships.test.ts). Changed from

   for (let i = 0; i < 250; ++i) relationshipsToDelete.push(relationships[Math.floor(Math.random() * relationships.length)]);

   to a deterministic i += 4 stride with assert.equal(relationshipsToDelete.length, 250). Determinism is a genuine improvement — but the old version could push the same relationship twice, so the test previously exercised duplicate-delete behavior and now does not. Two asks: call this out in the PR description, and rename the test — it is no longer "random".

2. [Medium] should insert and update auto-handled properties — IModelElements.test.ts:560. Original asserted afterUpdateElemFetched.asAny.arrayOfInt.length === 2 on the object it had just mutated in memory (vacuous); new code asserts on the re-fetched afterShortenArray. This is a real bug fix in the test and strictly better — but it is a semantic change hidden inside a "pure refactor" PR. Mention it.

3. [Low] Test-name typo fixes — "an iIModel" → "an iModel", "CRUD opertion" → "CRUD operation", "relationsip" → "relationship". Fine, just not "unchanged".

4. [Low, positive] Added try/finally cleanup in attempting to re-attach a non-checkpoint snapshot should be a no-op (IModelLifecycle.test.ts:194-198) and Run plain SQL against readonly connection (IModelECSql.test.ts:275-314). Good — these previously leaked an open db on failure. Several other tests still lack it (e.g. IModelRelationships.test.ts:100-166, 171-202, 211-355; IModelGeolocation.test.ts:117-170), which is pre-existing but is the obvious follow-up.

One thing to verify before merge: the original ran the entire suite under startBackend({ loadGcsWorkspaces: true }) (IModel.test.ts:83-84 on base). Now only IModelGeolocation does; every other file gets startBackend() → disableGcsWorkspaces(). IModelHost.startup early-returns when already valid (IModelHost.ts:636), so the config from the first caller wins while disableGcsWorkspaces() still runs unconditionally. That means suites like IModelLifecycle (Snapshot iModel properties, which asserts on geographicCoordinateSystem) now run with GCS workspaces suppressed where they previously did not. Confirm those assertions don't depend on GCS data, and confirm the file-ordering assumption holds (Geolocation's before calls shutdownBackend(), which closes every open IModelDb — safe only because each file's after closes its own fixtures first).

---

Other / Non-TS Files

None. No package.json, mocha config, or CI changes — correct, since the new files live in the same core/backend/src/test/imodel/ directory and the existing glob picks them up. Confirm whether CI requires a change file even for test-only PRs.

---

Blocking Issues

1. Fix the PR description — "Tests were not changed" is inaccurate; call out the deleteInstances sampling change and the afterShortenArray assertion fix explicitly so a reviewer doesn't have to diff 79 bodies to find them.
2. Rename deleteInstances for random relationship instances — it is deterministic now.
3. Hoist trackMutableIModel / mutableIModels into IModelTestFixtures.ts (duplicated 4×).
4. Hoist the read-only-fixture setup/teardown block into IModelTestFixtures.ts (duplicated 5×).
5. Delete or use openSeedReadonly.

Nits

• any → NodeJS.ProcessEnv at IModelGeolocation.test.ts:21.
• Drop the no-op async on five after hooks.
• Merge generateTestSnapshot / createIModelFromSeed behind one option.
• Make the after teardown consistent across the four tracker files.
• IModelElements.test.ts at 1,083 lines could take one more split.
• IModelLifecycle.test.ts:527 adds a startBackend() to a nested describe that didn't have one — harmless no-op, but unnecessary.

Pros & Cons Summary

Pros	Cons
Eliminates cross-test state leakage — the actual point of the PR, and it works	Fixture boilerplate copy-pasted into 5 files; tracker into 4
3,728-line monolith → 8 cohesive files, all ≤1,083 lines	openSeedReadonly exported but never used
All 79 tests preserved; verified body-by-body	PR description claims no test changes; 3 bodies changed semantics
Only ~5% runtime cost for real isolation	deleteInstances test name no longer matches behavior
GCS backend restart correctly scoped to geolocation	GCS now disabled for suites that previously had it — needs verification
Adds try/finally cleanup in two leak-prone tests	Many other tests still leak open dbs on failure
No output-filename collisions across the split	any and non-nullable-but-undefined-checked fixture declarations

---

Mecha-Mike using Opus 5

[aruniverse]

Could we keep this deterministic without removing duplicate-input coverage? The old sampling was with replacement, so it almost always passed repeated relationship entries in the same deleteRelationships call. That exercised the batch/native boundary with duplicate IDs; the new unique stride removes the only backend coverage I could find for that case.

Smallest fix: keep the deterministic subset, but intentionally replace one selected entry with a duplicate (or add a focused duplicate-input test). The test name should also drop “random” once the selection is deterministic.