From beb19143fb8de4b4fc8aeb3c6c9899906e193c90 Mon Sep 17 00:00:00 2001 From: Caleb Eby Date: Fri, 26 Feb 2021 15:03:02 -0800 Subject: [PATCH] Handle forgotten await (#43) --- .changeset/heavy-emus-walk.md | 34 ++++++++++++ src/extend-expect.ts | 43 +++++++++++---- src/index.ts | 100 +++++++++++++++++++++++----------- src/pptr-testing-library.ts | 45 ++++++++++----- src/user.ts | 80 ++++++++++++++++----------- tests/forgot-await.test.ts | 98 +++++++++++++++++++++++++++++++++ tests/test-utils.ts | 4 +- 7 files changed, 312 insertions(+), 92 deletions(-) create mode 100644 .changeset/heavy-emus-walk.md create mode 100644 tests/forgot-await.test.ts diff --git a/.changeset/heavy-emus-walk.md b/.changeset/heavy-emus-walk.md new file mode 100644 index 00000000..2846777d --- /dev/null +++ b/.changeset/heavy-emus-walk.md @@ -0,0 +1,34 @@ +--- +'test-mule': minor +--- + +Provide a helpful message if the user forgets to use `await`. + +For example, if a user forgets to use `await` in the jest-dom assertion: + +```js +test( + 'example', + withBrowser(async ({ screen, utils }) => { + await utils.injectHTML(''); + const button = await screen.getByText(/hi/i); + expect(button).toBeVisible(); + }), +); +``` + +Then a useful error message is produced: + +``` +Cannot execute assertion toBeVisible after test finishes. Did you forget to await? + + 103 | await utils.injectHTML(''); + 104 | const button = await screen.getByText(/hi/i); +> 105 | expect(button).toBeVisible(); + | ^ + 106 | }), + 107 | ); + 108 | +``` + +This is also handled for utility functions, user methods, and Testing Library queries. diff --git a/src/extend-expect.ts b/src/extend-expect.ts index 72c25c0e..cb2c2420 100644 --- a/src/extend-expect.ts +++ b/src/extend-expect.ts @@ -1,6 +1,6 @@ import type { ElementHandle, JSHandle } from 'puppeteer'; import { deserialize, serialize } from './serialize'; -import { jsHandleToArray } from './utils'; +import { jsHandleToArray, removeFuncFromStackTrace } from './utils'; import { port } from './vite-server'; const methods = [ @@ -91,14 +91,30 @@ Received ${this.utils.printReceived(arg)}`, throw error; } } + + const forgotAwait = removeFuncFromStackTrace( + new Error( + `Cannot execute assertion ${methodName} after test finishes. Did you forget to await?`, + ), + matcher, + ); + /** Handle error case for Target Closed error (forgot to await) */ + const handleExecutionAfterTestFinished = (error: any) => { + if (/target closed/i.test(error.message)) { + throw forgotAwait; + } + throw error; + }; + const ctxString = JSON.stringify(this); // contains stuff like isNot and promise - const result = await elementHandle.evaluateHandle( - // using new Function to avoid babel transpiling the import - // @ts-ignore - new Function( - 'element', - '...matcherArgs', - `return import("http://localhost:${port}/@test-mule/jest-dom") + const result = await elementHandle + .evaluateHandle( + // using new Function to avoid babel transpiling the import + // @ts-ignore + new Function( + 'element', + '...matcherArgs', + `return import("http://localhost:${port}/@test-mule/jest-dom") .then(({ jestContext, deserialize, ...jestDom }) => { const context = { ...(${ctxString}), ...jestContext } try { @@ -110,10 +126,13 @@ Received ${this.utils.printReceived(arg)}`, return { thrown: true, error } } })`, - ), - elementHandle, - ...matcherArgs.map((arg) => (isJSHandle(arg) ? arg : serialize(arg))), - ); + ), + elementHandle, + ...matcherArgs.map((arg) => + isJSHandle(arg) ? arg : serialize(arg), + ), + ) + .catch(handleExecutionAfterTestFinished); // Whether the matcher threw (this is different from the matcher failing) // The matcher failing means that it returned a result for Jest to throw diff --git a/src/index.ts b/src/index.ts index 5393f672..c06de38c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,7 +14,7 @@ import { fileURLToPath } from 'url'; koloristOpts.enabled = true; const ansiRegex = _ansiRegex({ onlyFirst: true }); import { testMuleUser, TestMuleUser } from './user'; -import { assertElementHandle } from './utils'; +import { assertElementHandle, removeFuncFromStackTrace } from './utils'; export type { TestMuleUser }; export interface TestMuleUtils { @@ -101,8 +101,17 @@ export const withBrowser: WithBrowser = (...args: any[]) => { const testPath = testFile ? path.relative(process.cwd(), testFile) : thisFile; return async () => { - const ctx = await createTab({ testPath, options }); - await Promise.resolve(testFn(ctx)).catch(async (error) => { + const { state, ...ctx } = await createTab({ testPath, options }); + const cleanup = async (leaveOpen: boolean) => { + state.isTestFinished = true; + if (!leaveOpen || options.headless) { + await ctx.page.close(); + } + ctx.page.browser().disconnect(); + }; + try { + await testFn(ctx); + } catch (error) { const messageForBrowser: undefined | unknown[] = // this is how we attach the elements to the error from testing-library error?.messageForBrowser || @@ -138,13 +147,10 @@ export const withBrowser: WithBrowser = (...args: any[]) => { console.log(...colorErr); }, ...(ansiColorsLog(...failureMessage) as any)); } - if (options.headless) await ctx.page.close(); - ctx.page.browser().disconnect(); + await cleanup(true); throw error; - }); - // close since test passed - await ctx.page.close(); - ctx.page.browser().disconnect(); + } + await cleanup(false); }; }; @@ -184,7 +190,9 @@ const createTab = async ({ }: { testPath: string; options: WithBrowserOpts; -}): Promise => { +}): Promise => { + /** Used to provide helpful warnings if things execute after the test finishes (usually means they forgot to await) */ + const state = { isTestFinished: false }; if (!serverPromise) serverPromise = createServer(); const browser = await connectToBrowser('chromium', headless); const browserContext = await browser.createIncognitoBrowserContext(); @@ -234,22 +242,38 @@ const createTab = async ({ await page.goto(`http://localhost:${port}`); + const safeEvaluate = async ( + caller: (...params: any) => any, + ...args: Parameters + ) => { + const forgotAwaitError = removeFuncFromStackTrace( + new Error( + `Cannot interact with browser using ${caller.name} after test finishes. Did you forget to await?`, + ), + caller, + ); + return await page.evaluate(...args).catch((error) => { + if (state.isTestFinished && /target closed/i.test(error.message)) { + throw forgotAwaitError; + } + throw error; + }); + }; + const runJS: TestMuleUtils['runJS'] = async (code) => { const encodedCode = encodeURIComponent(code); // This uses the testPath as the url so that if there are relative imports // in the inline code, the relative imports are resolved relative to the test file const url = `http://localhost:${port}/${testPath}?inline-code=${encodedCode}`; - const evalResult = await page.evaluateHandle( + const res = (await safeEvaluate( + runJS, `import(${JSON.stringify(url)}) - .then(m => {}) - .catch(e => - e instanceof Error - ? { message: e.message, stack: e.stack } - : e)`, - ); - const res = (await evalResult.jsonValue()) as - | undefined - | { message: string; stack: string }; + .then(m => {}) + .catch(e => + e instanceof Error + ? { message: e.message, stack: e.stack } + : e)`, + )) as undefined | { message: string; stack: string }; if (res === undefined) return; if (typeof res !== 'object') throw res; const { message, stack } = res; @@ -316,24 +340,33 @@ const createTab = async ({ }; const injectHTML: TestMuleUtils['injectHTML'] = async (html) => { - await page.evaluate((html) => { - document.body.innerHTML = html; - }, html); + await safeEvaluate( + injectHTML, + (html) => { + document.body.innerHTML = html; + }, + html, + ); }; const injectCSS: TestMuleUtils['injectCSS'] = async (css) => { - await page.evaluate((css) => { - const styleTag = document.createElement('style'); - styleTag.innerHTML = css; - document.head.append(styleTag); - }, css); + await safeEvaluate( + injectCSS, + (css) => { + const styleTag = document.createElement('style'); + styleTag.innerHTML = css; + document.head.append(styleTag); + }, + css, + ); }; const loadCSS: TestMuleUtils['loadCSS'] = async (cssPath) => { const fullPath = cssPath.startsWith('.') ? path.join(path.dirname(testPath), cssPath) : cssPath; - await page.evaluateHandle( + await safeEvaluate( + loadCSS, `import(${JSON.stringify( `http://localhost:${port}/${fullPath}?import`, )})`, @@ -344,7 +377,8 @@ const createTab = async ({ const fullPath = jsPath.startsWith('.') ? path.join(path.dirname(testPath), jsPath) : jsPath; - await page.evaluateHandle( + await safeEvaluate( + loadJS, `import(${JSON.stringify(`http://localhost:${port}/${fullPath}`)})`, ); }; @@ -357,17 +391,17 @@ const createTab = async ({ loadJS, }; - const screen = getQueriesForElement(page); + const screen = getQueriesForElement(page, state); // the | null is so you can pass directly the result of page.$() which returns null if not found const within: TestMuleContext['within'] = ( element: puppeteer.ElementHandle | null, ) => { assertElementHandle(element, within, 'within(el)', 'el'); - return getQueriesForElement(page, element); + return getQueriesForElement(page, state, element); }; - return { screen, utils, page, within, user: testMuleUser() }; + return { screen, utils, page, within, user: testMuleUser(state), state }; }; afterAll(async () => { diff --git a/src/pptr-testing-library.ts b/src/pptr-testing-library.ts index 03b70114..583c5fd8 100644 --- a/src/pptr-testing-library.ts +++ b/src/pptr-testing-library.ts @@ -1,6 +1,6 @@ import { port } from './vite-server'; import type { queries, BoundFunctions } from '@testing-library/dom'; -import { jsHandleToArray } from './utils'; +import { jsHandleToArray, removeFuncFromStackTrace } from './utils'; import type { JSHandle } from 'puppeteer'; type ElementToElementHandle = Input extends Element @@ -80,6 +80,7 @@ export interface BoundQueries extends BoundFunctions {} export const getQueriesForElement = ( page: import('puppeteer').Page, + state: { isTestFinished: boolean }, element?: import('puppeteer').ElementHandle, ) => { // @ts-expect-error @@ -96,15 +97,27 @@ export const getQueriesForElement = ( } return value; }); - const result: JSHandle< - Element | Element[] | DTLError - > = await page.evaluateHandle( - // using new Function to avoid babel transpiling the import - // @ts-expect-error - new Function( - 'argsString', - 'element', - `return import("http://localhost:${port}/@test-mule/dom-testing-library") + const forgotAwait = removeFuncFromStackTrace( + new Error( + `Cannot execute query ${queryName} after test finishes. Did you forget to await?`, + ), + query, + ); + /** Handle error case for Target Closed error (forgot to await) */ + const handleExecutionAfterTestFinished = (error: any) => { + if (/target closed/i.test(error.message) && state.isTestFinished) { + throw forgotAwait; + } + throw error; + }; + const result: JSHandle = await page + .evaluateHandle( + // using new Function to avoid babel transpiling the import + // @ts-expect-error + new Function( + 'argsString', + 'element', + `return import("http://localhost:${port}/@test-mule/dom-testing-library") .then(async ({ reviveElementsInString, printElement, addToElementCache, ...dtl }) => { const deserializedArgs = JSON.parse(argsString, (key, value) => { if (value.__serialized === 'RegExp') @@ -130,10 +143,14 @@ export const getQueriesForElement = ( return { failed: true, messageWithElementsRevived, messageWithElementsStringified } } })`, - ), - serializedArgs, - element?.asElement() || (await page.evaluateHandle(() => document)), - ); + ), + serializedArgs, + element?.asElement() || + (await page + .evaluateHandle(() => document) + .catch(handleExecutionAfterTestFinished)), + ) + .catch(handleExecutionAfterTestFinished); const failed = await result.evaluate( (r) => typeof r === 'object' && r !== null && (r as DTLError).failed, diff --git a/src/user.ts b/src/user.ts index de4f5d23..6f5ae90f 100644 --- a/src/user.ts +++ b/src/user.ts @@ -10,43 +10,59 @@ export interface TestMuleUser { click(element: ElementHandle | null): Promise; } -export const testMuleUser = () => { +export const testMuleUser = (state: { isTestFinished: boolean }) => { const user: TestMuleUser = { async click(el) { assertElementHandle(el, user.click, 'user.click(el)', 'el'); - const failed = await el.evaluateHandle((clickEl) => { - const clickElRect = clickEl.getBoundingClientRect(); + const forgotAwaitError = removeFuncFromStackTrace( + new Error( + `Cannot interact with browser using user.click after test finishes. Did you forget to await?`, + ), + user.click, + ); - // See if there is an element covering the center of the click target element - const coveringEl = document.elementFromPoint( - Math.floor(clickElRect.x + clickElRect.width / 2), - Math.floor(clickElRect.y + clickElRect.height / 2), - ); - if (coveringEl === clickEl || clickEl.contains(coveringEl)) return; - // TODO: try to find other points on the element that are clickable, - // in case the covering element does not cover the whole click-target element - const messagePart1 = - 'user.click(element)\n\nCould not click element:\n'; - const messagePart2 = '\n\nElement was covered by:\n'; - // TODO: instead of using .outerHTML use the html formatter - return { - message: - messagePart1 + - clickEl.outerHTML + - messagePart2 + - coveringEl!.outerHTML, - messageForBrowser: [messagePart1, clickEl, messagePart2, coveringEl], - }; - }); - - if (await failed.jsonValue()) { - const error = new Error( - (await failed - .getProperty('message') - .then((r) => r.jsonValue())) as any, - ); - // @ts-expect-error + const failed = await el + .evaluateHandle((clickEl) => { + const clickElRect = clickEl.getBoundingClientRect(); + + // See if there is an element covering the center of the click target element + const coveringEl = document.elementFromPoint( + Math.floor(clickElRect.x + clickElRect.width / 2), + Math.floor(clickElRect.y + clickElRect.height / 2), + ); + if (coveringEl === clickEl || clickEl.contains(coveringEl)) return; + // TODO: try to find other points on the element that are clickable, + // in case the covering element does not cover the whole click-target element + const messagePart1 = + 'user.click(element)\n\nCould not click element:\n'; + const messagePart2 = '\n\nElement was covered by:\n'; + // TODO: instead of using .outerHTML use the html formatter + return { + message: + messagePart1 + + clickEl.outerHTML + + messagePart2 + + coveringEl!.outerHTML, + messageForBrowser: [ + messagePart1, + clickEl, + messagePart2, + coveringEl, + ], + }; + }) + .catch((error) => { + if (state.isTestFinished && /target closed/i.test(error.message)) { + throw forgotAwaitError; + } + throw error; + }); + + const { message } = (await failed.jsonValue()) || ({} as any); + + if (message) { + const error = new Error(message) as any; error.messageForBrowser = await jsHandleToArray( await failed.getProperty('messageForBrowser'), ); diff --git a/tests/forgot-await.test.ts b/tests/forgot-await.test.ts new file mode 100644 index 00000000..2bbb6d4f --- /dev/null +++ b/tests/forgot-await.test.ts @@ -0,0 +1,98 @@ +import { withBrowser } from 'test-mule'; +import { printErrorFrames } from './test-utils'; + +test('forgot await in testing library query', (done) => { + withBrowser(async ({ screen }) => { + screen.getByText('hi').catch(async (error) => { + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: Cannot execute query getByText after test finishes. Did you forget to await? + ------------------------------------------------------- + tests/forgot-await.test.ts + + screen.getByText('hi').catch(async (error) => { + ^ + ------------------------------------------------------- + dist/cjs/index.cjs" + `); + done(); + }); + })(); +}); + +test('forgot await in jest dom assertion', (done) => { + withBrowser(async ({ screen, utils }) => { + await utils.injectHTML(''); + const button = await screen.getByText(/hi/i); + expect(button) + .toBeVisible() + .catch(async (error) => { + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: Cannot execute assertion toBeVisible after test finishes. Did you forget to await? + ------------------------------------------------------- + tests/forgot-await.test.ts + + .toBeVisible() + ^ + ------------------------------------------------------- + dist/cjs/index.cjs" + `); + done(); + }); + })(); +}); + +test('forgot await in utils.injectHTML', (done) => { + withBrowser(async ({ utils }) => { + utils.injectHTML('').catch(async (error) => { + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: Cannot interact with browser using injectHTML after test finishes. Did you forget to await? + ------------------------------------------------------- + tests/forgot-await.test.ts + + utils.injectHTML('').catch(async (error) => { + ^ + ------------------------------------------------------- + dist/cjs/index.cjs" + `); + done(); + }); + })(); +}); + +test('forgot await in utils.runJS', (done) => { + withBrowser(async ({ utils }) => { + utils.runJS('if (false) {}').catch(async (error) => { + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: Cannot interact with browser using runJS after test finishes. Did you forget to await? + ------------------------------------------------------- + tests/forgot-await.test.ts + + utils.runJS('if (false) {}').catch(async (error) => { + ^ + ------------------------------------------------------- + dist/cjs/index.cjs" + `); + done(); + }); + })(); +}); + +test('forgot await in user.click', (done) => { + withBrowser(async ({ user, utils, screen }) => { + await utils.injectHTML(''); + const button = await screen.getByText('Hi'); + user.click(button).catch(async (error) => { + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: Cannot interact with browser using user.click after test finishes. Did you forget to await? + ------------------------------------------------------- + tests/forgot-await.test.ts + + user.click(button).catch(async (error) => { + ^ + ------------------------------------------------------- + dist/cjs/index.cjs" + `); + done(); + }); + })(); +}); diff --git a/tests/test-utils.ts b/tests/test-utils.ts index 250539c4..0ad077fe 100644 --- a/tests/test-utils.ts +++ b/tests/test-utils.ts @@ -22,10 +22,12 @@ export const printErrorFrames = async (error: Error) => { ) { return stackFrame.raw; } + const relativePath = path.relative(process.cwd(), stackFrame.fileName); + if (relativePath.startsWith('dist/')) return relativePath; const file = await fs.readFile(stackFrame.fileName, 'utf8'); const line = file.split('\n')[stackFrame.line - 1]; return ( - path.relative(process.cwd(), stackFrame.fileName) + + relativePath + '\n\n' + line + '\n' +