Skip to content

feat: improved ueberdb speed and esm compatibility#952

Closed
SamTV12345 wants to merge 3 commits into
mainfrom
feature/node-testrunner
Closed

feat: improved ueberdb speed and esm compatibility#952
SamTV12345 wants to merge 3 commits into
mainfrom
feature/node-testrunner

Conversation

@SamTV12345
Copy link
Copy Markdown
Member

No description provided.

@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Migrate test suite from Vitest to Node.js native test runner

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Migrate test suite from Vitest to Node.js native test runner
• Replace Vitest assertions with Node.js assert module
• Update test hooks from Vitest to Node.js equivalents (beforeAll→before, afterAll→after)
• Remove Vitest dependency and update test command in package.json
• Use fileURLToPath for __filename compatibility in ESM tests
Diagram
flowchart LR
  A["Vitest Test Framework"] -->|Replace| B["Node.js Native Test Runner"]
  C["Vitest Assertions"] -->|Replace| D["Node.js assert/strict Module"]
  E["Vitest Hooks<br/>beforeAll/afterAll"] -->|Replace| F["Node.js Hooks<br/>before/after"]
  G["__filename in ESM"] -->|Replace| H["fileURLToPath import.meta.url"]
  I["package.json vitest script"] -->|Remove| J["Use node --test command"]
Loading

Grey Divider

File Changes

1. test/lib/test_lib.ts 🧪 Tests +52/-58

Migrate from Vitest to Node.js test runner

test/lib/test_lib.ts


2. test/rusty/test_rusty.spec.ts 🧪 Tests +31/-31

Replace Vitest with Node.js test APIs

test/rusty/test_rusty.spec.ts


3. test/elasticsearch/test.elasticsearch.spec.ts 🧪 Tests +7/-6

Update test hooks and imports for Node.js

test/elasticsearch/test.elasticsearch.spec.ts


View more (25)
4. test/mysql/test_mysql.spec.ts 🧪 Tests +9/-8

Convert Vitest to Node.js test framework

test/mysql/test_mysql.spec.ts


5. test/postgres/test.postgresql.spec.ts 🧪 Tests +10/-10

Migrate test suite to Node.js native runner

test/postgres/test.postgresql.spec.ts


6. test/mock/test_metrics.spec.ts 🧪 Tests +6/-6

Replace Vitest assertions and hooks

test/mock/test_metrics.spec.ts


7. test/surrealdb/test.surrealdb.spec.ts 🧪 Tests +5/-5

Update to Node.js test framework

test/surrealdb/test.surrealdb.spec.ts


8. test/couch/test.couch.spec.ts 🧪 Tests +5/-5

Convert Vitest hooks to Node.js equivalents

test/couch/test.couch.spec.ts


9. test/mock/test_bulk.spec.ts 🧪 Tests +5/-3

Replace Vitest with Node.js test APIs

test/mock/test_bulk.spec.ts


10. test/cassandra/test.cassandra.spec.ts 🧪 Tests +4/-4

Update test framework to Node.js native

test/cassandra/test.cassandra.spec.ts


11. test/rethinkdb/rethinkdb.spec.ts 🧪 Tests +5/-5

Migrate to Node.js test runner

test/rethinkdb/rethinkdb.spec.ts


12. test/mysql/test.mysql.spec.ts 🧪 Tests +6/-6

Convert Vitest to Node.js test framework

test/mysql/test.mysql.spec.ts


13. test/mongodb/test.spec.ts 🧪 Tests +11/-8

Update test structure for Node.js runner

test/mongodb/test.spec.ts


14. test/mock/test_flush.spec.ts 🧪 Tests +3/-2

Replace Vitest with Node.js test APIs

test/mock/test_flush.spec.ts


15. test/redis/test.redis.spec.ts 🧪 Tests +4/-4

Migrate to Node.js native test runner

test/redis/test.redis.spec.ts


16. test/memory/test_tojson.spec.ts 🧪 Tests +5/-4

Update test hooks and assertions

test/memory/test_tojson.spec.ts


17. test/mock/test_findKeys.spec.ts 🧪 Tests +3/-3

Convert Vitest to Node.js test framework

test/mock/test_findKeys.spec.ts


18. test/mock/test_lru.spec.ts 🧪 Tests +3/-2

Replace Vitest imports with Node.js test

test/mock/test_lru.spec.ts


19. test/mock/test_setSub.spec.ts 🧪 Tests +3/-2

Migrate to Node.js test runner

test/mock/test_setSub.spec.ts


20. test/memory/test_memory.spec.ts 🧪 Tests +3/-2

Update test imports for Node.js

test/memory/test_memory.spec.ts


21. test/memory/test.memory.spec.ts 🧪 Tests +2/-2

Replace Vitest hooks with Node.js equivalents

test/memory/test.memory.spec.ts


22. test/memory/test_getSub.spec.ts 🧪 Tests +3/-2

Convert to Node.js test framework

test/memory/test_getSub.spec.ts


23. test/rusty/test.rusty.spec.ts 🧪 Tests +1/-1

Remove unused Vitest imports

test/rusty/test.rusty.spec.ts


24. test/dirty/test.dirty.spec.ts 🧪 Tests +1/-1

Update to Node.js test framework

test/dirty/test.dirty.spec.ts


25. test/sqlite/test.sqlite.spec.ts 🧪 Tests +1/-1

Replace Vitest with Node.js test runner

test/sqlite/test.sqlite.spec.ts


26. package.json Dependencies +1/-2

Remove Vitest dependency and update test script

package.json


27. pnpm-lock.yaml Additional files +0/-903

...

pnpm-lock.yaml


28. vitest.config.ts Additional files +0/-17

...

vitest.config.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 10, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0)

Grey Divider


Action required

1. CI test matrix broken 🐞 Bug ☼ Reliability
Description
The new test script does not handle the existing workflow’s pnpm run test <db> invocation, so
the extra argument will be forwarded to node --test as a test file/pattern and can fail or run
unintended tests. This will break the npmpublish.yml test job matrix and can also cause every
matrix shard to run the entire test suite instead of a single DB target.
Code

package.json[R139-143]

    "build": "pnpm run build:js && pnpm run build:types",
    "build:js": "pnpm exec rolldown -c rolldown.config.mjs",
    "build:types": "pnpm exec tsc --emitDeclarationOnly",
-    "test": "vitest --test-timeout=120000",
+    "test": "node --experimental-strip-types --test --test-timeout=120000 test/**/*.spec.ts",
    "ts-check": "tsc --noEmit",
Evidence
package.json hard-codes test/**/*.spec.ts for the node test runner, but the release workflow
passes a per-DB argument (${{ matrix.db }}) to the test script. With npm/pnpm, that argument is
forwarded to the underlying command, so the workflow behavior no longer matches the script’s
expectations (and per-DB sharding is lost).

package.json[134-145]
.github/workflows/npmpublish.yml[11-53]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `test` script no longer supports `pnpm run test <db>` used by the GitHub Actions matrix, so the workflow will forward `<db>` to `node --test` as an extra positional argument (file/pattern), breaking CI and eliminating per-DB sharding.

## Issue Context
The workflow intentionally runs one DB suite per job (`matrix.db`). With Vitest this could be achieved by passing an argument; with Node’s test runner you need to either:
- pass an explicit test file/glob per db, or
- use `--test-name-pattern` to select tests by suite name.

## Fix Focus Areas
- package.json[134-145]
- .github/workflows/npmpublish.yml[11-53]

## Suggested fix approach
- Update the `test` script to accept an optional argument and translate it into a file glob (recommended). For example:
 - `pnpm run test couch` => run `test/couch/**/*.spec.ts`
 - `pnpm run test mysql` => run `test/mysql/**/*.spec.ts`
 - `pnpm run test mongo` => run `test/mongodb/**/*.spec.ts`
- Or update the workflow to call `node --test` with the correct file glob directly instead of passing a db argument.
- Ensure `pnpm run test` (no args) still runs the full suite locally if desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Mock metrics DB not closed 🐞 Bug ☼ Reliability
Description
test/mock/test_metrics.spec.ts creates a new ueberdb.Database in beforeEach() for every test
but only closes db once in an after() hook, leaving earlier instances unclosed. This can keep
open handles (timers/sockets) and lead to hangs or flaky test termination under the Node test
runner.
Code

test/mock/test_metrics.spec.ts[R59-70]

    mock = settings.mock
    mock.once('init', (cb: any) => cb());
  });
-  afterAll(async () => {
+  after(async () => {
    mock.once('close', (cb: any) => cb());
    await db.close();
  });
-  beforeEach(async function (context) {
-    key = expect.getState().currentTestName
+  beforeEach(async (t) => {
+    key = t.name;
  });
  afterEach(async () => {
    mock.removeAllListeners();
Evidence
The suite reassigns db on every beforeEach() (so previous instances become unreachable), but
cleanup only happens once in after(). The repository already has a global after() hook dumping
open handles if Node doesn’t exit, so leaked handles can directly fail the test run.

test/mock/test_metrics.spec.ts[51-71]
test/memory/test.memory.spec.ts[6-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test/mock/test_metrics.spec.ts` initializes a new DB instance in `beforeEach()` but only closes it once in `after()`, so prior per-test DB instances are never closed.

## Issue Context
Node’s test runner (and this repo’s `test/memory/test.memory.spec.ts`) is sensitive to open handles that prevent process exit.

## Fix Focus Areas
- test/mock/test_metrics.spec.ts[51-71]

## Suggested fix approach
Choose one:
1) If each test needs a fresh DB:
- Move `db.close()` (and mock close) into `afterEach()` and remove the `after()` hook.

2) If the DB should be shared across tests:
- Change the DB initialization hook from `beforeEach()` to `before()` and keep `after()` for teardown.
- Keep a separate `beforeEach()` only for per-test state like `key = t.name`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. MySQL suite named postgres 🐞 Bug ⚙ Maintainability
Description
The MySQL integration test suite is labeled 'postgres test', which makes logs and failures
misleading and complicates test selection/debugging. This name was preserved while modifying the
suite to add Node test runner timeout options.
Code

test/mysql/test.mysql.spec.ts[R5-6]

+describe('postgres test', {timeout: 120000}, ()=>{
    const portMappings: PortWithOptionalBinding[] = [
Evidence
The file starts a MariaDB container and calls test_db('mysql'), but the suite name says `postgres
test`, which is incorrect and confusing for reporters and grep-based filtering.

test/mysql/test.mysql.spec.ts[5-23]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The MySQL container test suite is named `postgres test`, which is misleading.

## Issue Context
This affects readability and the usefulness of test output / test name pattern matching.

## Fix Focus Areas
- test/mysql/test.mysql.spec.ts[5-23]

## Suggested fix approach
- Rename the suite string from `'postgres test'` to `'mysql test'` (or similar).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread package.json
Comment on lines 139 to 143
"build": "pnpm run build:js && pnpm run build:types",
"build:js": "pnpm exec rolldown -c rolldown.config.mjs",
"build:types": "pnpm exec tsc --emitDeclarationOnly",
"test": "vitest --test-timeout=120000",
"test": "node --experimental-strip-types --test --test-timeout=120000 test/**/*.spec.ts",
"ts-check": "tsc --noEmit",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Ci test matrix broken 🐞 Bug ☼ Reliability

The new test script does not handle the existing workflow’s pnpm run test <db> invocation, so
the extra argument will be forwarded to node --test as a test file/pattern and can fail or run
unintended tests. This will break the npmpublish.yml test job matrix and can also cause every
matrix shard to run the entire test suite instead of a single DB target.
Agent Prompt
## Issue description
The `test` script no longer supports `pnpm run test <db>` used by the GitHub Actions matrix, so the workflow will forward `<db>` to `node --test` as an extra positional argument (file/pattern), breaking CI and eliminating per-DB sharding.

## Issue Context
The workflow intentionally runs one DB suite per job (`matrix.db`). With Vitest this could be achieved by passing an argument; with Node’s test runner you need to either:
- pass an explicit test file/glob per db, or
- use `--test-name-pattern` to select tests by suite name.

## Fix Focus Areas
- package.json[134-145]
- .github/workflows/npmpublish.yml[11-53]

## Suggested fix approach
- Update the `test` script to accept an optional argument and translate it into a file glob (recommended). For example:
  - `pnpm run test couch` => run `test/couch/**/*.spec.ts`
  - `pnpm run test mysql` => run `test/mysql/**/*.spec.ts`
  - `pnpm run test mongo` => run `test/mongodb/**/*.spec.ts`
- Or update the workflow to call `node --test` with the correct file glob directly instead of passing a db argument.
- Ensure `pnpm run test` (no args) still runs the full suite locally if desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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