Merge 93dd0cf into 1cba4d6

mbland · Jan 16, 2024 · 612c674 · 612c674
2 parents 1cba4d6 + 93dd0cf
commit 612c674
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 26 deletions.
diff --git a/ci/vitest.config.browser.js b/ci/vitest.config.browser.js
@@ -5,7 +5,6 @@ export default mergeConfig(baseConfig, defineConfig({
   test: {
     outputFile: 'TESTS-TestSuites-browser.xml',
     coverage: {
-      provider: 'istanbul',
       reportsDirectory: 'coverage-browser'
     },
     browser: {

diff --git a/ci/vitest.config.js b/ci/vitest.config.js
@@ -7,6 +7,7 @@ export default mergeConfig(viteConfig, defineConfig({
     reporters: [ 'junit', 'default' ],
     coverage: {
       enabled: true,
+      provider: 'istanbul',
       reporter: [ 'text', 'lcovonly' ],
       reportsDirectory: 'coverage-jsdom'
     }

diff --git a/test-modules/missing.html b/test-modules/missing.html
@@ -7,10 +7,7 @@
     <meta charset="UTF-8" />
     <meta name="viewport" content="width=device-width, initial-scale=1.0" />
     <title>Test Page for JsdomPageOpener missing an app div</title>
-    <!-- Provide a query suffix so import from index.html doesn't get reused and
-       - cause "JsdomPageOpener > doesn't throw if missing app div" to fail.
-       - https://github.com/nodejs/help/issues/2751#issuecomment-1075535742 -->
-    <script type="module" src="./main.js?version=missing"></script>
+    <script type="module" src="./main.js"></script>
   </head>
   <body>
     <div id="not-the-div-you're-looking-for"></div>

diff --git a/test/jsdom-fixture.js b/test/jsdom-fixture.js
@@ -0,0 +1,15 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+export default class JsdomFixture {
+  static #origWindow = globalThis.window
+  static #origDocument = globalThis.document
+
+  restoreOrigWindowAndDocument() {
+    globalThis.window = JsdomFixture.#origWindow
+    globalThis.document = JsdomFixture.#origDocument
+  }
+}
diff --git a/test/jsdom.test.js b/test/jsdom.test.js
@@ -6,22 +6,17 @@
  */
 
 import JsdomPageOpener from '../lib/jsdom.js'
-import { afterEach, beforeAll, describe, expect, test, vi } from 'vitest'
-
-describe.skipIf(globalThis.window !== undefined)('JsdomPageOpener', () => {
-  const origWindow = globalThis.window
-  const origDocument = globalThis.document
+import JsdomFixture from './jsdom-fixture.js'
+import { afterEach, beforeAll, describe, expect, test } from 'vitest'
 
+describe.skipIf(globalThis.window)('JsdomPageOpener', () => {
   /** @type {JsdomPageOpener} */
   let opener
+  const fixture = new JsdomFixture()
 
   beforeAll(async () => {opener = new JsdomPageOpener(await import('jsdom'))})
 
-  afterEach(() => {
-    globalThis.window = origWindow
-    globalThis.document = origDocument
-    vi.restoreAllMocks()
-  })
+  afterEach(fixture.restoreOrigWindowAndDocument)
 
   test('restores original globalThis.{window,document}', async () => {
     const pagePath = './test-modules/index.html'
@@ -50,15 +45,4 @@ describe.skipIf(globalThis.window !== undefined)('JsdomPageOpener', () => {
       expect(err.cause.cause.message).toBe('test error')
     })
   })
-
-  test('doesn\'t throw if missing app div', async () => {
-    const pagePath = './test-modules/missing.html'
-    const consoleSpy = vi.spyOn(console, 'error')
-      .mockImplementationOnce(() => {})
-
-    const { close } = await opener.open('/basedir/', pagePath)
-    close()
-
-    expect(consoleSpy).toBeCalledWith('no #app element')
-  })
 })
diff --git a/test/missing-app-div/jsdom.test.js b/test/missing-app-div/jsdom.test.js
@@ -0,0 +1,57 @@
+/* eslint-env browser */
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+import JsdomPageOpener from '../../lib/jsdom.js'
+import JsdomFixture from '../jsdom-fixture.js'
+import { afterEach, beforeAll, describe, expect, test, vi } from 'vitest'
+
+// This is in a separate directory from the other tests in ../jsdom.test.js. to
+// prevent Node.js from using the same `test-modules/main.js` import. Otherwise:
+//
+// - If the test case were in the same file, the "missing #app div" branch
+//   wouldn't execute and the test would fail.
+//
+// - If this test file were in the same directory, the Istanbul coverage
+//   reporter wouldn't see the coverage from the "missing #app div" branch. I
+//   don't know exactly why that is.
+//
+// At the same time, the previous `src="./main.js?version=missing"` query suffix
+// is no longer necessary.
+//
+// This solves the coverage drop from:
+//
+// - mbland/test-page-opener#23
+//   mbland/test-page-opener@01a79f6
+//
+// I got the idea to organize the tests this way after successfully covering
+// similar code in:
+//
+// - mbland/tomcat-servlet-testing-example#85
+//   mbland/tomcat-servlet-testing-example@b5df30e
+describe.skipIf(globalThis.window)('JsdomPageOpener', () => {
+  /** @type {JsdomPageOpener} */
+  let opener
+  const fixture = new JsdomFixture()
+
+  beforeAll(async () => {opener = new JsdomPageOpener(await import('jsdom'))})
+
+  afterEach(() => {
+    fixture.restoreOrigWindowAndDocument()
+    vi.restoreAllMocks()
+  })
+
+  test('logs error if missing #app div', async () => {
+    const pagePath = './test-modules/missing.html'
+    const consoleSpy = vi.spyOn(console, 'error')
+      .mockImplementationOnce(() => {})
+
+    const { close } = await opener.open('/basedir/', pagePath)
+    close()
+
+    expect(consoleSpy).toBeCalledWith('no #app element')
+  })
+})