Replace test/page-loader.js with test-page-opener #83
Conversation
This file contains 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
Required adding test-page-opener to the test.server.deps.inline config variable. Also updated all internal import paths to end with the '.js' extension. These changes were a consequence of the fact that the previous test/page-loader.js file was automatically bundled via Rollup, and the "external" test-page-opener isn't by default. Without bundling, test-page-opener can't resolve Handlebars modules transformed via rollup-plugin-handlebars-precompiler. --- On first adding test-page-opener to the project, running main.test.js in the browser passed. When running it in Node, all of the internal imports that didn't end with '.js' failed. This followed on the heels of mbland/test-page-opener#20, which I created when test-page-opener's own internal imports failed when used in this project. (I also filed mbland/test-page-opener#21 when its internal istanbul-lib-coverage import broke in this project, too.) After updating all the local imports on .js files, the Handlebars imports (ending in '.hbs') continued to fail with ERR_UNKNOWN_FILE_EXTENSION: ```sh % pnpm test -- run main.test.js RUN v1.1.3 /.../tomcat-servlet-testing-example/strcalc/src/main/frontend ❯ main.test.js (1) ❯ String Calculator UI on initial page load (1) × contains the "Hello, World!" placeholder —— Failed Tests 1 —— FAIL main.test.js > String Calculator UI on initial page load > contains the "Hello, World!" placeholder Error: opening index.html ❯ JsdomPageOpener.open node_modules/.pnpm/[email protected]/node_modules/test-page-opener/lib/jsdom.js:85:13 ❯ TestPageOpener.open node_modules/.pnpm/[email protected]/node_modules/test-page-opener/index.js:109:18 ❯ main.test.js:18:26 16| 17| test('contains the "Hello, World!" placeholder', async () => { 18| const { document } = await opener.open('index.html') | ^ 19| const appElem = document.querySelector('#app') 20| Caused by: Error: importing file:///.../tomcat-servlet-testing-example/strcalc/src/main/frontend/main.js ❯ node_modules/.pnpm/[email protected]/node_modules/test-page-opener/lib/jsdom.js:173:25 ❯ importModulesOnEvent node_modules/.pnpm/[email protected]/node_modules/test-page-opener/lib/jsdom.js:131:15 Caused by: TypeError: Unknown file extension ".hbs" for /.../mbland/tomcat-servlet-testing-example/strcalc/src/main/frontend/components/calculator.hbs ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Serialized Error: { code: 'ERR_UNKNOWN_FILE_EXTENSION' } ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ ``` (I'm rather relieved to have implemented mbland/test-page-opener#14 just today, which is why there are such nice "Caused by:" messages above.) When I temporarily replaced the npm dependency with my local working copy via `pnpm link ../../../../../test-page-opener`, the test would pass again. It occurred to me that the Node runs of the test weren't including test-page-opener in the Rollup bundle containing the transformed .hbs files. Or, it would include it when it was linked to the local copy, but not include it when installed normally. I eventually figured out that the Vitest config setting server.deps.inline would cause the "external" test-page-opener to get bundled, enabling it to resolve the '.hbs' imports. - <https://vitest.dev/config/#server-deps-inline>
mbland
added a commit
to mbland/test-page-opener
that referenced
this pull request
Jan 9, 2024
This took a lot of little changes and additional `@type` comments here and there. But I think it actually did make the code a bit better in a couple of key places. Also updated the README to include directions on including `test-page-opener` directly in a test bundle. See mbland/tomcat-servlet-testing-example#83 and commit 4c1bdb8985bfdc336eb49407bb5fe7b8dad082e6. Changes include: - Added `settings.jsdoc.preferredTypes.Object = "object"` to .eslintrc to enable "Object.<..., ...>" syntax in a JSDoc `@typedef`. Got rid of some extra whitespaces in .eslintrc, too. - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/rules/check-types.md#why-not-capital-case-everything - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/settings.md#settings-to-configure-check-types-and-no-undefined-types - Updated jsconfig.json to specify: ```json "lib": [ "ES2022" ], "module": "node16", "target": "es2020", "strict": true, "exclude": [ "node_modules/**", "coverage*/**", "jsdoc/**" ] ``` The "lib", "modules", and "target" lines are to ensure compatibility with Node v18, and there's no more disabling `strictNullChecks` and `noImplicitAny` after `"strict": true`. Most of the changes in this commit are a result of removing those two options. - Added the jsdoc-plugin-intersection JSDoc plugin to prevent TypeScript intersection types from breaking `pnpm jsdoc`. I needed to use an intersection type in the `@typedef` for `CovWindow` to fix the `window` types when setting the Istanbul coverage map. Neither the JSDoc `@extends` tag or a union type (`|`) would do. - https://www.npmjs.com/package/jsdoc-plugin-intersection - https://stackoverflow.com/questions/36737921/how-to-extend-a-typedef-parameter-in-jsdoc - jsdoc/jsdoc#1285 - Defined `@typedef CovWindow` to eliminate warnings in the `BrowserPageOpener` code for creating and merging coverage maps. - Added a check for `window.open()` returning `null` in `BrowserPageOpener.open()`, along with a new test covering this case. - Defined `@typedef JSDOM` to eliminate warnings in `JsdomPageOpener`. - Restored globalThis.{document,window} instead of deleting them, and added a test to validate this. This also fixed a subtle bug whereby calling `reject(err)` previously allowed the rest of the function to continue. (Thanks for forcing me to look at this, TypeScript!) - Added @types/{chai,istanbul-lib-coverage,jsdom} to devDependencies and added a `pnpm typecheck` command. Now the whole project and its dependencies pass strict type checking. - Updated everything under test/ and test-modules/ to be strictly TypeScript compiliant. - Some "TestPageOpener > loads page with module successfully" assertions had to be extended with `|| {}`. This is because TypeScript doesn't recognize the `expect(...).not.toBeNull()` expression as a null check. - Added a new missing.html and "JsdomPageOpener > doesn't throw if missing app div" test case to cover new null check in test-modules/main.js. This did, however, throw off Istanbul coverage, but not V8 coverage. Running just the "doesn't throw" test case shows 0% coverage of main.js, even though the test clearly passes. My suspicion is that Istanbul can't associate the `./main.js?version=missing` import path from missing.html with the test-modules/main.js file. So now `pnpm test:ci:jsdom` will use the V8 provider, and `pnpm test:ci:browser`, which doesn't use missing.html, will continue to use Istanbul. Each task outputs its own separate .lcov file which then gets merged into Coveralls. - Updated `pnpm test:ci` to incorporate `pnpm jsdoc` and `pnpm typecheck`. - Other little style cleanups sprinkled here and there. --- Normally I'd prefer not to commit a change this large at once, and I'd likely ask someone else to break it up. Then again, each of these changes is so small and understandable, and they're thematically related to one another. Plus, the total change isn't that long. Hence, I've rationalized breaking my own rule in this instance.
mbland
added a commit
to mbland/test-page-opener
that referenced
this pull request
Jan 9, 2024
This took a lot of little changes and additional `@type` comments here and there. But I think it actually did make the code a bit better in a couple of key places. Also updated the README to include directions on including `test-page-opener` directly in a test bundle. See mbland/tomcat-servlet-testing-example#83 and commit mbland/tomcat-servlet-testing-example@4c1bdb8. Changes include: - Added `settings.jsdoc.preferredTypes.Object = "object"` to .eslintrc to enable "Object.<..., ...>" syntax in a JSDoc `@typedef`. Got rid of some extra whitespaces in .eslintrc, too. - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/rules/check-types.md#why-not-capital-case-everything - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/settings.md#settings-to-configure-check-types-and-no-undefined-types - Updated jsconfig.json to specify: ```json "lib": [ "ES2022" ], "module": "node16", "target": "es2020", "strict": true, "exclude": [ "node_modules/**", "coverage*/**", "jsdoc/**" ] ``` The "lib", "modules", and "target" lines are to ensure compatibility with Node v18, and there's no more disabling `strictNullChecks` and `noImplicitAny` after `"strict": true`. Most of the changes in this commit are a result of removing those two options. - Added the jsdoc-plugin-intersection JSDoc plugin to prevent TypeScript intersection types from breaking `pnpm jsdoc`. I needed to use an intersection type in the `@typedef` for `CovWindow` to fix the `window` types when setting the Istanbul coverage map. Neither the JSDoc `@extends` tag or a union type (`|`) would do. - https://www.npmjs.com/package/jsdoc-plugin-intersection - https://stackoverflow.com/questions/36737921/how-to-extend-a-typedef-parameter-in-jsdoc - jsdoc/jsdoc#1285 - Defined `@typedef CovWindow` to eliminate warnings in the `BrowserPageOpener` code for creating and merging coverage maps. - Added a check for `window.open()` returning `null` in `BrowserPageOpener.open()`, along with a new test covering this case. - Defined `@typedef JSDOM` to eliminate warnings in `JsdomPageOpener`. - Restored globalThis.{document,window} instead of deleting them, and added a test to validate this. This also fixed a subtle bug whereby calling `reject(err)` previously allowed the rest of the function to continue. (Thanks for forcing me to look at this, TypeScript!) - Added @types/{chai,istanbul-lib-coverage,jsdom} to devDependencies and added a `pnpm typecheck` command. Now the whole project and its dependencies pass strict type checking. - Updated everything under test/ and test-modules/ to be strictly TypeScript compiliant. - Some "TestPageOpener > loads page with module successfully" assertions had to be extended with `|| {}`. This is because TypeScript doesn't recognize the `expect(...).not.toBeNull()` expression as a null check. - Added a new missing.html and "JsdomPageOpener > doesn't throw if missing app div" test case to cover new null check in test-modules/main.js. This did, however, throw off Istanbul coverage, but not V8 coverage. Running just the "doesn't throw" test case shows 0% coverage of main.js, even though the test clearly passes. My suspicion is that Istanbul can't associate the `./main.js?version=missing` import path from missing.html with the test-modules/main.js file. So now `pnpm test:ci:jsdom` will use the V8 provider, and `pnpm test:ci:browser`, which doesn't use missing.html, will continue to use Istanbul. Each task outputs its own separate .lcov file which then gets merged into Coveralls. - Updated `pnpm test:ci` to incorporate `pnpm jsdoc` and `pnpm typecheck`. - Other little style cleanups sprinkled here and there. --- Normally I'd prefer not to commit a change this large at once, and I'd likely ask someone else to break it up. Then again, each of these changes is so small and understandable, and they're thematically related to one another. Plus, the total change isn't that long. Hence, I've rationalized breaking my own rule in this instance.
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.
Required adding test-page-opener to the test.server.deps.inline config variable. Also updated all internal import paths to end with the '.js' extension.
These changes were a consequence of the fact that the previous test/page-loader.js file was automatically bundled via Rollup, and the "external" test-page-opener isn't by default. Without bundling, test-page-opener can't resolve Handlebars modules transformed via rollup-plugin-handlebars-precompiler.
On first adding test-page-opener to the project, running main.test.js in the browser passed. When running it in Node, all of the internal imports that didn't end with '.js' failed. This followed on the heels of mbland/test-page-opener#20, which I created when test-page-opener's own internal imports failed when used in this project. (I also filed mbland/test-page-opener#21 when its internal istanbul-lib-coverage import broke in this project, too.)
After updating all the local imports on .js files, the Handlebars imports (ending in '.hbs') continued to fail with
ERR_UNKNOWN_FILE_EXTENSION:
(I'm rather relieved to have implemented mbland/test-page-opener#14 just today, which is why there are such nice "Caused by:" messages above.)
When I temporarily replaced the npm dependency with my local working copy via
pnpm link ../../../../../test-page-opener, the test would pass again.It occurred to me that the Node runs of the test weren't including test-page-opener in the Rollup bundle containing the transformed .hbs files. Or, it would include it when it was linked to the local copy, but not include it when installed normally.
I eventually figured out that the Vitest config setting server.deps.inline would cause the "external" test-page-opener to get bundled, enabling it to resolve the '.hbs' imports.