Skip to content

Fix: declare @mikro-orm/core in the build_setup db fixture manifests (stacked on #383) - #489

Open
AmaadMartin wants to merge 1 commit into
fix/build-setup-cjs-fixtures-types-nodefrom
fix/build-setup-fixtures-declare-mikro-orm-core
Open

Fix: declare @mikro-orm/core in the build_setup db fixture manifests (stacked on #383)#489
AmaadMartin wants to merge 1 commit into
fix/build-setup-cjs-fixtures-types-nodefrom
fix/build-setup-fixtures-declare-mikro-orm-core

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A — no public issue is tracking this.
  2. Or, if no issue exists, describe the change:
    Problem: Four tests/integration/build_setup fixtures import @mikro-orm/core with a bare specifier from their db_init_check script but never declare it in their own package.json:
Fixture Importing file Form
js_commonjs db_init_check.js:7 const {MikroORM} = require('@mikro-orm/core');
js_esm db_init_check.js:8 import {MikroORM} from '@mikro-orm/core';
ts_commonjs db_init_check.ts:8 const {MikroORM} = require('@mikro-orm/core');
ts_esm db_init_check.ts:8 import {MikroORM} from '@mikro-orm/core';

The specifier resolves today only via transitive hoisting: @google/adk is a file:../../../../core dependency, and core/package.json declares "@mikro-orm/core": "^6.6.10" in its runtime dependencies, so npm install inside a fixture hoists that transitive package into the fixture's own node_modules root, where the bare specifier happens to find it. A fixture exists to prove a freshly-installed consumer project works, so it must not depend on an implementation detail of @google/adk's dependency tree.

Solution: Declare @mikro-orm/core in dependencies of the four db fixtures, so the package is a first-class dependency edge of each fixture.

  • Range ^6.6.10, copied verbatim from core/package.json:49. The invariant that matters is one instance per fixture tree: db_init_check monkey-patches MikroORM.init on the module object it imported, so if npm nested a second copy for the consumer, the patch and the consumer would disagree and DatabaseSessionService.init() would try to open a real database. Using the identical range guarantees npm dedupes to a single physical copy (verified below).
  • dependencies, not devDependencies, because the import executes at runtime under npm run test:db, which is what the harness spawns (build_setup_test.ts:70).
  • Exactly four manifests. ts_commonjs_native_addon and ts_esm_native_addon have no db_init_check and the db test is skipIf-skipped for them, so they are untouched. No source file, no test file, and no published manifest (core/, dev/, integrations/) changes; the root package-lock.json is unaffected because the build_setup fixtures are not npm workspaces (the root declares only core, dev, integrations), and fixture lockfiles are gitignored via .gitignore:8.

Precedent for the shape: ts_commonjs_native_addon/package.json and ts_esm_native_addon/package.json already declare a non-@google package (onnxruntime-node) as a further entry in dependencies, in alphabetical order. Same here — @mikro-orm/core sorts after @google/genai.

Collision check (required by the dev workflow): gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 plus gh pr diff --name-only on every plausibly adjacent PR. No open PR declares @mikro-orm/core in these manifests. Three overlaps found:

Durability note: the range now exists in five places (core/package.json plus the four fixtures). If core bumps @mikro-orm/core to a new major, the fixtures must be bumped with it or npm install will resolve two copies. A systematic manifest-completeness guard is the right answer to that and is deliberately not in this PR (#450 is the in-flight attempt).

Scope deliberately not reduced: this is the whole change. No executable code changed, so coverage is unaffected — there is no new line or branch to cover, and no unit test is added, because a test asserting the literal contents of a package.json would be a worse, hardcoded copy of the manifest-completeness checker that is already queued separately.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, and stated deliberately: this change adds zero executable lines (four JSON manifest entries). No new code to cover; coverage thresholds unchanged.
[x] All unit tests pass locally. — the targeted suite for these fixtures is the integration suite below; no unit test file is affected by this change.

Integration suite (build_setup_test.ts), before and after, identical split:

npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
Run Result
Pre-change manifests (base = #383 head) Test Files 1 passed (1) / Tests 20 passed | 4 skipped (24) / 539.68s
With this change Test Files 1 passed (1) / Tests 20 passed | 4 skipped (24) / 577.58s

The four skips are the two skipIf-gated tests on the two native-addon fixtures, exactly as before.

One environment caveat, reported honestly. As configured on this branch, the integration project sets no hookTimeout, so beforeAll's per-fixture npm install (~60s cold here) is killed by vitest's 10s default and every suite fails in the hook — on the unmodified base as well as with this change:

Failed Suites 6 ... Error: Hook timed out in 10000ms.  ❯ build_setup_test.ts:28:5 (await execAsync('npm install', ...))
Tests  24 skipped (24)

That is pre-existing (it is what #117 / #257 are about) and unrelated to this change. Both runs in the table above were therefore taken with hookTimeout: 900000 set locally and temporarily in vitest.config.ts; it is reverted and is not part of this diff (git diff shows only the four manifests). A second artefact of that timeout is worth noting for anyone reproducing: an install killed mid-flight leaves an empty node_modules/@google/adk, after which npm run test:db fails with ERR_MODULE_NOT_FOUND for @google/adk — recover with rm -rf node_modules package-lock.json && npm install, not a plain re-install.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

From the repo root, npm install && npm run build first (the fixtures depend on core/dev via file:), then per fixture:

cd tests/integration/build_setup/<js_commonjs|js_esm|ts_commonjs|ts_esm>
npm install
npm ls --depth=0
npm ls @mikro-orm/core
npm run build        # ts_commonjs and ts_esm only
npm run test:db
rm -rf node_modules package-lock.json dist

Real output. Before (js_esm, pre-change manifest) — @mikro-orm/core absent from the direct edges:

$ npm ls --depth=0
js_esm_build_setup@1.0.0 .../tests/integration/build_setup/js_esm
├── @google/adk-devtools@1.4.0
├── @google/adk@1.4.0
└── @google/genai@2.15.0

After (same fixture, with this change):

$ npm ls --depth=0
js_esm_build_setup@1.0.0 .../tests/integration/build_setup/js_esm
├── @google/adk-devtools@1.4.0
├── @google/adk@1.4.0
├── @google/genai@2.15.0
└── @mikro-orm/core@6.6.16

$ npm ls @mikro-orm/core        # one resolved copy, every other edge "deduped"
└── @mikro-orm/core@6.6.16      # (8 further mentions, all marked deduped)

$ find node_modules -type d -path '*@mikro-orm/core'
node_modules/@mikro-orm/core    # exactly one physical copy

$ npm run test:db
DYNAMIC_IMPORT_SUCCESS

All four fixtures were run this way; each lists @mikro-orm/core@6.6.16 at --depth=0, has exactly one physical copy, and prints DYNAMIC_IMPORT_SUCCESS. ts_commonjs and ts_esm additionally print Build complete with empty stderr.

Negative control — the test can fail. There is no new assertion to mutate, so the latent gap is demonstrated directly. Two findings, both reported as observed rather than as predicted:

  1. The documented cheap control does not fail here, and it is worth knowing why. Deleting only the fixture's copy (rm -rf node_modules/@mikro-orm && node ./db_init_check.js) still prints DYNAMIC_IMPORT_SUCCESS, because Node's upward directory walk finds the repo-root install: require.resolve('@mikro-orm/core')<repo>/node_modules/@mikro-orm/core/index.js. Inside this monorepo the fixture is shadowed by an ancestor node_modules, which is exactly the kind of accidental resolution a declared dependency stops relying on.
  2. With every resolvable copy removed, the unfixed fixture fails hard — fixture copy deleted and the repo-root node_modules/@mikro-orm moved aside (restored immediately afterwards):
$ node ./db_init_check.js
Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@mikro-orm/core' imported from
  .../tests/integration/build_setup/js_esm/db_init_check.js
    at Object.getPackageJSONURL (node:internal/modules/package_json_reader:314:9)

Note the failure mode is a hard load-time ERR_MODULE_NOT_FOUND, not the DYNAMIC_IMPORT_FAILED branch — the import is top-level, so it throws before testInit()'s try/catch ever runs, and the harness assertion expect(response).toContain('DYNAMIC_IMPORT_SUCCESS') fails against empty stdout. This proves the script genuinely requires the package at fixture level; the fix makes that requirement explicit instead of inherited.

For completeness on the failure mode this guards against: the transitive supply is currently two-deep — core declares @mikro-orm/core, and @google/adk-devtools declares five @mikro-orm/* drivers that each depend on it — so core alone dropping the dependency would not break the fixtures today. The claim this PR makes is the narrower, verified one: the fixtures must not depend on either package's dependency tree, and after this change npm install records the edge directly regardless of what core and dev declare.

Local validation of the pushed commit (this PR is stacked, so the fork's pull_request: branches: [main] workflow does not trigger and no test job runs):

  • npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts → 20 passed, 4 skipped (with the local hook-timeout override described above)
  • npm run build → exit 0
  • npm run lint → exit 0
  • npx prettier --check tests/integration/build_setup/*/package.json → "All matched files use Prettier code style!"

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas. — N/A; JSON manifests carry no comments.
[x] I have added tests that prove my fix is effective or that my feature works. — see the negative control above; no new test file is added, and build_setup_test.ts is deliberately left byte-identical.
[x] New and existing unit tests pass locally with my changes.

The four db_init_check scripts import @mikro-orm/core with a bare
specifier, but none of the four fixture manifests declared it. The import
resolved only because @google/adk is a file: dependency on core/, whose
own dependencies pull @mikro-orm/core into the fixture's node_modules
root by hoisting. If core ever drops or moves that dependency, the
fixtures fail with MODULE_NOT_FOUND pointing at the wrong package.

Declare it in dependencies (the import executes at runtime under
npm run test:db) with the range copied verbatim from core/package.json so
npm dedupes to a single instance -- db_init_check patches MikroORM.init on
the module object it imported, so a second nested copy would make the
patch and the consumer disagree.
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