Convert tests to vitest#2127
Conversation
| /** | ||
| * @deprecated Use chai's expect-style API instead (`expect(actualValue).to.equal(expectedValue)`) | ||
| * @see https://www.chaijs.com/api/bdd/ | ||
| * @deprecated Use vitest's expect API instead |
There was a problem hiding this comment.
I can get rid of all the deprecation in the follow-up PR, for now I want to convert existing tests with as little changes as possible, to establish a baseline.
There was a problem hiding this comment.
Pull request overview
Migrates the repository’s Node/task/browser test execution from the Mocha/Chai/Sinon/nyc stack to Vitest (with v8 coverage), and replaces the legacy Mocha browser runners with Playwright- and @vitest/browser-driven browser coverage.
Changes:
- Introduces a multi-project Vitest setup (node/tasks/browser) and updates npm scripts + CI to run the new test matrix.
- Converts existing tests/assertions/spies to Vitest APIs and adds new browser setup for running specs in Chromium.
- Removes Mocha/nyc-era runners/config and updates coverage ignore directives to v8-compatible annotations.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.js | Adds Vitest multi-project configuration (node/tasks/browser) and v8 coverage settings. |
| package.json | Swaps Mocha/Chai/Sinon/nyc deps for Vitest + browser/coverage deps and updates test scripts. |
| .github/workflows/ci.yml | Updates browser CI job to run Playwright smoke tests and Vitest browser project. |
| Gruntfile.js | Removes Mocha/nyc-driven test tasks from Grunt pipelines (incl. prior min test hook). |
| tasks/tests/bin.test.js | Converts the bin tests from Grunt/Mocha-style task registration to Vitest tests + custom matcher. |
| tasks/tests/git.test.js | Migrates assertions to Vitest and adds git user config so commits work in CI. |
| tasks/tests/.eslintrc.js | Updates lint globals to reflect Vitest APIs instead of Mocha env. |
| tasks/test-mocha.js | Removes Mocha/nyc Grunt task wrapper. |
| nyc.config.js | Removes nyc configuration in favor of Vitest v8 coverage. |
| spec/env/common.js | Reworks shared spec helpers to use Vitest assertions and resets handlebarsEnv in a global beforeEach. |
| spec/env/node.js | Removes Chai/Sinon setup; relies on Vitest globals and sets up Node Handlebars environment. |
| spec/env/browser-vitest-pre.js | Adds pre-setup for browser tests (noConflict sentinel + global polyfill). |
| spec/env/browser-vitest.js | Adds Vitest browser setup importing ESM Handlebars source and defining CompilerContext. |
| spec/env/browser.js | Removes Chai/Sinon setup and drops minimized-test branching. |
| spec/env/runner.js | Removes legacy Mocha runner. |
| spec/env/runtime.js | Removes legacy Mocha runtime env loader. |
| spec/index.html | Converts Mocha browser harness page into a standalone smoke test page. |
| spec/umd.html | Converts UMD page from Mocha harness to standalone RequireJS-driven smoke test. |
| spec/umd-runtime.html | Converts UMD runtime page from Mocha harness to standalone RequireJS-driven smoke test. |
| spec/.eslintrc.js | Updates test globals from Mocha/Sinon to Vitest (vi, hooks, etc.). |
| spec/basic.js | Removes per-file handlebarsEnv setup now handled centrally. |
| spec/utils.js | Updates assertions from Chai to Vitest. |
| spec/builtins.js | Updates assertions from Chai to Vitest. |
| spec/helpers.js | Replaces xit with it.skip for Vitest. |
| spec/security.js | Replaces Sinon spying/restoration with vi.spyOn + vi.restoreAllMocks and updates assertions. |
| spec/regressions.js | Replaces Sinon spying/restoration with vi.spyOn + vi.restoreAllMocks and updates assertions. |
| spec/runtime.js | Adjusts browser-only noConflict test gating. |
| spec/precompiler.js | Refactors callback-style tests to async/await using a Promise wrapper. |
| spec/javascript-compiler.js | Skips compilerInfo monkey-patching tests in browser mode; updates assertions to Vitest. |
| lib/precompiler.js | Replaces istanbul ignore annotations with v8 ignore annotations. |
| lib/index.js | Updates coverage ignore annotations for v8 coverage tooling. |
| lib/handlebars/runtime.js | Updates coverage ignore annotations for v8 coverage tooling. |
| lib/handlebars/no-conflict.js | Updates coverage ignore annotations for v8 coverage tooling. |
| lib/handlebars/compiler/javascript-compiler.js | Updates coverage ignore annotations for v8 coverage tooling. |
| lib/handlebars/compiler/compiler.js | Updates coverage ignore annotations for v8 coverage tooling. |
| lib/handlebars/compiler/code-gen.js | Updates coverage ignore annotations for v8 coverage tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spec/env/common.js
Outdated
| if (errMsgMatcher) { | ||
| expect(function () { | ||
| self._compileAndExecute(); | ||
| }).toThrowError(errMsgMatcher); | ||
| } else { | ||
| expect(function () { | ||
| self._compileAndExecute(); | ||
| }).toThrow(); |
There was a problem hiding this comment.
HandlebarsTestBench.prototype.toThrow(errorLike, errMsgMatcher) no longer asserts errorLike at all. Many specs call .toThrow(Error) / .toThrow(Exception, /msg/), so this change can allow incorrect exception types to pass. Update the implementation to validate both the error type (when errorLike is provided) and the message matcher (when provided), ideally without executing _compileAndExecute() multiple times.
| if (errMsgMatcher) { | |
| expect(function () { | |
| self._compileAndExecute(); | |
| }).toThrowError(errMsgMatcher); | |
| } else { | |
| expect(function () { | |
| self._compileAndExecute(); | |
| }).toThrow(); | |
| var fn = function () { | |
| self._compileAndExecute(); | |
| }; | |
| if (errorLike && errMsgMatcher) { | |
| expect(fn).toThrowError(errorLike, errMsgMatcher); | |
| } else if (errorLike) { | |
| expect(fn).toThrowError(errorLike); | |
| } else if (errMsgMatcher) { | |
| expect(fn).toThrowError(errMsgMatcher); | |
| } else { | |
| expect(fn).toThrow(); |
spec/security.js
Outdated
| if ({}.__defineGetter__ == null || {}.__lookupGetter__ == null) { | ||
| return this.skip(); // Browser does not support this exploit anyway | ||
| return; // Browser does not support this exploit anyway | ||
| } |
There was a problem hiding this comment.
This test used to call this.skip() when the runtime doesn't support __defineGetter__/__lookupGetter__. Changing it to return; will mark the test as passed (not skipped), which changes intent and can hide missing coverage. Use Vitest's conditional skipping (e.g., define the test with it.skip/it.runIf/it.skipIf depending on the condition) so it reports as skipped rather than silently succeeding.
| { | ||
| test: { | ||
| name: 'tasks', | ||
| include: ['tasks/tests/*.test.js'], | ||
| globals: true, | ||
| }, |
There was a problem hiding this comment.
The tasks test project runs in Vitest's default worker pool, which can execute test files in parallel. tasks/tests/git.test.js calls process.chdir(), which mutates global process state and can make the tasks suite flaky if another test file runs concurrently. Consider configuring the tasks project to avoid parallel file execution (e.g., fileParallelism: false) or to run in isolated processes (e.g., pool: 'forks') so process.chdir() can't interfere with other tests.
| "test:browser": "vitest run --project browser", | ||
| "test:unit": "vitest run --project node", | ||
| "test:tasks": "vitest run --project tasks", | ||
| "test:browser-smoke": "playwright test --config tests/browser/playwright.config.js", |
There was a problem hiding this comment.
Why do we still need "test:browser-smoke"? Shouldn't the vitetest browser-tests do the same if we add Firefox and Webkit? 🤔
Edit: okay, I think you kept this to test the built bundles? But you changed it to not run the whole test-suite, but just a simple smoke test. I understand. I think that makes sense.
There was a problem hiding this comment.
They test different things:
Vitest browser project imports from lib/handlebars.js (source code) and runs the full spec suite in Chromium. This tests library logic in a browser environment.
Playwright smoke tests (test:browser-smoke) load the built dist/ bundles via <script> tags and RequireJS. This
tests:
- dist/handlebars.js works as a global script (spec/index.html)
- dist/handlebars.js works as an AMD module via RequireJS (spec/umd.html)
- dist/handlebars.runtime.js works as an AMD module (spec/umd-runtime.html)
- All three across Chromium, Firefox, and WebKit.
The Vitest browser tests never touch the dist artifacts at all, so they can't catch issues in the build/bundle
pipeline (e.g., broken UMD wrapper, missing exports, RequireJS incompatibility). The Playwright smoke suite is still
needed for that coverage.
There was a problem hiding this comment.
okay, I think you kept this to test the built bundles? But you changed it to not run the whole test-suite, but just a simple smoke test. I understand. I think that makes sense.
yup, exactly!
spec/umd.html, spec/umd-runtime.html to standalone smoke tests driven by Playwright;
builtins, helpers, partials, etc.) now runs in Chromium alongside the UMD smoke tests;
spec/env/browser-vitest-pre.js (polyfills global, sets up noConflict sentinel);
tests;
Browser test notes
Specs requiring Node.js APIs (precompiler.js, spec.js, require.js, source-map.js) are excluded from the browser
project. The #compilerInfo tests in javascript-compiler.js are skipped in browser mode because ESM namespace exports
are read-only and cannot be monkey-patched.
Coverage note
The previous nyc configuration enforced 100% using istanbul instrumentation. The new v8 coverage provider has a
different branch-counting methodology that reports a higher number of total branches for the same code, resulting in
lower branch percentages. The thresholds have been set to match actual measured coverage.
I can try to close this gap in a follow-up PR by introducing new tests covering extra edge cases.