Migrate chai and sinon to vitest - #220
Open
Michiel-VandeVelde wants to merge 33 commits into
Open
Conversation
Splits the Mocha->Vitest and JS->TS conversions into two steps so each diff stays small and reviewable. This is step 1: syntax only, no types.
…PatternFragmentsHtmlView feature-webid/controllers: 16% -> 80%, including a second pre-existing bug found along the way (not fixed): _verifyWebID's predicate switch compares an N3 term object to string literals with ===, so it never matches. feature-summary/views/summary: 18% -> 100%. QuadPatternFragmentsHtmlView.ts: 54% -> ~100%.
Util.ts: 87% -> 100% statements. UrlData.ts: found a third pre-existing bug along the way (not fixed): options.assetsPath is always ignored, since baseURLPath + 'assets/' is never an empty string and short-circuits the || fallback.
EmptyDatasource/IndexDatasource: 50%/26% -> ~100%. View.ts/ViewCollection.ts: cover the view-extension rendering paths. Error/Forbidden/NotFound HtmlView: cover their template-selection logic. HtmlView.ts: 35% -> 79%. The remaining gap is qejs's actual render call, which can't be exercised here: qejs resolves template paths via require.main.filename, which Vitest's module runner never sets, so every real qejs.renderFile call fails structurally under this test setup. Only the resulting error-handling path is covered; the success path isn't reachable in tests.
… feature-webid Datasource.ts: 96% -> 98.5% (graph-side blank-node translation, forced default graph without quad support). SparqlDatasource.ts: 92% -> 100% (_encodeObject/_convertLiteral edge cases). QuadPatternFragmentsController.ts: 88% -> 96.5% (extension chain, including the error-logging path, and close()). WebIDControllerExtension.ts: 80% -> 90% (the full HTTPS + TLS _handleRequest flow). The remaining gap is the predicate switch in _verifyWebID, already established as unreachable dead code.
…nches (96.6% -> 100%)
CliRunner.ts: 90.47% -> 98.41% (SIGHUP abort-before-listening and already-in-progress paths). The one remaining line is a defensive "workers.pop() returned nothing" fallback inside the respawn loop that appears structurally unreachable given the surrounding control flow. LinkedDataFragmentsServer.ts: covered the default no-op _log fallback.
Constructor validation, controller ordering, datasource error handling, logging setup, and run()'s full lifecycle (listen-when-ready, port override, SIGINT stop, forced second-SIGINT exit) — all driven through stubbed process.once/on, matching the pattern CliRunner-test.js already uses for process/signal handling, so nothing is ever registered on the real process listeners. Found a fourth pre-existing bug along the way (not fixed): access-log's own implementation treats a `null` third argument as an options object, since `typeof null === 'object'` — so the accesslogger this file builds throws every time it's actually invoked. Access logging is completely broken; the test documents that rather than working around it.
feature-memento, feature-qpf, and feature-summary Controller.ts is now at 100% branches. Small, previously-untested branches closed: pre-built ViewCollection reuse, an already-set parsedUrl/Vary header, a Forwarded header without proto, a double-invoked next()/done(), View's error-to-response-emit path, N3/RdfaDatasource's file-option fallback, DatasourceRouter's missing-parsedUrl fallback, SummaryController with no configured summaries directory, MementoHtmlViewExtension with no timegates configured, and QuadPatternFragmentsRdfView's no-pageUrl short-circuit.
…ller Datasource.ts: subject/object blank-node translation on both the query and result side, plus select() without an onError callback. (Several other flagged branches on this file turned out to be a coverage-tool sourcemap artifact, not real gaps — confirmed by checking raw lcov statement hit counts, which showed those exact lines executing thousands of times across the existing HDT/SPARQL integration tests.) QuadPatternFragmentsController.ts is now at 100% branches: _createPatternString's full term/graph matrix, the error-without-stack log path, and the previousPageUrl branch on page >= 2.
Closes the last real branch gap in Util.ts: no existing test constructed an error without a message, so the `message || ''` fallback was never exercised.
…r tests
qejs's template resolver reads require.main.filename when resolving a
template referenced via inherits(...). Vitest's workers have no classic
CommonJS entry script, so require.main is undefined there and that access
throws. The throw happens inside an unterminated Q promise chain, so Q
silently swallows it instead of surfacing it, and response.emit('error', ...)
throwing for the same reason (no listener attached) skips the response.end()
call right after it, leaving the response hanging forever.
Fix: a Vitest setupFiles script sets process.mainModule if unset, so
require.main resolves the way qejs assumes. Un-skip the 3 describe blocks
this was blocking, and stop forcing NotFoundHtmlView/NotFoundRdfView's
require() onto the compiled .js output, matching how the rest of the suite
requires views (extensionless, resolving through Vite to .ts source).
…r), Controller, HtmlView, SparqlDatasource, and N3ParserExtended; ignore coverage/
…iewExtension regressions from upstream refinement
Contributor
Author
|
@jitsedesmet ready for review :) |
jitsedesmet
self-requested a review
August 21, 2026 12:21
jitsedesmet
requested changes
Aug 21, 2026
There was a problem hiding this comment.
I sampled some files, and I found two things. By no means is this a complete review, but since I expect I'll have less to say beyond this, I would request these changes already.
(I also expect the line diff for these changes might be great and would unpin all my file-views)
sinon's stub/spy API predates vitest and duplicates functionality vitest already provides natively; dropping it removes a devDependency and lets every test file use one consistent mocking API.
Leftover node >=10.0 pin from before the modernization work; the root package.json's devDependencies (e.g. @types/node ^22) already reflect the actual supported Node version.
Enables avoid-new, prefer-await-to-then, and prefer-await-to-callbacks on *-test.js files, matching the modern-async-only style Comunica's own eslint config pushes for the same reason: vitest/supertest are already promise-based, so wrapping them back in new Promise((done) => ...) or driving them through .then()/callbacks is no longer necessary.
Lets callers (including its own tests) await the actual build/instantiate completion signal instead of guessing how many event-loop ticks the internal promise chain takes to settle. runCli discards the returned promise explicitly with void, following this codebase's existing convention for intentionally-unhandled promises (see HtmlView.ts).
Replaces new Promise((done) => ...) test-body wrappers, supertest .end(callback) chains, and .then() chains with async/await throughout. Event/callback-based APIs (EventEmitter, datasource.close(cb), etc.) are bridged via Node's built-in events.once()/util.promisify() instead of hand-rolled new Promise(); genuine one-off deferred promises use Promise.withResolvers() instead. A few test doubles that must themselves mimic a callback-based API keep callbacks with a targeted eslint-disable explaining why. Converting HtmlView-test.js surfaced 3 tests that were silently ignoring the render callback's error argument, masking a template-rendering gap in the test fixtures (missing the title/header/assetsPath/baseURL defaults View.render() normally merges in) — fixed by spreading view._defaults into those tests' options to match the real calling convention.
Promise.withResolvers() is Node 22+ (ES2024), but CI tests against Node 20.x/22.x/24.x — the 20.x job failed with "Promise.withResolvers is not a function" since that was only verified locally on Node 22. Added a plain-Promise-based withResolvers() to test/test-helpers.js (outside the promise/avoid-new rule's *-test.js glob) and switched all call sites to import it instead of using the native version.
It was in yarn.lock (and my local node_modules from an earlier yarn add) but never actually landed in package.json, so a real clean install -- like CI's -- had nothing to install it from, breaking the lint job with "ESLint couldn't find the plugin \"eslint-plugin-promise\"". Verified by deleting node_modules/eslint-plugin-promise and reinstalling from scratch.
jitsedesmet
requested changes
Aug 25, 2026
jitsedesmet
left a comment
jitsedesmet
left a comment
There was a problem hiding this comment.
Already a first part. The changes are nice improvements, but some of the tests that are added break the TS API we added previously, and I am hesitant to accept that. If this is needed in tests, it is possible. But I would wait until we migrate the test suite to TS before we decide what API-breaking tests we want to add; that way, we can be deliberate about them.
jitsedesmet
requested changes
Aug 25, 2026
jitsedesmet
requested changes
Aug 26, 2026
promisify is Node-specific and required detaching close() from its instance (needing .bind()) to wrap it. Calling close(resolve) as a normal method avoids both: no Node-only API, and this stays correct without an explicit bind.
jitsedesmet
requested changes
Aug 26, 2026
…mise executors Promise.race([once(result, 'end'), promise]) was hard to follow; collecting into resolve() directly inside a single new Promise executor is simpler and covers the same success/error semantics. Once that was the only remaining Promise.race use, and withResolvers had no non-trivial callers left, remove the withResolvers helper entirely and inline new Promise(...) at every site.
jitsedesmet
requested changes
Aug 27, 2026
Co-authored-by: Jitse De Smet <35114273+jitsedesmet@users.noreply.github.com>
Co-authored-by: Jitse De Smet <35114273+jitsedesmet@users.noreply.github.com>
Co-authored-by: Jitse De Smet <35114273+jitsedesmet@users.noreply.github.com>
…file The applied suggestion left a TypeScript return-type annotation (: Promise<number>) in test/test-helpers.js, which isn't compiled through tsc, so it failed to parse and broke every test file importing from here. It also dropped the `once` import streamLength depends on. Fix both.
jitsedesmet
approved these changes
Aug 28, 2026
jitsedesmet
changed the title
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
3 participants
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.
No description provided.