Test suite + transaction/parameter correctness fixes - #283
Merged
thelindat merged 9 commits intoJun 7, 2026
Merged
Conversation
…g shared transaction params
Member
|
There's a bit of a smell in here, but we can just manually clean it up later so whatever. |
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
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.
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.
startTransactionreported success before the commit landed and swallowed commit failures. The disposer fired an unawaitedcommit()and returned the connection to the pool mid-statement. Now disposal is async (Symbol.asyncDispose),startTransactionawaits the commit explicitly, and a failed commit returnsfalseinstead oftrue.connection.ts,startTransaction.ts,rawQuery.ts,rawExecute.ts,rawTransaction.tsgetConnection(connectionId)) was returned as the shared instance, so the borrow site'sawait usingwould 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.tslibstore()was unusable: the assertion was inverted (threw on the correct input) and the returned reference was off-by-one.lib/MySQL.tsstartTransaction's 30s timeout timer was never cleared, leaking one timer per transaction.startTransaction.tsparseArgumentspadded missing parameters at the wrong indices.parseArguments.tstransactionErrorbuilt its message but never returned it (missingreturn), so transaction-error logs/events lost the query detail.rawTransaction.tsA small, behavior-preserving refactor (
config.ts) extractsgetIsolationLevelStatementand letsgetConnectionOptionstake 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.disposeto an asyncSymbol.asyncDispose, and the call sites fromusingtoawait using. This is required. Committing/rolling back is async and a sync disposer cannot await it. esbuild lowersawait usingfor thenode22target; 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
bun test— built in, native TS/ESM, Jest-compatible API, built-in coverage. No new runtime deps.libwrapper, the full DB layer, logger, and profiler.mysql2/promiseis mocked at preload (tests/setup.ts→tests/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.tests/integration/, skipped unlessDB_CONNECTIONis set, and excluded from the coverage gate.Coverage gate
coverageThreshold = 0.85inbunfig.toml(bun's built-in, enforced per file).tests/and the untestable bootstrapdatabase/index.ts(a retry loop with a 30s sleep) are excluded viacoveragePathIgnorePatterns. Every measured file is ≥85% line coverage.A second
tsconfig.json(root)bun resolves TS path aliases from the root
tsconfig.json, andsrcuses bare specifiers (config,utils/*,database). The repo previously had onlysrc/tsconfig.json(used by the esbuild build). The new root config extends it and addsbaseUrl/pathsso bun can resolve those imports; it does not affect the production build.CI
.github/workflows/test.ymlrunsbun install --frozen-lockfile && bun run test:coverageon push tomainand on PRs.How to run