test(rollup-plugin-html): migrate from mocha/chai to node:test#3097
test(rollup-plugin-html): migrate from mocha/chai to node:test#3097bennypowers wants to merge 2 commits into
Conversation
Replace mocha/chai with node:test and node:assert/strict across all 9 test files (139 tests). Update test script to use node --experimental-strip-types --test. Change source imports to dist imports and use import type for type-only imports. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
| import { parse } from 'parse5'; | ||
| import path from 'path'; | ||
| import { extractAssets } from '../../../../src/input/extract/extractAssets.js'; | ||
| import { extractAssets } from '../../../../dist/input/extract/extractAssets.js'; |
There was a problem hiding this comment.
why do you import from dist now?
I saw you did similar change in other PRs, but not 100% clear why
There was a problem hiding this comment.
this requires TypeScript 5.7+ with rewriteRelativeImportExtensions: true. That lets source files use .ts extensions in imports. we determined in chat that this migration is not worth doing until after node 22 EOL
There was a problem hiding this comment.
got it
then I think we need to run build always before tests, shouldn't this be part of the change too? or is that smth for the future PR when all PRs are complete?
There was a problem hiding this comment.
it runs before tests in ci
local dev and test watch will remain broken until we finish the job and point tests at src
afaict there's no satisfying way to maintain the current dx while doing this upgrade piecemeal
| "demo:spa": "rm -rf demo/dist && rollup -c demo/spa/rollup.config.js --watch & npm run serve-demo", | ||
| "serve-demo": "node ../dev-server/dist/bin.js --watch --root-dir demo/dist --app-index index.html --compatibility none --open", | ||
| "test:node": "mocha test/**/*.test.ts --require ts-node/register --reporter dot", | ||
| "test:watch": "mocha test/**/*.test.ts --require ts-node/register --watch --watch-files src,test" |
There was a problem hiding this comment.
I saw you kept "watch" in other PRs, any specific reason to remove it here?
| expect(result).to.include("url('assets/image1.png')"); | ||
| expect(result).to.include("url('assets/image2.png')"); | ||
| assert.ok(result.includes("url('assets/image1.png')")); | ||
| assert.ok(result.includes("url('assets/image2.png')")); |
There was a problem hiding this comment.
chai gives nice output in such case which
- increased debugging locally
- help understand what failed in the pipeline, especially if it's executed on Windows
I think this is very important and I'd prefer to have a better alternative to this
I quickly checked possibilities and asked copilot and this is the first idea to start this discussion: create a reusable helper like this and put it into test-utils/assert.js
function assertIncludes(actual, expected) {
if (!actual.includes(expected)) {
const preview = actual.length > 200
? actual.slice(0, 200) + '...'
: actual;
throw new assert.AssertionError({
message:
`Expected substring not found.\n\n` +
`Expected:\n${expected}\n\n` +
`Actual (preview):\n${preview}`,
actual,
expected,
operator: 'includes'
});
}
}There was a problem hiding this comment.
we already export expectIncludes and expectNotIncludes, i can refactor to use that
…ript Replace assert.ok(x.includes(y)) with expectIncludes for better error messages on failure. Add back test:watch script that was accidentally removed during migration. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
node --experimental-strip-types --test --test-force-exit../src/) to dist imports (../dist/)import typefor type-only imports (TypeScript strip-types compatibility)__dirnamewithimport.meta.dirnameTest plan