From b944b7cea7e0722a24a85fabd4d97a2d70453305 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Wed, 9 Nov 2022 12:55:47 -0800 Subject: [PATCH] chore: remove assert flow (#4554) chore: update tests to handle no assert flow --- integration/error-boundary-test.ts | 11 +- integration/error-data-request-test.ts | 5 - integration/transition-test.ts | 4 +- .../__tests__/server-test.ts | 3 +- packages/remix-server-runtime/server.ts | 348 ++++++++---------- 5 files changed, 161 insertions(+), 210 deletions(-) diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts index 04b9bea72bc..d90ec588280 100644 --- a/integration/error-boundary-test.ts +++ b/integration/error-boundary-test.ts @@ -112,7 +112,7 @@ test.describe("ErrorBoundary", () => { throw new Error("Kaboom!") } export function ErrorBoundary() { - return

${OWN_BOUNDARY_TEXT}

+ return

${OWN_BOUNDARY_TEXT}

} export default function () { return ( @@ -146,7 +146,7 @@ test.describe("ErrorBoundary", () => { throw new Error("Kaboom!") } export function ErrorBoundary() { - return
${OWN_BOUNDARY_TEXT}
+ return
${OWN_BOUNDARY_TEXT}
} export default function () { return
@@ -176,7 +176,7 @@ test.describe("ErrorBoundary", () => { } export function ErrorBoundary() { - return
${OWN_BOUNDARY_TEXT}
+ return
${OWN_BOUNDARY_TEXT}
} `, @@ -305,6 +305,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickSubmitButton(HAS_BOUNDARY_ACTION); + await page.waitForSelector("#own-boundary"); expect(await app.getHtml("main")).toMatch(OWN_BOUNDARY_TEXT); }); @@ -314,6 +315,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto(HAS_BOUNDARY_ACTION); await app.clickSubmitButton(HAS_BOUNDARY_ACTION); + await page.waitForSelector("#own-boundary"); expect(await app.getHtml("main")).toMatch(OWN_BOUNDARY_TEXT); }); @@ -589,6 +591,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickSubmitButton(NO_ROOT_BOUNDARY_ACTION); + await page.waitForSelector(`text=${INTERNAL_ERROR_BOUNDARY_HEADING}`) expect(await app.getHtml("h1")).toMatch(INTERNAL_ERROR_BOUNDARY_HEADING); }); @@ -604,6 +607,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickLink(NO_ROOT_BOUNDARY_LOADER_RETURN); + await page.waitForSelector(`text=${INTERNAL_ERROR_BOUNDARY_HEADING}`) expect(await app.getHtml("h1")).toMatch(INTERNAL_ERROR_BOUNDARY_HEADING); }); @@ -621,6 +625,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickSubmitButton(NO_ROOT_BOUNDARY_ACTION_RETURN); + await page.waitForSelector(`text=${INTERNAL_ERROR_BOUNDARY_HEADING}`) expect(await app.getHtml("h1")).toMatch(INTERNAL_ERROR_BOUNDARY_HEADING); }); }); diff --git a/integration/error-data-request-test.ts b/integration/error-data-request-test.ts index dc0a7d9f1f7..456eee1ecd4 100644 --- a/integration/error-data-request-test.ts +++ b/integration/error-data-request-test.ts @@ -3,8 +3,6 @@ import { test, expect } from "@playwright/test"; import { createAppFixture, createFixture, js } from "./helpers/create-fixture"; import type { Fixture, AppFixture } from "./helpers/create-fixture"; -const ENABLE_REMIX_ROUTER = !!process.env.ENABLE_REMIX_ROUTER; - test.describe("ErrorBoundary", () => { let fixture: Fixture; let appFixture: AppFixture; @@ -105,9 +103,6 @@ test.describe("ErrorBoundary", () => { function assertConsoleError(str: string) { expect(errorLogs[0]).toEqual(str); - if (ENABLE_REMIX_ROUTER) { - expect(errorLogs[1]).toEqual(str); - } } test("returns a 405 x-remix-error on a data fetch to a path with no loader", async () => { diff --git a/integration/transition-test.ts b/integration/transition-test.ts index 63bd78d2eb9..6cf713b1c9e 100644 --- a/integration/transition-test.ts +++ b/integration/transition-test.ts @@ -216,8 +216,8 @@ test.describe("rendering", () => { await page.waitForLoadState("networkidle"); expect( - responses.map((res) => new URL(res.url()).searchParams.get("_data")) - ).toEqual([`routes/${PAGE}`, `routes/${PAGE}/index`]); + responses.map((res) => new URL(res.url()).searchParams.get("_data")).sort() + ).toEqual([`routes/${PAGE}`, `routes/${PAGE}/index`].sort()); await page.waitForSelector(`h2:has-text("${PAGE_TEXT}")`); await page.waitForSelector(`h3:has-text("${PAGE_INDEX_TEXT}")`); diff --git a/packages/remix-server-runtime/__tests__/server-test.ts b/packages/remix-server-runtime/__tests__/server-test.ts index 75024ca1620..ae7c4d2ce51 100644 --- a/packages/remix-server-runtime/__tests__/server-test.ts +++ b/packages/remix-server-runtime/__tests__/server-test.ts @@ -3,8 +3,7 @@ import { ServerMode } from "../mode"; import type { ServerBuild } from "../build"; import { mockServerBuild } from "./utils"; -const ENABLE_REMIX_ROUTER = !!process.env.ENABLE_REMIX_ROUTER; -const DATA_CALL_MULTIPIER = ENABLE_REMIX_ROUTER ? 2 : 1; +const DATA_CALL_MULTIPIER = 1; function spyConsole() { // https://github.com/facebook/react/issues/7047 diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 409f15e1e58..790d8289099 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -52,35 +52,19 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( let response: Response; if (url.searchParams.has("_data")) { - let responsePromise = handleDataRequest({ - request: - // We need to clone the request here instead of the call to the new - // handler otherwise the first handler will lock the body for the other. - // Cloning here allows the new handler to be the stream reader and delegate - // chunks back to this cloned request. - ENABLE_REMIX_ROUTER ? request.clone() : request, - loadContext, - matches: matches!, - serverMode, - }); - let routeId = url.searchParams.get("_data")!; - if (ENABLE_REMIX_ROUTER) { - let [response, remixRouterResponse] = await Promise.all([ - responsePromise, - handleDataRequestRR(serverMode, staticHandler!, routeId, request), - ]); - if (!request.signal.aborted) { - assertResponsesMatch(response, remixRouterResponse); - } - - console.log("Returning Remix Router Data Request Response"); - responsePromise = Promise.resolve(remixRouterResponse); + if (ENABLE_REMIX_ROUTER) { + response = await handleDataRequestRR(serverMode, staticHandler!, routeId, request); + } else { + response = await handleDataRequest({ + request, + loadContext, + matches: matches!, + serverMode, + }); } - response = await responsePromise; - if (build.entry.module.handleDataRequest) { let match = matches!.find((match) => match.route.id == routeId)!; response = await build.entry.module.handleDataRequest(response, { @@ -93,68 +77,34 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( matches && matches[matches.length - 1].route.module.default == null ) { - let responsePromise = handleResourceRequest({ - request: - // We need to clone the request here instead of the call to the new - // handler otherwise the first handler will lock the body for the other. - // Cloning here allows the new handler to be the stream reader and delegate - // chunks back to this cloned request. - ENABLE_REMIX_ROUTER ? request.clone() : request, - loadContext, - matches, - serverMode, - }); - if (ENABLE_REMIX_ROUTER) { - let [response, remixRouterResponse] = await Promise.all([ - responsePromise, - handleResourceRequestRR( - serverMode, - staticHandler!, - matches.slice(-1)[0].route.id, - request - ), - ]); - - if (!request.signal.aborted) { - assertResponsesMatch(response, remixRouterResponse); - } - - console.log("Returning Remix Router Resource Request Response"); - responsePromise = Promise.resolve(remixRouterResponse); + response = await handleResourceRequestRR( + serverMode, + staticHandler!, + matches.slice(-1)[0].route.id, + request + ); + } else { + response = await handleResourceRequest({ + request, + loadContext, + matches, + serverMode, + }); } - - response = await responsePromise; } else { - let responsePromise = handleDocumentRequest({ - build, - loadContext, - matches, - request: - // We need to clone the request here instead of the call to the new - // handler otherwise the first handler will lock the body for the other. - // Cloning here allows the new handler to be the stream reader and delegate - // chunks back to this cloned request. - ENABLE_REMIX_ROUTER ? request.clone() : request, - routes, - serverMode, - }); - if (ENABLE_REMIX_ROUTER) { - let [response, remixRouterResponse] = await Promise.all([ - responsePromise, - handleDocumentRequestRR(serverMode, build, staticHandler!, request), - ]); - - if (!request.signal.aborted) { - assertResponsesMatch(response, remixRouterResponse); - } - - console.log("Returning Remix Router Document Request Response"); - responsePromise = Promise.resolve(remixRouterResponse); + response = await handleDocumentRequestRR(serverMode, build, staticHandler!, request) + } else { + response = await handleDocumentRequest({ + build, + loadContext, + matches, + request, + routes, + serverMode, + }) } - - response = await responsePromise; } if (request.method === "HEAD") { @@ -1065,122 +1015,6 @@ function getRenderableMatches( return matches.slice(0, lastRenderableIndex + 1); } -async function assert( - a: Response, - b: Response, - accessor: (r: Response) => object | Promise, - message: string -) { - let aStr = JSON.stringify(await accessor(a)); - let bStr = JSON.stringify(await accessor(b)); - - if (aStr !== bStr) { - console.error(message); - message += "\nResponse 1:\n" + aStr + "\nResponse 2:\n" + bStr; - throw new Error(message); - } -} - -async function assertResponsesMatch(_a: Response, _b: Response) { - let a = _a.clone(); - let b = _b.clone(); - assert( - a, - b, - (r) => Object.fromEntries(r.headers.entries()), - "Headers did not match!" - ); - - if (a.headers.get("Content-Type")?.startsWith("application/json")) { - if (a.headers.get("X-Remix-Error")) { - assert( - a, - b, - async (r) => { - let { stack, ...json } = await r.json(); - return { - ...json, - stack: stack ? "yes" : "no", - }; - }, - "JSON error response body did not match!\n Response 1:\n" + - (await a.clone().text()) + - "\nResponse 2:\n" + - (await b.clone().text()) - ); - } else { - assert( - a, - b, - (r) => r.json(), - "JSON response body did not match!\nResponse 1:\n" + - (await a.clone().text()) + - "\nResponse 2:\n" + - (await b.clone().text()) - ); - } - } else { - assert( - a, - b, - async (r) => { - let text = await r.text(); - - // Strip stack trace from default error boundary HTML - if ( - text.includes(">Application Error") && - text.includes("💿 Hey developer👋") - ) { - return stripStackTraceFromDefaultErrorBoundary(text); - } - - if (text.includes("", scriptStart + 1); - let scriptContents = text.substring(scriptStart, scriptEnd + 9); - let remixContext = JSON.parse( - scriptContents - .replace("", "") - ); - // If we didn't have an error - assert the full document - if (!remixContext?.appState?.error?.stack) { - return text; - } - - // Otherwise remove the stack trace and assert the text/JSON as a - // concatenated string - remixContext.appState.error.stack = "yes"; - text = text.replace(scriptContents, "CONTEXT REMOVED"); - return text + "\n\nContext:\n" + JSON.stringify(remixContext); -} - function returnLastResortErrorResponse(error: any, serverMode?: ServerMode) { if (serverMode !== ServerMode.Test) { console.error(error); @@ -1200,3 +1034,121 @@ function returnLastResortErrorResponse(error: any, serverMode?: ServerMode) { }, }); } + + +// TODO: Remove before we "finalize" router migration +// async function assert( +// a: Response, +// b: Response, +// accessor: (r: Response) => object | Promise, +// message: string +// ) { +// let aStr = JSON.stringify(await accessor(a)); +// let bStr = JSON.stringify(await accessor(b)); + +// if (aStr !== bStr) { +// console.error(message); +// message += "\nResponse 1:\n" + aStr + "\nResponse 2:\n" + bStr; +// throw new Error(message); +// } +// } + +// async function assertResponsesMatch(_a: Response, _b: Response) { +// let a = _a.clone(); +// let b = _b.clone(); +// assert( +// a, +// b, +// (r) => Object.fromEntries(r.headers.entries()), +// "Headers did not match!" +// ); + +// if (a.headers.get("Content-Type")?.startsWith("application/json")) { +// if (a.headers.get("X-Remix-Error")) { +// assert( +// a, +// b, +// async (r) => { +// let { stack, ...json } = await r.json(); +// return { +// ...json, +// stack: stack ? "yes" : "no", +// }; +// }, +// "JSON error response body did not match!\n Response 1:\n" + +// (await a.clone().text()) + +// "\nResponse 2:\n" + +// (await b.clone().text()) +// ); +// } else { +// assert( +// a, +// b, +// (r) => r.json(), +// "JSON response body did not match!\nResponse 1:\n" + +// (await a.clone().text()) + +// "\nResponse 2:\n" + +// (await b.clone().text()) +// ); +// } +// } else { +// assert( +// a, +// b, +// async (r) => { +// let text = await r.text(); + +// // Strip stack trace from default error boundary HTML +// if ( +// text.includes(">Application Error") && +// text.includes("💿 Hey developer👋") +// ) { +// return stripStackTraceFromDefaultErrorBoundary(text); +// } + +// if (text.includes("", scriptStart + 1); +// let scriptContents = text.substring(scriptStart, scriptEnd + 9); +// let remixContext = JSON.parse( +// scriptContents +// .replace("", "") +// ); +// // If we didn't have an error - assert the full document +// if (!remixContext?.appState?.error?.stack) { +// return text; +// } + +// // Otherwise remove the stack trace and assert the text/JSON as a +// // concatenated string +// remixContext.appState.error.stack = "yes"; +// text = text.replace(scriptContents, "CONTEXT REMOVED"); +// return text + "\n\nContext:\n" + JSON.stringify(remixContext); +// }