From 1b32763ee8f13d8ba8d400b1545825d871ed3da2 Mon Sep 17 00:00:00 2001 From: uid11 Date: Sun, 3 Nov 2024 03:14:17 +0300 Subject: [PATCH] FI-1484 feat: rename `getParamsFromUrl` to `getParamsFromUrlOrThrow` fix: test timeout and interrupt timeout during `waitBeforeRetry` --- autotests/packs/allTests.ts | 6 ++- autotests/routes/apiRoutes/AddUser.ts | 2 +- autotests/routes/apiRoutes/CreateProduct.ts | 2 +- autotests/routes/pageRoutes/Search.ts | 2 +- .../internalTypeTests/mockApiRoute.skip.ts | 4 +- .../internalTypeTests/waitForEvents.skip.ts | 4 +- autotests/tests/main/exists.ts | 4 +- autotests/tests/mockApiRoute.ts | 2 +- scripts/writePrunedPackageJson.ts | 2 +- src/Route.ts | 2 +- src/actions/mock/mockApiRoute.ts | 4 +- src/types/routes.ts | 4 +- src/utils/fullMocks/FullMocksRoute.ts | 2 +- src/utils/getRouteInstanceFromUrl.ts | 8 ++-- src/utils/resourceUsage.ts | 8 ++-- src/utils/retry/runPackWithRetries.ts | 2 +- src/utils/test/beforeTest.ts | 6 +-- src/utils/test/getPreviousRunId.ts | 37 ++++++++++++++++ src/utils/test/waitBeforeRetry.ts | 43 +++++++++---------- 19 files changed, 89 insertions(+), 55 deletions(-) create mode 100644 src/utils/test/getPreviousRunId.ts diff --git a/autotests/packs/allTests.ts b/autotests/packs/allTests.ts index eefbc47e..bac8a1f8 100644 --- a/autotests/packs/allTests.ts +++ b/autotests/packs/allTests.ts @@ -33,8 +33,10 @@ const filterTestsIntoPack: FilterTestsIntoPack = ({options}) => options.meta.tes const userAgent = 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.35 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.35'; -const packTimeoutInMinutes = 5; const msInMinute = 60_000; +const packTimeoutInMinutes = 5; + +const waitBeforeRetryTimeout = 1_000; /** * Pack of tests or tasks (pack configuration object). @@ -82,7 +84,7 @@ export const pack: Pack = { userAgent, viewportHeight: 1080, viewportWidth: 1920, - waitBeforeRetry: () => 0, + waitBeforeRetry: () => waitBeforeRetryTimeout, waitForAllRequestsComplete: { maxIntervalBetweenRequestsInMs: 500, timeout: 30_000, diff --git a/autotests/routes/apiRoutes/AddUser.ts b/autotests/routes/apiRoutes/AddUser.ts index 23bb290d..643b0581 100644 --- a/autotests/routes/apiRoutes/AddUser.ts +++ b/autotests/routes/apiRoutes/AddUser.ts @@ -12,7 +12,7 @@ const pathStart = '/api/users'; * Client API route for adding user-worker. */ export class AddUser extends ApiRoute { - static override getParamsFromUrl(url: Url): Params { + static override getParamsFromUrlOrThrow(url: Url): Params { const urlObject = new URL(url); assertValueIsTrue( diff --git a/autotests/routes/apiRoutes/CreateProduct.ts b/autotests/routes/apiRoutes/CreateProduct.ts index 9dd9889d..b7b0c338 100644 --- a/autotests/routes/apiRoutes/CreateProduct.ts +++ b/autotests/routes/apiRoutes/CreateProduct.ts @@ -18,7 +18,7 @@ export class CreateProduct extends ApiRoute< ApiCreateProductRequest, ApiCreateProductResponse > { - static override getParamsFromUrl(url: Url): Params { + static override getParamsFromUrlOrThrow(url: Url): Params { const urlObject = new URL(url); assertValueIsTrue( diff --git a/autotests/routes/pageRoutes/Search.ts b/autotests/routes/pageRoutes/Search.ts index 27cd282c..f8bae3da 100644 --- a/autotests/routes/pageRoutes/Search.ts +++ b/autotests/routes/pageRoutes/Search.ts @@ -11,7 +11,7 @@ type Params = Readonly<{searchQuery: string}> | undefined; * Route of the Search page. */ export class Search extends PageRoute { - static override getParamsFromUrl(url: Url): Params { + static override getParamsFromUrlOrThrow(url: Url): Params { const {pathname, searchParams} = new URL(url); assertValueIsTrue(pathname === '/search', 'search route matches on url', {url}); diff --git a/autotests/tests/internalTypeTests/mockApiRoute.skip.ts b/autotests/tests/internalTypeTests/mockApiRoute.skip.ts index 7abe5762..6c88eaa0 100644 --- a/autotests/tests/internalTypeTests/mockApiRoute.skip.ts +++ b/autotests/tests/internalTypeTests/mockApiRoute.skip.ts @@ -35,10 +35,10 @@ void mockApiRoute(Main, apiMockFunction); // @ts-expect-error: unmockApiRoute require API route as first argument void unmockApiRoute(Main); -// @ts-expect-error: mockApiRoute require API route with static method getParamsFromUrl +// @ts-expect-error: mockApiRoute require API route with static method getParamsFromUrlOrThrow void mockApiRoute(CreateDevice, apiMockFunction); -// @ts-expect-error: unmockApiRoute require API route with static method getParamsFromUrl +// @ts-expect-error: unmockApiRoute require API route with static method getParamsFromUrlOrThrow void unmockApiRoute(CreateDevice); // ok diff --git a/autotests/tests/internalTypeTests/waitForEvents.skip.ts b/autotests/tests/internalTypeTests/waitForEvents.skip.ts index 03e96e42..c4832401 100644 --- a/autotests/tests/internalTypeTests/waitForEvents.skip.ts +++ b/autotests/tests/internalTypeTests/waitForEvents.skip.ts @@ -63,7 +63,7 @@ void waitForRequestToRoute(AddUser, ({delay}, {requestBody, url}) => { request.requestBody.job === 'foo' && 'delay' in routeParams && routeParams.delay > 0, ); -// @ts-expect-error: waitForRequestToRoute does not accept routes without `getParamsFromUrl` method +// @ts-expect-error: waitForRequestToRoute does not accept routes without `getParamsFromUrlOrThrow` method void waitForRequestToRoute(GetUser); // ok @@ -84,5 +84,5 @@ void waitForResponseToRoute(AddUser, ({delay}, {responseBody, request: {requestB routeParams.delay > 0, ); -// @ts-expect-error: waitForResponseToRoute does not accept routes without `getParamsFromUrl` method +// @ts-expect-error: waitForResponseToRoute does not accept routes without `getParamsFromUrlOrThrow` method void waitForResponseToRoute(GetUser); diff --git a/autotests/tests/main/exists.ts b/autotests/tests/main/exists.ts index 1f1d0d16..43507316 100644 --- a/autotests/tests/main/exists.ts +++ b/autotests/tests/main/exists.ts @@ -77,7 +77,7 @@ test('exists', {meta: {testId: '1'}, testIdleTimeout: 10_000, testTimeout: 15_00 const searchPage = await assertPage(Search, {searchQuery}); /** - * Do not use the following pageParams and url (by getParamsFromUrl) checks in your code. + * Do not use the following pageParams and url (by getParamsFromUrlOrThrow) checks in your code. * These are e2ed internal checks. Use `assertPage` instead. */ await expect(searchPage.pageParams, 'pageParams is correct after assertPage').eql({ @@ -86,7 +86,7 @@ test('exists', {meta: {testId: '1'}, testIdleTimeout: 10_000, testTimeout: 15_00 const url = await getDocumentUrl(); - await expect(SearchRoute.getParamsFromUrl(url), 'page url has expected params').eql({ + await expect(SearchRoute.getParamsFromUrlOrThrow(url), 'page url has expected params').eql({ searchQuery, }); diff --git a/autotests/tests/mockApiRoute.ts b/autotests/tests/mockApiRoute.ts index 7d985e5b..a5c3c149 100644 --- a/autotests/tests/mockApiRoute.ts +++ b/autotests/tests/mockApiRoute.ts @@ -37,7 +37,7 @@ test( const fetchUrl = `https://reqres.in/api/product/${productId}?size=${product.size}` as Url; - const productRouteParams = CreateProductRoute.getParamsFromUrl(fetchUrl); + const productRouteParams = CreateProductRoute.getParamsFromUrlOrThrow(fetchUrl); const productRouteFromUrl = new CreateProductRoute(productRouteParams); diff --git a/scripts/writePrunedPackageJson.ts b/scripts/writePrunedPackageJson.ts index 5f64dc4a..301d956d 100644 --- a/scripts/writePrunedPackageJson.ts +++ b/scripts/writePrunedPackageJson.ts @@ -1,5 +1,5 @@ /** - * @file Generates and writes pruned lightweight package.json for npm package. + * @file Generates and writes pruned lightweight `package.json` for npm package. */ import {writeFileSync} from 'node:fs'; diff --git a/src/Route.ts b/src/Route.ts index e44c4094..63c6c8ec 100644 --- a/src/Route.ts +++ b/src/Route.ts @@ -25,7 +25,7 @@ export abstract class Route { * Returns route params from the passed url. * @throws {Error} If the route does not match on the url. */ - static getParamsFromUrl?(url: Url, method?: Method): unknown; + static getParamsFromUrlOrThrow?(url: Url, method?: Method): unknown; /** * Returns the url of the route. diff --git a/src/actions/mock/mockApiRoute.ts b/src/actions/mock/mockApiRoute.ts index b3300d34..f0f2e12c 100644 --- a/src/actions/mock/mockApiRoute.ts +++ b/src/actions/mock/mockApiRoute.ts @@ -17,9 +17,9 @@ import type { /** * Mock API for some API route. - * Applicable only for routes with the `getParamsFromUrl` method. + * Applicable only for routes with the `getParamsFromUrlOrThrow` method. * The mock is applied to a request that matches the route by url - * (by methods `getParamsFromUrl` and `isMatchUrl`) and by HTTP method (by `getMethod`). + * (by methods `getParamsFromUrlOrThrow` and `isMatchUrl`) and by HTTP method (by `getMethod`). */ export const mockApiRoute = async < RouteParams, diff --git a/src/types/routes.ts b/src/types/routes.ts index 9e39c440..6f7eec10 100644 --- a/src/types/routes.ts +++ b/src/types/routes.ts @@ -16,7 +16,7 @@ export type ApiRouteClassType< }; /** - * API Route class with static method getParamsFromUrl. + * API Route class with static method getParamsFromUrlOrThrow. */ export type ApiRouteClassTypeWithGetParamsFromUrl< RouteParams = Any, @@ -24,5 +24,5 @@ export type ApiRouteClassTypeWithGetParamsFromUrl< SomeResponse extends Response = Response, > = ApiRouteClassType & Readonly<{ - getParamsFromUrl: Exclude<(typeof ApiRoute)['getParamsFromUrl'], undefined>; + getParamsFromUrlOrThrow: Exclude<(typeof ApiRoute)['getParamsFromUrlOrThrow'], undefined>; }>; diff --git a/src/utils/fullMocks/FullMocksRoute.ts b/src/utils/fullMocks/FullMocksRoute.ts index 0cf88b03..25ef975e 100644 --- a/src/utils/fullMocks/FullMocksRoute.ts +++ b/src/utils/fullMocks/FullMocksRoute.ts @@ -14,7 +14,7 @@ import type {FullMocksRouteParams, Url} from '../../types/internal'; * @internal */ export class FullMocksRoute extends ApiRoute { - static override getParamsFromUrl(url: Url): FullMocksRouteParams { + static override getParamsFromUrlOrThrow(url: Url): FullMocksRouteParams { const {fullMocks: fullMocksConfig} = getFullPackConfig(); const fullMocksState = getFullMocksState(); diff --git a/src/utils/getRouteInstanceFromUrl.ts b/src/utils/getRouteInstanceFromUrl.ts index 43bc92ac..8bc0b36c 100644 --- a/src/utils/getRouteInstanceFromUrl.ts +++ b/src/utils/getRouteInstanceFromUrl.ts @@ -9,9 +9,9 @@ type Return = /** * Get route instance and route params from url and method by route class. - * @throws {Error} If `url` accepted without errors by `getParamsFromUrl`, + * @throws {Error} If `url` accepted without errors by `getParamsFromUrlOrThrow`, * but not match by `isMatchUrl` method. - * If `url` not accepted by `getParamsFromUrl`, returns `undefined`. + * If `url` not accepted by `getParamsFromUrlOrThrow`, returns `undefined`. * @internal */ export const getRouteInstanceFromUrl = ( @@ -22,7 +22,7 @@ export const getRouteInstanceFromUrl = ( let routeParams: RouteParams | undefined; try { - routeParams = Route.getParamsFromUrl(url) as RouteParams; + routeParams = Route.getParamsFromUrlOrThrow(url) as RouteParams; route = new Route(routeParams); } catch { return undefined; @@ -30,7 +30,7 @@ export const getRouteInstanceFromUrl = ( if (route.isMatchUrl(url) !== true) { throw new E2edError( - `Inconsistency in "${Route.name}" route: isMatchUrl does not accept url accepted without errors by getParamsFromUrl`, + `Inconsistency in "${Route.name}" route: isMatchUrl does not accept url accepted without errors by getParamsFromUrlOrThrow`, {route, url}, ); } diff --git a/src/utils/resourceUsage.ts b/src/utils/resourceUsage.ts index 505ec509..83885b85 100644 --- a/src/utils/resourceUsage.ts +++ b/src/utils/resourceUsage.ts @@ -10,7 +10,7 @@ const availableCpuCount = availableParallelism(); let previousCores = cpus(); let previousCpuUsage = process.cpuUsage(); let previousTimeInMs = Date.now(); -let timeoutInterval: NodeJS.Timeout; +let timeoutObject: NodeJS.Timeout; const logResourceUsage = (): void => { const cores = cpus(); @@ -65,7 +65,7 @@ const logResourceUsage = (): void => { * @internal */ export const startResourceUsageReading = (resourceUsageReadingInternal: number): void => { - assertValueIsUndefined(timeoutInterval, 'timeoutInterval in not defined', { + assertValueIsUndefined(timeoutObject, 'timeoutObject in not defined', { resourceUsageReadingInternal, }); @@ -73,7 +73,7 @@ export const startResourceUsageReading = (resourceUsageReadingInternal: number): return; } - timeoutInterval = setInterval(logResourceUsage, resourceUsageReadingInternal); + timeoutObject = setInterval(logResourceUsage, resourceUsageReadingInternal); - timeoutInterval.unref(); + timeoutObject.unref(); }; diff --git a/src/utils/retry/runPackWithRetries.ts b/src/utils/retry/runPackWithRetries.ts index 936016b3..b189c685 100644 --- a/src/utils/retry/runPackWithRetries.ts +++ b/src/utils/retry/runPackWithRetries.ts @@ -47,7 +47,7 @@ export const runPackWithRetries = async (): Promise => { endE2ed(EndE2edReason.RetriesCycleEnded); } catch (error) { - generalLog('Caught an error on running testso', { + generalLog('Caught an error on running test', { error, retriesState: truncateRetriesStateForLogs(retriesState), }); diff --git a/src/utils/test/beforeTest.ts b/src/utils/test/beforeTest.ts index 0ad23c2e..29e870ec 100644 --- a/src/utils/test/beforeTest.ts +++ b/src/utils/test/beforeTest.ts @@ -32,7 +32,7 @@ type Options = Readonly<{ testStaticOptions: TestStaticOptions; }>; -const additionalDurationToPlaywrightTestTimeoutInMs = 500; +const additionToPlaywrightTestTimeout = 500; /** * Internal before test hook. @@ -60,9 +60,7 @@ export const beforeTest = ({ const testIdleTimeout = options.testIdleTimeout ?? testIdleTimeoutFromConfig; const testTimeout = options.testTimeout ?? testTimeoutFromConfig; - test.setTimeout( - testTimeout + additionalDurationToPlaywrightTestTimeoutInMs + (beforeRetryTimeout ?? 0), - ); + test.setTimeout(testTimeout + additionToPlaywrightTestTimeout + (beforeRetryTimeout ?? 0)); setTestIdleTimeout(testIdleTimeout); setTestTimeout(testTimeout); diff --git a/src/utils/test/getPreviousRunId.ts b/src/utils/test/getPreviousRunId.ts new file mode 100644 index 00000000..a74d95c1 --- /dev/null +++ b/src/utils/test/getPreviousRunId.ts @@ -0,0 +1,37 @@ +import {assertValueIsTrue} from '../asserts'; + +import type {RunId} from '../../types/internal'; + +type Return = Readonly<{previousRunId: RunId | undefined; retryIndex: number}>; + +/** + * Get previous `runId` if any, by current `runId`. + * @internal + */ +export const getPreviousRunId = (runId: RunId): Return => { + const indexOfRetryIndex = runId.lastIndexOf('-'); + + assertValueIsTrue( + indexOfRetryIndex > 0 && indexOfRetryIndex < runId.length - 1, + 'runId has dash', + {runId}, + ); + + const retryIndex = Number(runId.slice(indexOfRetryIndex + 1)); + + assertValueIsTrue( + Number.isInteger(retryIndex) && retryIndex > 0, + 'retryIndex from runId is correct', + {runId}, + ); + + const previousRetryIndex = retryIndex - 1; + + if (previousRetryIndex < 1) { + return {previousRunId: undefined, retryIndex}; + } + + const previousRunId = `${runId.slice(0, indexOfRetryIndex)}-${previousRetryIndex}` as RunId; + + return {previousRunId, retryIndex}; +}; diff --git a/src/utils/test/waitBeforeRetry.ts b/src/utils/test/waitBeforeRetry.ts index ef618ac3..d33428a0 100644 --- a/src/utils/test/waitBeforeRetry.ts +++ b/src/utils/test/waitBeforeRetry.ts @@ -1,11 +1,16 @@ -import {assertValueIsTrue} from '../asserts'; import {getFullPackConfig} from '../config'; -import {getTestRunEventFileName, readEventFromFile} from '../fs'; +import {getTestRunEventFileName, readEventFromFile, writeLogEventTime} from '../fs'; import {generalLog} from '../generalLog'; import {getTimeoutPromise} from '../promise'; +import {getPreviousRunId} from './getPreviousRunId'; + import type {FullTestRun, RunId, TestStaticOptions} from '../../types/internal'; +import {test} from '@playwright/test'; + +const additionToTimeout = 500; + /** * Waits before running test for some time from pack config (for retries). * @internal @@ -14,30 +19,12 @@ export const waitBeforeRetry = async ( runId: RunId, testStaticOptions: TestStaticOptions, ): Promise => { - const indexOfRetryIndex = runId.lastIndexOf('-'); - - assertValueIsTrue( - indexOfRetryIndex > 0 && indexOfRetryIndex < runId.length - 1, - 'runId has dash', - {runId, testStaticOptions}, - ); - - const retryIndex = Number(runId.slice(indexOfRetryIndex + 1)); - - assertValueIsTrue( - Number.isInteger(retryIndex) && retryIndex > 0, - 'retryIndex from runId is correct', - {runId, testStaticOptions}, - ); + const {previousRunId, retryIndex} = getPreviousRunId(runId); - const previousRetryIndex = retryIndex - 1; - - if (previousRetryIndex < 1) { + if (previousRunId === undefined) { return; } - const previousRunId = `${runId.slice(0, indexOfRetryIndex)}-${previousRetryIndex}` as RunId; - const fileName = getTestRunEventFileName(previousRunId); const fileText = await readEventFromFile(fileName); @@ -55,7 +42,7 @@ export const waitBeforeRetry = async ( const fullTestRun = JSON.parse(fileText) as FullTestRun; const {runError, status} = fullTestRun; - const {waitBeforeRetry: waitBeforeRetryFromConfig} = getFullPackConfig(); + const {testIdleTimeout, waitBeforeRetry: waitBeforeRetryFromConfig} = getFullPackConfig(); const previousError = runError === undefined ? undefined : String(runError); @@ -70,8 +57,18 @@ export const waitBeforeRetry = async ( return; } + test.setTimeout(timeoutInMs + additionToTimeout); + + const timeoutObject = setInterval(() => { + void writeLogEventTime(); + }, testIdleTimeout); + + timeoutObject.unref(); + await getTimeoutPromise(timeoutInMs); + clearInterval(timeoutObject); + return timeoutInMs; } catch (error) { generalLog('Caught an error on getting timeout for "before retry" waiting', {