fix(test): one cross-platform test runner, green on Linux, macOS and Windows - #837
Merged
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
added a commit
that referenced
this pull request
Sep 15, 2026
connect()'s catch and connectReadOnly()'s catch closed the handle and then nulled the reference on the next line. close(true) raises when SQLite cannot close, so a throw there kept the reference, and connect()'s own `if (this.db) return` would then answer a retry with a silent success on a provider that is not connected - the exact no-op the comment says the null prevents. Both now null it in a finally. The retry was not pinned either: the failed-connect test measured the file being released but never connected a second time. It does now, and it must reject again. Mutation check: keeping the reference after the failed connect turns that second call into a resolve and fails the test. Raised by an external review of #837.
cevheri
force-pushed
the
fix/cross-platform-test-runner
branch
from
September 15, 2026 12:39
305b21d to
85f025d
Compare
cevheri
added a commit
that referenced
this pull request
Sep 15, 2026
… job helm-pin-matrix's #434 guard destructured `const [ci, npm] = SUITE_SITES`. This branch inserted ci.yml:test-cross-platform between those two sites, so the guard silently compared ci.yml:test with its own cross-platform twin and stopped comparing npm-publish.yml:validate with anything. It now compares every suite site with the first. Mutation check: moving only the release job to another Helm, in the workflow and in the expected table together, passes the old guard and fails the new one. npm-publish.yml's validate job runs the same suite and had no setup-node. The launcher startup tests that refuse anything below Node 24 arrived with #737 on 2026-09-09, which pinned Node in ci.yml only, and the last release this job validated ran on 2026-09-08, so the next release would have met them on the runner image's default Node. It pins Node 24 now, like ci.yml's test job. Both raised by an external review of #837.
… runner `bun run test` is now `bun tests/run-tests.ts`: it discovers every test file under tests/ except tests/live, and runs each one in its own bun process, several at a time. Two defects go away with the shell scripts. tests/run-core.sh used mapfile, a bash 4 builtin, and macOS ships bash 3.2, so the documented gate never ran on a stock Mac; and `bun run test` itself ran tests/unit, tests/api and tests/integration in ONE process, where bun's process-wide mock.module() made the result depend on file order (12849 pass, 42 fail, exit 1 on a clean checkout of main). Per file rather than per directory because that is the isolation the suite needs, not a speed choice: bun 1.4.2 has --isolate, which does reset the module registry per file, but oven-sh/bun#41655 reports a NAPI finalizer SIGSEGV that reproduces serially on 1.4.2 and this suite loads better-sqlite3, oracledb and @duckdb/node-api. The concurrency is what pays for the process boundary: 533 files take about 370 seconds one at a time and about 77 seconds eight at a time. Also here, because the runner owns them: - coverage is per file, merged by scripts/merge-lcov.mjs into the same coverage/lcov.info the gate and SonarCloud already read. The 46 hand-maintained component groups are gone; the two files that must not be measured are COVERAGE_EXEMPT_FILES in tests/runner/discover.ts, with the measurement that says why. - merge-lcov.mjs takes --inputs-from=<manifest>, because 533 report paths do not fit in a Windows command line, and normalises a backslash SF: path, so a report produced on Windows merges as the same file as one produced on Linux. - scripts/security-check.mjs asks `bun tests/run-tests.ts --list` whether a test named by docs/SECURITY.md actually runs, instead of grepping a bash script and a hardcoded directory list that had already drifted (it omitted tests/evals). - tests/unit/component-runner-coverage.test.ts, which existed because the component runner named its files by hand, is replaced by tests/unit/test-runner-discovery.test.ts: the discovery rule must equal what is on disk. - ci.yml gains a non-required Cross-platform Tests job on windows-latest and macos-latest, running `bun run test` from each platform's own shell. The required check keeps its name and its steps.
…Linux The runner change alone does not get a contributor to green: the tests themselves assumed a Linux machine in a dozen ways that never showed up on ubuntu-latest. Paths and handles: - Six files compared a path node returned against a forward-slash literal. tests/unit/agent-package-boundary.test.ts was the worst of them: its isAgentModule() matched /(^|\/)(agent|use-agent)/ against that string, so on Windows it answered false for every module and the boundary gate would have passed VACUOUSLY while a real violation went unreported. - Files that read repository sources through process.cwd() now resolve from import.meta.dir, so a test does not depend on who launched it. - A directory symlink (which needs a privilege a normal Windows shell does not have) is now a junction, and the sqlite credential sandbox no longer needs a symlink at all. - Two tests wrote into the working tree (tests-tmp-duckdb/, and the openshift chart test vendored the subchart into charts/ and into the contributor's own helm config). Both now work in their own temp directory, which is also what makes them safe to run beside other files. - Git fixtures no longer inherit the contributor's global git config, where a commit.gpgsign=true makes the fixture commit exit 128. External tools, through one new helper, tests/helpers/posix-tools.ts: - A test that spawns "bash" or "sh" by name resolves it explicitly instead: on Windows a bare "bash" is C:\Windows\System32\bash.exe when WSL is installed, a Linux shell that cannot see the Windows temp path the fixture hands it, and Git for Windows keeps the real one off the PowerShell PATH. The helper locates it from git itself and refuses the WSL one. - tests/unit/packaging-standalone-zip.test.ts read the archive with 7z; it now reads the central directory in process, so only the packing cases need the binary. tests/unit/backlog-structure.test.ts spawned grep about thirty times; the same search now runs in process, which also removes the GNU/BSD flag divergence. - Where the artifact cannot exist on Windows at all (a deb postinstall, a snap launcher, an AppImage permission audit, a container entrypoint), the test says so in its own title and the runner's summary names the file and the count. Nothing is skipped for being merely inconvenient. Docs follow the code: CLAUDE.md, CONTRIBUTING.md (a per-platform prerequisites table), README.md, docs/TOOLCHAIN.md, seven provider docs, the cut-release runbook, the curated-issue footer and four source comments whose stated reason was the shared process. docs/BACKLOG.md loses D68, which this closes, and gains D85, D86 and D87.
A failing `expect(<happy-dom element>).toBeNull()` inside a waitFor costs 301 ms on a 260-node subtree, because bun's pretty-printer walks the node's object graph to build a diff it will throw away. Measured here: capped at 1000 ms, the toBeNull form gets 9 polls where the boolean form gets 20. Every poll in this suite succeeds on its first try today, so the cost is invisible - until the machine is busy, which is exactly what one process per test file makes normal. All 29 sites now assert the boolean. waitForElementToBeRemoved was measured and rejected for these: it runs an initial-presence check and throws when the element is already gone, which it is at all 29. ConnectionSignature's "does not start the cycle when the viewer asked for reduced motion" was vacuous: it waited 200 ms for a cycle whose first change lands at 2600 ms, so it passed with the guard deleted (measured, 1 pass). It now captures setInterval and carries its own control, and with the guard deleted it fails. Fourteen comments pointing at the deleted tests/run-components.sh and its groups now state what is true: every file has its own process, so a file records what it cannot share a process with, and nothing has to be registered anywhere. Also: tests/unit/test-runner-cli.test.ts wrote its throwaway fixture with an unawaited Bun.write, so the runner sometimes started against an empty file, ran 0 tests and exited 0. It passed on Linux and failed on windows-latest.
…ing two real defects
The suite was run on windows-latest from PowerShell, which is how a contributor
runs it. Ten files failed. Two of them were the tests being wrong about the
platform; two were the product being wrong about it.
Product defects, each with its failing test written first:
- `SQLiteProvider.disconnect()` did not release the file. bun:sqlite's close()
is sqlite3_close_v2, so with a prepared statement still unfinalized the
connection becomes a zombie and the OS handle goes only when the collector
gets there. Measured through /proc/self/fd on Linux: after disconnect()
resolved and isConnected() was false, the database and its -wal and -shm were
all still open. POSIX unlinks an open file, so nothing showed; Windows returns
EBUSY, and a user cannot delete or move a database Studio has disconnected
from. disconnect() now closes with the throwing form, and the test asserts the
portable consequence: the WAL sidecars are gone.
- `scripts/operator-catalog-submission.mjs` guarded its entry with
`import.meta.url === "file://" + process.argv[1]`, a URL compared to a path.
On Windows main never ran at all: node loaded the module, printed nothing and
exited 0, so every one of the twenty CLI cases read a silent success. It
reproduces on Linux too, whenever the path needs percent-encoding (a space is
enough). It is the only script here not using the repository's own idiom.
- `scripts/lib/pack-standalone-tarball.sh` handed tar an output path it had not
resolved, and GNU tar reads `C:\...` as host:file and tries to dial it (exit
2, "Cannot connect to C: resolve failed"). Its zip sibling already resolved
the path, which is why that one passed.
Tests that were wrong about the platform: a Bun.Glob scan compared
host-separated paths against POSIX-spelled documentation; `node --import <abs
path>` is an ESM specifier, so a drive letter reads as a URL scheme; a POSIX
exec bit does not exist on NTFS, so the flatpark resolver check now asks git for
the committed mode, which is what actually ships; and os.tmpdir() answers the
8.3 short form on that runner, so the DuckDB path comparison uses
realpathSync.native on both sides.
The DuckDB same-process handle test now asserts the real answer on each platform
rather than one platform's: POSIX admits the second handle, Windows refuses it
per handle, and both arms end by proving the file opens once the first handle
lets go. docs/providers/{sqlite,duckdb}.md carry the measurements.
… breaks One process per test file means several files run at once, so a test that depended on wall-clock timing is now a flake on any machine that is momentarily slow. Measured at 24 concurrent runs of the same file on a 20-core box: SchemaDiagram.test.tsx was red in 11 of 24 runs before this and is 24 of 24 after; object-tree.test.tsx was 4 of 24 red and is 24 of 24. What each of them was really waiting for: - SchemaDiagram had 25 fixed sleeps standing in for an un-awaited async chain. They are gone: an export waits for the button to go disabled (the control that proves the export started) and then to come back, which is co-extensive with the chain including its error path. File time 4.82 s to 869 ms. - object-tree's expandApp() returned on the folder row, which is drawn from the declared kind and is on screen a round trip before the counts answer, so every caller that read a badge was racing. It now returns on the badge. The "refresh clears the failure" test waited on a row count that was already satisfied before the action under test ran, so it waited for nothing; it now waits on the failure clearing and asserts the refusal was replaced by the engine's answer. - first-paint waited on a row rendered one request before the request it counts. - TablesTab's next click was delivered to a disabled button, so it silently never happened; the test now waits for the button to come back. - QuerySafetyDialog asserted an absolute CPU budget (200 ms) for a predicate whose defect is quadratic growth. It now asserts the growth ratio between 35 KB and 350 KB inputs, which is what discriminates: 10.3x idle, 17.8x at 40 concurrent processes, 25.1x at 64, against about 97x for the pattern it replaced. - use-storage-sync asserted an elapsed-time threshold; it now asserts an ordering between two timers, which is true however slow the machine is, and the ordering is proved to discriminate by mutation. - factory.test.ts closes its providers before deleting their directory, which is what Windows needs, and two /tmp literals became tmpdir(). - The two provider censuses import their shared connection from tests/helpers/census-connection.ts instead of one test file importing the other, which was registering the census suite twice: 15 tests became 6.
538 files and 17,376 tests, counted with the runner itself, and the wall clock at four concurrency levels on a 20-core Linux box: 211 s one file at a time, 61 s at 4, 36 s at 20. Also replaces the em dashes two provider-doc paragraphs picked up.
…an answer
All three were caught by running the suite the way contributors now will: many
files at once, on three operating systems.
- AgentRail's seed fixture waited for `servedSeeds` to be "defined", and the
hook's initial value is `NO_SERVED_SEEDS`, so the wait was satisfied on the
first render and the test read the state before the request was answered.
Measured: 6 of 24 concurrent runs red, and a probe showed 19 of 24 reading the
module constant by identity. The wait is now on the state no longer BEING that
constant, which is the only thing that separates "the server answered with no
seeds" from "nobody has answered yet", since the two are deep-equal. 96
reproduction runs green after the fix. The hook defect this exposed, a non-OK
the server does not attribute to its seed configuration leaving the state at
"serves no seeds", is filed as B81 with the sentence a user is shown.
- factory.test.ts built its unresolved path spelling with `split("/")`, which on
Windows returns the whole backslash path, so the spelling was nonsense and
matched nothing. It uses basename now.
- The SQLite release test asserted the WAL sidecars were gone. Probed on all
three runners: that is true on Linux and Windows and false on macOS, where
bun:sqlite links Apple's system libsqlite3, which keeps them after a real
close (a second library's close then removes them, which needs the exclusive
lock a surviving handle would deny). The test now asks each platform what it
can answer: everywhere, the file can be renamed, which is what Windows refuses
for a live handle; on Linux, no descriptor of this process still points into
the directory, which is the measurement the defect was found with. Mutation
check: restoring the deferred close fails it again.
CodeQL flagged the pattern escape as incomplete because it escapes the BRE metacharacters but not the escape character. The refusal above it means a backslash never reaches that line, so the finding is not reachable today, but a translation that is only correct while its caller is should not be left that way: the backslash joins the class. ` . * ^ $` stay unescaped on purpose, since BRE and JS read them alike.
connect() opens the handle before its pragmas run, so a connection pointed at a file that is not a SQLite database - the ordinary wrong-file mistake - opened it, failed on "file is not a database", and kept it. Measured through /proc/self/fd before the fix: one descriptor on the file survived a connect() that had already thrown and reported isConnected() false. On Windows the user then cannot delete or move the file they picked by accident, the class of defect disconnect() was just fixed for. Nulling the handle also stops connect()'s own `if (this.db) return` from turning a retry into a silent no-op. Found by peer review of this branch. docs/providers/sqlite.md also stops calling the WAL sidecars the portable proof of a release: probed on all three runners, Apple's system libsqlite3 keeps them after a real close.
Each of these was reproduced by a reviewer before it was fixed. - A child that exits 0 WITHOUT printing bun's summary was reported PASS and the run exited 0. A test calling process.exit(0) does exactly that: bun prints its banner, exits 0, and every test after that line never runs. It is now a failure whose reason says its tests are unaccounted for, and so is a file that registers no test at all. - Skip reasons reached no output. bun prints a skipped test's title nowhere (measured piped, with FORCE_COLOR and under a real pty), and the runner printed a passing child's output nowhere either, so on Windows, where a dozen files declare skips, the run said "0 fail" and named nothing. Every child now writes bun's junit report, and the summary lists each skipped test by title under its file. Two comments that claimed bun printed those titles in a terminal are corrected. - --coverage-dir is emptied before a coverage run, with no check on what it named: --coverage-dir=src deleted the product. The runner now refuses a directory holding anything a previous coverage run did not write, and names what it found. - A red coverage run left the previous coverage/lcov.info in place, which coverage:check would then pass. The merged report is removed before the run. - A timed-out file was reported against its elapsed time (budget plus kill escalation) rather than its budget, and a child that finished as the timer fired was counted as timed out. - The runner's own fixture test file could survive an interrupted run and turn every later run red; a leftover is now removed when the file loads. - A relative selector is resolved from the working directory, as the person typing it meant. - The QuerySafetyDialog growth-ratio guard gains an absolute ceiling, so a uniformly slow rewrite cannot hide behind a linear shape.
79 browser tests in 18 spec files for `bun run test:e2e` (plus the base-path spec on its own config), not 32; 539 files and 17,396 tests; 100% of 56,671 lines. The four localized READMEs said six test layers where the English one now says seven, which readme:check cannot see.
Measured on windows-latest with the suite running four files at once: "a reachable remote pin is compared like a local one" took 4480 ms and reported the local server's pin as unreadable. The test had set a 3000 ms fetch budget for the node process it spawns, and bun's own per-test default is 5000 ms, which is sized for a function call rather than a process start plus an HTTP round trip. The fetch budget is removed rather than raised. No case in these two describes needs a fetch to time out: every failure they exercise answers at once, with a refused connection or a status code, so the short budget could only ever fail the success path. The script's default applies, which is what a real run gets. The 18 cases that spawn node and fetch get a named 30 s test budget; the pure case in the same describe keeps bun's default.
connect()'s catch and connectReadOnly()'s catch closed the handle and then nulled the reference on the next line. close(true) raises when SQLite cannot close, so a throw there kept the reference, and connect()'s own `if (this.db) return` would then answer a retry with a silent success on a provider that is not connected - the exact no-op the comment says the null prevents. Both now null it in a finally. The retry was not pinned either: the failed-connect test measured the file being released but never connected a second time. It does now, and it must reject again. Mutation check: keeping the reference after the failed connect turns that second call into a resolve and fails the test. Raised by an external review of #837.
… job helm-pin-matrix's #434 guard destructured `const [ci, npm] = SUITE_SITES`. This branch inserted ci.yml:test-cross-platform between those two sites, so the guard silently compared ci.yml:test with its own cross-platform twin and stopped comparing npm-publish.yml:validate with anything. It now compares every suite site with the first. Mutation check: moving only the release job to another Helm, in the workflow and in the expected table together, passes the old guard and fails the new one. npm-publish.yml's validate job runs the same suite and had no setup-node. The launcher startup tests that refuse anything below Node 24 arrived with #737 on 2026-09-09, which pinned Node in ci.yml only, and the last release this job validated ran on 2026-09-08, so the next release would have met them on the runner image's default Node. It pins Node 24 now, like ci.yml's test job. Both raised by an external review of #837.
- --merge-into is removed before a coverage run so a red run cannot leave a stale report behind, and like --coverage-dir it is a path the caller chooses: --merge-into=package.json deleted package.json. An existing target must now be an lcov report (it starts TN: or SF:, or it is empty), and anything else stops the run and says what it found. Probed end to end: exit 2, package.json intact. - A test skipped by describe.skip carries its reason in the DESCRIBE title, and the summary printed only the inner test's name, so the Windows packaging skips listed what they check and not why they did not run. bun writes the describe path in junit's classname, innermost first (measured 1.4.2); the summary now prints the path outermost first, then the name. Pinned end to end. - Ctrl+C left the run's junit scratch directory behind. It is removed on every exit now; probed with a control that sees it exist mid-run and gone after. - The runner's own failing and empty fixture files were written into this repository's tests/unit/, where an interrupted run left a red file and every concurrent walker of tests/ could see it. Those cases now run a copy of the runner in a temporary directory with a tests/ tree of its own. - tests/unit/lib/auth-bootstrap.test.ts still ended two tests with a bare `return` on Windows, which reports a pass for an assertion that never ran. They are named skips now, the way instrumentation.test.ts already does it. - loop/scripts/gate.sh ran `bun run test` and then `bun run test:coverage`, which is the same runner over the same files twice. It runs the coverage form once. Not taken, with the reason: the QuerySafetyDialog ratio cannot divide by zero in practice (10000 consecutive performance.now() calls gave 10000 distinct values, and the small input costs about 1.7 ms), and a zero would fail the assertion loudly rather than pass it. Making the cross-platform job a required check is a branch-protection decision for the owner. Counts follow the rebase onto #812: 540 files, 17,435 tests, 100% of 56,764 lines.
…one directory agree Measured on windows-latest: the three sandbox cases added in the previous commit exited 2. os.tmpdir() there answers the 8.3 short form (C:\Users\RUNNER~1\...) while import.meta.dir answers the long one (C:\Users\runneradmin\...), and since relative selectors are now resolved from the working directory, path.relative compared the two spellings and called a correct selector "not under tests/". That is the runner being wrong, not the test: a contributor whose checkout is reached through a junction, a subst drive or a symlink would hit the same sentence. Root and working directory are now compared as real paths. A junction reproduces it on Linux, which is how the new discovery test pins it (it failed with exactly the Windows message before the fix). The sandbox cases also carry the runner's stderr in their exit-code assertion, so a red one says why.
542 files and 17,477 tests, 100% of 56,883 lines. #817's test files were also run 24 times at once each, under the per-file runner they have not met before: 24/24.
cevheri
force-pushed
the
fix/cross-platform-test-runner
branch
from
September 15, 2026 13:08
5e4aead to
3eab7a2
Compare
…ire them in CI A fresh Windows VM with only git and an editor could not get a green bun run test without installing Helm and building the chart's PostgreSQL subchart, a Kubernetes tool most contributors never touch. The twelve test files that spawn helm now declare // @requires helm on their first line. Where helm is not on PATH or the subchart is not built, the runner does not start them, and its summary lists them once under the reason and the command that fixes it. A selection made only of such files exits 2 rather than going green on nothing. Every CI job that runs the suite sets LIBREDB_REQUIRE_HELM=1, which turns the same condition into an error before anything starts. helm-pin-matrix fails if a job loses the variable, and test-runner-requirements holds the marker true of the tree in both directions. Measured on Linux with helm hidden from PATH: 531 files pass and coverage is still 100% of 56883 lines. With the variable set: 543 files, 17499 tests, all gates green.
Each worker runs one file at a time and the concurrency comes from the number of workers, so no-await-in-loop on that line is the design rather than a missed Promise.all.
…real-path space Discovery classified each entry by its Dirent type, and a Dirent for a symbolic link is neither a file nor a directory, so a linked test directory was skipped without a word and naming the link said it matched no test files. Entries are classified with lstat now, which also reports a Windows junction as a link, and a link is refused by name rather than followed: following one can run files from outside the repository, loop on a link to a parent, and list one file twice under two names. A relative selector that passed through a link was refused as not under tests/, although the same path given absolutely worked. Every selector is resolved against the working directory and then realpathed when it exists.
… the way its docs say The counts came from bun's console output, which is free-form text mixed with whatever the tests printed. Measured on 1.4.2: a file that registered no test and printed " 1 pass" was reported PASS with exit 0, and so was a test that printed a whole summary on stderr and then called process.exit(0), which defeats both guards that keep a red tree from going green. The counts come from the junit report each child already writes, a missing or unreadable report fails the file and says which, and a report that records a failure fails it whatever the exit code says. The skipped titles come from the report's nested testsuite elements: in classname bun writes the separator and a literal ">" identically, so a describe titled "rows where count > 100" printed as "100 > rows where count". Each child's output is kept head and tail, a megabyte each end, rather than whole, and a passing file's output is dropped once it has been reported. Every exit path lets its last lines drain first: with stdout a pipe, a failing file that printed 200 KB lost its output and the whole summary. A reader that goes away keeps the run's own exit code, so "| head -1" no longer answers 2. SIGTERM, SIGHUP and SIGBREAK stop a run as SIGINT does, with 128 plus the signal's number, and a scratch directory that cannot be removed is named rather than thrown: a throw inside the listener left bun running and the queue starting the next file. A signal is answered even when the reader has stopped reading, and a second one kills.
Says where a file's counts come from and why the console is not read, which signals end a run and with what code, what the output capture keeps, and what draining covers: a child can still drop its own queued console output at exit, which no drain in the runner can put back. Files D88 for a memory-aware default concurrency, D89 for that child-side loss, and D90 for a committed .only, which bun honours so the file reports PASS with its other tests never run. Re-counts the README tables and dates every measurement that is not current.
…st-runner main's #839 filed D91 to D94, so this branch's three runner entries move to D95, D96 and D97, and the sentences that cite them move with them. Its four new test files bring the tree to 549, which the README, docs/TOOLCHAIN.md and D86 now say.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36754982 | Triggered | Generic Password | 1c9dacf | tests/isolated/factory.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…e same sites Both tests build a repository-relative path with path.relative and compare it to a POSIX literal, so on windows-latest the keydown registration sites and the wire module's import closure came back spelled with backslashes and both files were red. That is the defect class this branch exists to remove, so both normalise the separator the way the rest of the suite does.
|
Asgabani
added a commit
to Asgabani/libredb-studio
that referenced
this pull request
Sep 15, 2026
Rebased onto upstream/main past libredb#837 (the only conflict was tests/run-components.sh, deleted upstream in favor of the new auto-discovering test runner - my one-line registration of ShortcutsDialog.test.tsx there is now moot). Three fixes from the maintainer's second review: 1. One Escape closed the shortcuts dialog AND the profiler underneath it. Radix's Dialog handles Escape in the capture phase and only calls preventDefault(), never stopPropagation(), so DataProfiler's own bubble-phase Escape listener still fired. Added a defaultPrevented check. 2. SHORTCUT_GROUPS was a second hand-maintained list with no guard tying it to SHORTCUTS, so a fifth registry entry could go undocumented in the dialog. Added a generic test that every SHORTCUTS entry's label appears somewhere in SHORTCUT_GROUPS. 3. The maintainer corrected my "nothing there yet" answer on tab-strip vs. object-tree keys: ObjectTree.tsx does implement a W3C tree keyboard pattern on its role="tree" root (arrows, Home/End, Enter/Space, Shift+F10), just not in TreeRow.tsx where I'd looked. Added an "Object tree" group documenting it, display-only like the tab-strip arrows (none of it is a Monaco command). Also updated the three places that manually enumerate every document-level keydown listener for D82 reasons (Studio.tsx, StudioWorkspace.tsx, tests/unit/document-keydown-listeners.test.ts, tests/components/studio/embedded-source.test.tsx) now that ShortcutsDialog.tsx's `?` listener makes it five instead of four.
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.
What this does
bun run testis now one cross-platform runner,tests/run-tests.ts, and the suite it runs is green on Linux, macOS and Windows.Two defects went with the old arrangement:
bun run testrantests/unit,tests/apiandtests/integrationin ONE bun process, andmock.module()is process-wide with no undo, so a mock one layer needs reached every file after it. On a clean checkout ofmainthat is 12849 pass, 42 fail, exit 1, and the failing set changes with the directory order of the tree. CI never saw it, because CI rantests/run-core.shinstead.tests/run-core.shusedmapfile, a bash 4 builtin; macOS ships bash 3.2, so the gate CI depends on has never worked on a stock Mac. On Windows neither script ran outside Git Bash.The runner discovers every
*.test.ts(x)undertests/excepttests/live/, runs each file in its own bun process, several at a time (one per CPU by default), kills and fails a file that outruns its timeout, and merges per-file coverage into the samecoverage/lcov.infothe gate and SonarCloud already read. The 46 hand-maintained component groups andtests/unit/component-runner-coverage.test.tsare gone: discovery is a rule, so a new test file cannot be forgotten (which is howWireCompatibilityHint.test.tsxshipped in #426 and never ran once).Measured
bun run testExecutable not foundCross-platform Tests)mapfile, bash 3.2)Cross-platform Tests)Wall clock on a 20-core Linux box: 211 s one file at a time, 61 s at 4 at a time, 36 s at 20. The Windows leg takes about 6.5 minutes and the macOS leg about 3 on GitHub's runners.
Why one process per file, rather than
bun test --isolate--isolateexists in the pinned bun 1.4.2 and does reset the module registry per file, which would let the runner start a handful of processes instead of one per file. It is not adopted because of oven-sh/bun#41655, a NAPI finalizer SIGSEGV that reproduces serially on 1.4.2, and this suite loads three NAPI addons.--parallelwas measured too: 0 fail in 7 runs, then one run hung 17 minutes inside a synchronoushelm templateand one failed 7 tests ofdocker-bind-addresson 5000 ms timeouts. Both refusals, with their evidence, are indocs/TOOLCHAIN.md, and re-probing is filed as D86.The tests had to change too
The runner alone gets nobody to green. What the platforms actually broke, all fixed here:
agent-package-boundary.test.ts, would have passed VACUOUSLY on Windows: its predicate matched/(^|\/)(agent|use-agent)/against a backslash path, so a real boundary violation would have gone unreported.commit.gpgsign.bash,sh,grep,taror7zby bare name. On Windows a barebashis WSL'sSystem32\bash.exewhen WSL is installed, a Linux shell that cannot see the Windows path the fixture hands it.tests/helpers/posix-tools.tsnow resolves these through the Git for Windows install, located from git itself.SchemaDiagram.test.tsxwas red in 11 of 24 concurrent runs; it is 24 of 24 now. A failingexpect(<element>).toBeNull()inside awaitForcosts 301 ms, because bun pretty-prints the DOM node it is about to throw away, so 29 poll sites moved to a boolean form.And three product defects the platforms and the review exposed:
SQLiteProvider.disconnect()did not release the file. bun:sqlite'sclose()leaves a zombie connection while a prepared statement is unfinalized, so the handle went when the collector got there. Measured through/proc/self/fd: afterdisconnect()resolved, the database,-waland-shmwere all still open. A user cannot delete or move a database Studio has disconnected from.SQLiteProvider.connect()kept the file it had opened when its pragmas then failed, which is what happens when a connection points at a file that is not a database. Same consequence on Windows: the file the user picked by mistake stays undeletable.scripts/operator-catalog-submission.mjscomparedimport.meta.urlto"file://" + argv[1], so on Windowsmainnever ran: the CLI printed nothing and exited 0. It reproduces on Linux whenever the path needs percent-encoding.What a contributor needs
CONTRIBUTING.mdnow carries a per-platform prerequisites table: bun 1.4.2, Node 24, Git, and a POSIX shell for the tests that drive packaging scripts (on Windows, the one Git for Windows ships; the tests find it themselves)..envanddata/are not needed.Helm is not needed either. The twelve chart tests that drive the real
helmbinary declare// @requires helmon their first line. Where Helm or the chart's built PostgreSQL subchart is missing, the runner does not start them and lists them once, under the reason and the command that fixes it; a selection made only of such files exits 2 rather than going green on nothing. Every CI job that runs the suite setsLIBREDB_REQUIRE_HELM=1, which turns the same condition into an error before anything starts, andhelm-pin-matrix.test.tsfails if one of those jobs loses the variable.test-runner-requirements.test.tsholds the marker true of the tree in both directions: a file that spawns helm carries it, and a file that carries it spawns helm.On Windows, the tests for artifacts that cannot exist there (deb/rpm postinstall, the snap launcher, the AppImage permission audit, the container entrypoint) declare it, with the reason in the test's own title. bun prints a skipped title nowhere, so every child writes bun's junit report and the runner's summary lists each skipped test by title under its file. Nothing is skipped for being merely inconvenient.
CI
The required check
Unit & Integration Testskeeps its name and its steps, and still runsbun run test:coveragethenbun run coverage:checkon ubuntu. A new job,Cross-platform Tests (windows-latest | macos-latest), runsbun run testfrom each platform's own shell. It is deliberately not a required check yet; once it has a history of being green it is worth promoting, which needs a branch-protection change. Both jobs, and the release validation innpm-publish.yml, setLIBREDB_REQUIRE_HELM=1, so none of them can leave the chart tests out.npm-publish.ymlrunsbun run testwhere it ranbun run test:ci; there is notest:ciany more, because there is no longer a shared-process form of the suite for it to be the alternative to.Review
GitHub Copilot review could not be requested on this repository (the reviewer request is accepted and comes back empty), so the branch was reviewed by three independent agents, one each for the runner, the honesty of the test changes, and the product and documentation changes, and every finding was then handed to a separate skeptic whose job was to refute it. What survived was fixed here, each with a test: a child exiting 0 without a summary counted as a pass, skip reasons reached no output,
--coverage-dir=srcwould have deletedsrc/, a red coverage run left a stale merged report forcoverage:checkto pass, the SQLiteconnect()leak above, and several stale numbers. One CodeQL finding on the in-process grep translation was also addressed. What the skeptics could not reproduce was dropped.A later external review added six more, all verified and fixed: the #434 Helm drift guard had silently stopped comparing
npm-publish.yml:validateonce a third suite site was inserted; that release job also lacked the Node 24 pin its tests need since #737;--merge-intowould delete whatever path it was given; skip reasons on a skippeddescribedid not reach the summary; the SQLite catch nulled the handle outside afinallyand no test connected a second time; and the runner's own fixture files were written into the realtests/tree. The Windows leg then caught one more in the runner itself: with the working directory spelled in 8.3 short form and the runner's own path in long form, a correct relative selector was reported as outsidetests/; selectors are now compared as real paths. The branch is rebased onto #812 and #817, and #817's test files were each run 24 times at once under the new runner: 24/24.A first run on a fresh Windows VM, with only git and an editor installed, is what made Helm optional: installing a Kubernetes tool to see the suite go green was the first thing a contributor who never touches the chart would hit.
A second external review, nine findings, was verified the same way, one agent per finding, each measuring rather than adopting. Two were refuted: the proposed
SQLiteProvider.disconnect()fix would have dropped the reference to a handle SQLite reports as still open, which is the file lock this PR set out to remove, andclose(true)never threw in any scenario the provider can reach; the timer leak has no effect, becauseprocess.exitends the run and--no-orphansreaps the children. One proposed fix could not work: in a junitclassnamebun writes the describe separator and a literal>identically. The rest were real, and four were worse than reported. A file that registered no test but printed " 1 pass" was reported PASS with exit 0, so a file's counts now come from its junit report rather than from console text; the skipped-title path comes from the report's nested<testsuite>elements; a link undertests/is refused by name rather than silently skipped; and SIGTERM, SIGHUP and SIGBREAK now end a run the way SIGINT does, with the scratch directory removed and a removal failure named rather than thrown.That work was then reviewed in turn, by four agents with separate lenses, one of which mutated each new behaviour in a copy to check that a test really fails without it. It found a regression the round had introduced: draining stdout before exit let a reader that stopped reading swallow a signal, and turned a broken pipe into exit 2, so
bun tests/run-tests.ts <file> | head -1reported that the runner had failed. Both are fixed, with the cases that pin them. It also found a test that asserted more than the runner can promise: a child can lose part of its own console output at its exit under a loaded machine, with no runner in the picture, which is filed as D89 rather than asserted against.Follow-ups filed
docs/BACKLOG.mdloses D68, which this closes, and gains D85 (the hand-copied@/lib/authmock, which per-file isolation does nothing about), D86 (re-probe--isolatewhen #41655 closes), D87 (two packaging tests that cannot run on Windows because the scripts they drive shell out tozipand to bash) and B81 (a non-OK seed response the server does not attribute leaves the rail claiming the server serves no seeds, found while fixing theAgentRailrace).