Skip to content

Test suite + transaction/parameter correctness fixes - #283

Merged
thelindat merged 9 commits into
overextended:mainfrom
Kenshiin13:fix/transaction-safety-and-test-suite
Jun 7, 2026
Merged

Test suite + transaction/parameter correctness fixes#283
thelindat merged 9 commits into
overextended:mainfrom
Kenshiin13:fix/transaction-safety-and-test-suite

Conversation

@Kenshiin13

Copy link
Copy Markdown
Member

Summary

Adds a unit test suite (bun) with an enforced coverage gate and CI, and fixes a set of correctness bugs surfaced while writing the tests. Every fix lands with a regression test that fails against the old code and passes against the new.


Why

oxmysql had no automated tests. While building one, several latent bugs turned up in the query/transaction paths. Including one that could report a transaction as committed when the commit actually failed. This PR fixes those and locks them in with tests, then gates the suite at 85% per-file line coverage in CI.


Bug fixes

Each is its own commit with a matching regression test.

Severity Fix File
Blocker startTransaction reported success before the commit landed and swallowed commit failures. The disposer fired an unawaited commit() and returned the connection to the pool mid-statement. Now disposal is async (Symbol.asyncDispose), startTransaction awaits the commit explicitly, and a failed commit returns false instead of true. connection.ts, startTransaction.ts, rawQuery.ts, rawExecute.ts, rawTransaction.ts
Major A connection borrowed by id (getConnection(connectionId)) was returned as the shared instance, so the borrow site's await using would roll back and release a connection its owner still held. Now reused connections get a non-owning handle whose disposer is a no-op. connection.ts
Major lib store() was unusable: the assertion was inverted (threw on the correct input) and the returned reference was off-by-one. lib/MySQL.ts
Minor startTransaction's 30s timeout timer was never cleared, leaking one timer per transaction. startTransaction.ts
Minor parseArguments padded missing parameters at the wrong indices. parseArguments.ts
Minor transactionError built its message but never returned it (missing return), so transaction-error logs/events lost the query detail. rawTransaction.ts

A small, behavior-preserving refactor (config.ts) extracts getIsolationLevelStatement and lets getConnectionOptions take an optional connection string, so both can be unit-tested; production callers are unchanged. One dead import was dropped (pool.ts).

Behavioral / compatibility note

The transaction-disposal fix moves the connection wrapper from a synchronous Symbol.dispose to an async Symbol.asyncDispose, and the call sites from using to await using. This is required. Committing/rolling back is async and a sync disposer cannot await it. esbuild lowers await using for the node22 target; a transaction smoke test was run against a live FXServer to confirm it works in the built bundle (queries return, commits persist, failed transactions roll back, and 30 concurrent queries against a 10-connection pool all complete, proving connections are released).


Tests

  • Runner: bun test — built in, native TS/ESM, Jest-compatible API, built-in coverage. No new runtime deps.
  • 137 unit tests across the parsing/coercion utils, config, the lib wrapper, the full DB layer, logger, and profiler.
  • mysql2/promise is mocked at preload (tests/setup.tstests/helpers/db.ts) with a fake pool/connection that records the op sequence (begin/query/commit/rollback/release). This makes the DB layer deterministic with no database and lets the transaction-lifecycle bugs be asserted directly. FXServer natives (GetConvar, RegisterCommand, onNet, …) are stubbed in the same preload.
  • Real-DB test preserved — the previous benchmark moved to tests/integration/, skipped unless DB_CONNECTION is set, and excluded from the coverage gate.

Coverage gate

coverageThreshold = 0.85 in bunfig.toml (bun's built-in, enforced per file). tests/ and the untestable bootstrap database/index.ts (a retry loop with a 30s sleep) are excluded via coveragePathIgnorePatterns. Every measured file is ≥85% line coverage.

A second tsconfig.json (root)

bun resolves TS path aliases from the root tsconfig.json, and src uses bare specifiers (config, utils/*, database). The repo previously had only src/tsconfig.json (used by the esbuild build). The new root config extends it and adds baseUrl/paths so bun can resolve those imports; it does not affect the production build.

CI

.github/workflows/test.yml runs bun install --frozen-lockfile && bun run test:coverage on push to main and on PRs.


How to run

bun test                 # unit suite
bun run test:coverage    # unit suite + 85% per-file gate (what CI runs)

# real-DB integration (opt-in)
DB_CONNECTION=mysql://root@localhost bun run test:integration

@thelindat

Copy link
Copy Markdown
Member

There's a bit of a smell in here, but we can just manually clean it up later so whatever.

@thelindat
thelindat merged commit c216a12 into overextended:main Jun 7, 2026
1 check passed
@Kenshiin13
Kenshiin13 deleted the fix/transaction-safety-and-test-suite branch June 7, 2026 11:21
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.

2 participants