From 85b19d8095877aad83cef6b6ee69a5c245718b63 Mon Sep 17 00:00:00 2001 From: barak igal Date: Sun, 3 Dec 2023 14:39:46 +0200 Subject: [PATCH] fix: remove usage of global disposeAfter from withFeature and getRunningFeature (#2176) --- .../test/electron-node.unit.ts | 21 +- .../test/worker-thread.unit.ts | 11 +- packages/test-kit/src/run-environment.ts | 21 +- packages/test-kit/src/with-feature.ts | 189 ++++++++++-------- packages/test-kit/src/with-local-fixture.ts | 8 +- 5 files changed, 139 insertions(+), 111 deletions(-) diff --git a/packages/electron-commons/test/electron-node.unit.ts b/packages/electron-commons/test/electron-node.unit.ts index 3d943f486..048cd09a9 100644 --- a/packages/electron-commons/test/electron-node.unit.ts +++ b/packages/electron-commons/test/electron-node.unit.ts @@ -5,7 +5,7 @@ import path from 'node:path'; import type { TopLevelConfig } from '@wixc3/engine-core'; import testFeature, { serverEnv } from '@fixture/disconnecting-env/dist/disconnecting-env.feature.js'; import { setupRunningNodeEnv } from '../test-kit/setup-running-node-env.js'; -import { disposeAfter } from '@wixc3/testing'; +import { Disposables } from '@wixc3/patterns'; const { expect } = chai; chai.use(chaiAsPromised); @@ -28,6 +28,8 @@ const setupRunningEnv = ({ featuresConfig, stdio }: SetupRunningFeatureOptions) }); describe('onDisconnectHandler for node environment initializer', () => { + const disposables = new Disposables(); + afterEach(() => disposables.dispose()); describe('without own uncaughtException handling', () => { it('should catch on dispose of env', async () => { const { dispose, exitPromise } = await setupRunningEnv({ @@ -42,7 +44,8 @@ describe('onDisconnectHandler for node environment initializer', () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'exit' } })], }); - disposeAfter(dispose, { + + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); @@ -53,7 +56,8 @@ describe('onDisconnectHandler for node environment initializer', () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'exception' } })], }); - disposeAfter(dispose, { + + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); @@ -64,7 +68,7 @@ describe('onDisconnectHandler for node environment initializer', () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'promise-reject' } })], }); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); @@ -76,7 +80,7 @@ describe('onDisconnectHandler for node environment initializer', () => { featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'exception' } })], stdio: 'pipe', }); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); @@ -100,7 +104,7 @@ describe('onDisconnectHandler for node environment initializer', () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'exit', handleUncaught } })], }); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); @@ -110,18 +114,17 @@ describe('onDisconnectHandler for node environment initializer', () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'exception', handleUncaught } })], }); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); - await expect(exitPromise).to.eventually.deep.eq({ exitCode: 1 }); }); it('should catch on env unhandled promise rejection', async () => { const { exitPromise, dispose } = await setupRunningEnv({ featuresConfig: [testFeature.use({ errorsConfig: { throwError: 'promise-reject', handleUncaught } })], }); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `env ${testFeature.id}`, }); diff --git a/packages/electron-commons/test/worker-thread.unit.ts b/packages/electron-commons/test/worker-thread.unit.ts index 24e5dccb8..4cf42e415 100644 --- a/packages/electron-commons/test/worker-thread.unit.ts +++ b/packages/electron-commons/test/worker-thread.unit.ts @@ -8,7 +8,7 @@ import workerThreadFeature, { serverEnv, type WorkerService, } from '@fixture/worker-thread/dist/worker-thread.feature.js'; -import { disposeAfter } from '@wixc3/testing'; +import { Disposables } from '@wixc3/patterns'; import { expect } from 'chai'; import { setupRunningNodeEnv } from '../test-kit/setup-running-node-env.js'; @@ -24,9 +24,12 @@ const setupRunningEnv = (featureId: string) => const timeout = 3000; describe('workerthread environment type', () => { + const disposables = new Disposables(); + afterEach(() => disposables.dispose()); it('initializes worker, calls API and disposes', async () => { const { dispose, communication } = await setupRunningEnv(workerThreadFeature.id); - disposeAfter(dispose, { + + disposables.add(dispose, { timeout, name: `worker thread ${workerThreadFeature.id}`, }); @@ -42,7 +45,7 @@ describe('workerthread environment type', () => { it('initializes multiple workers, calls API and disposes', async () => { const { dispose, communication } = await setupRunningEnv(`${workerThreadFeature.id}/${multiFeature.id}`); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `worker thread ${workerThreadFeature.id}/${multiFeature.id}`, }); @@ -60,7 +63,7 @@ describe('workerthread environment type', () => { const { dispose, communication } = await setupRunningEnv( `${workerThreadFeature.id}/${contextualMultiPreloadFeature.id}`, ); - disposeAfter(dispose, { + disposables.add(dispose, { timeout, name: `worker thread ${workerThreadFeature.id}/${contextualMultiPreloadFeature.id}`, }); diff --git a/packages/test-kit/src/run-environment.ts b/packages/test-kit/src/run-environment.ts index ee855ee1f..d5a31ca17 100644 --- a/packages/test-kit/src/run-environment.ts +++ b/packages/test-kit/src/run-environment.ts @@ -1,14 +1,13 @@ import { RuntimeEngine, type AnyEnvironment, type FeatureClass, type Running } from '@wixc3/engine-core'; import { getRunningFeature as originalGetRunningFeature, type RunningFeatureOptions } from '@wixc3/engine-scripts'; -import { disposeAfter } from '@wixc3/testing'; /** * get a running feature with no browser environment - * @param disposeAfterTestTimeout if false, will not dispose the engine after the test + * @param autoDisposeTimeout if false, will not dispose the engine after the test */ export async function getRunningFeature( options: RunningFeatureOptions, - disposeAfterTestTimeout: false | number = 10_000, + autoDisposeTimeout: false | number = 10_000, ): Promise<{ runningApi: Running; engine: RuntimeEngine; @@ -16,11 +15,17 @@ export async function getRunningFeature Promise; }> { const runningFeature = await originalGetRunningFeature(options); - if (disposeAfterTestTimeout) { - disposeAfter(runningFeature.engine.shutdown, { - name: `engine shutdown for ${options.featureName}`, - timeout: disposeAfterTestTimeout, - }); + if (autoDisposeTimeout) { + if (typeof afterEach !== 'undefined') { + afterEach(`engine shutdown for ${options.featureName}`, function () { + this.timeout(autoDisposeTimeout); + return runningFeature.engine.shutdown(); + }); + } else { + throw new Error( + `autoDisposeTimeout is set but the environment you are running does not have global "afterEach", set it to false to avoid auto-dispose.`, + ); + } } return runningFeature; diff --git a/packages/test-kit/src/with-feature.ts b/packages/test-kit/src/with-feature.ts index b464c26a1..d26322a84 100644 --- a/packages/test-kit/src/with-feature.ts +++ b/packages/test-kit/src/with-feature.ts @@ -1,7 +1,7 @@ import { nodeFs as fs } from '@file-services/node'; import type { TopLevelConfig } from '@wixc3/engine-core'; import { Disposables, type DisposableItem, type DisposableOptions } from '@wixc3/patterns'; -import { createDisposalGroup, disposeAfter, mochaCtx } from '@wixc3/testing'; +import { mochaCtx } from '@wixc3/testing'; import { DISPOSE_OF_TEMP_DIRS } from '@wixc3/testing-node'; import { uniqueHash } from '@wixc3/engine-scripts'; import isCI from 'is-ci'; @@ -149,12 +149,36 @@ export interface Tracing { name?: string; } +export type WithFeatureApi = { + /** + * Opens a browser page with the feature loaded + * @param options - options to override the suite options + * @returns a promise that resolves to the page and the response + */ + getLoadedFeature: (options?: IFeatureExecutionOptions) => Promise<{ + page: playwright.Page; + response: playwright.Response | null; + getMetrics: () => Promise<{ + marks: PerformanceEntry[]; + measures: PerformanceEntry[]; + }>; + }>; + /** + * @deprecated use `onDispose` instead + */ + disposeAfter: typeof Disposables.prototype.add; + /** + * Add a disposable to be disposed after the test. + * by default this will add the disposable to the `FINALE` group + * if running in persist mode, the disposable will be disposed after the suite + */ + onDispose: typeof Disposables.prototype.add; +}; + export const WITH_FEATURE_DISPOSABLES = 'WITH_FEATURE_DISPOSABLES'; export const PAGE_DISPOSABLES = 'PAGE_DISPOSABLES'; export const TRACING_DISPOSABLES = 'TRACING_DISPOSABLES'; -createDisposalGroup(WITH_FEATURE_DISPOSABLES, { after: 'default', before: DISPOSE_OF_TEMP_DIRS }); -createDisposalGroup(PAGE_DISPOSABLES, { before: WITH_FEATURE_DISPOSABLES }); -createDisposalGroup(TRACING_DISPOSABLES, { before: PAGE_DISPOSABLES }); +export const FINALE = 'FINALE'; let browser: playwright.Browser | undefined = undefined; let featureUrl = ''; @@ -178,7 +202,7 @@ if (typeof after !== 'undefined') { }); } -export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { +export function withFeature(withFeatureOptions: IWithFeatureOptions = {}): WithFeatureApi { const envDebugMode = 'DEBUG' in process.env; const debugMode = !!process.env.DEBUG; const port = parseInt(process.env.DEBUG!); @@ -254,8 +278,6 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { }); } - const tracingDisposables = new Set<(testName?: string) => Promise>(); - let dedicatedBrowser: playwright.Browser | undefined; let dedicatedBrowserContext: playwright.BrowserContext | undefined; @@ -275,29 +297,26 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { } }); - let dispose = disposeAfter; - let alreadyInitialized = false; + const disposables = new Disposables(); + disposables.registerGroup(DISPOSE_OF_TEMP_DIRS, { after: 'default' }); + disposables.registerGroup(WITH_FEATURE_DISPOSABLES, { after: 'default', before: DISPOSE_OF_TEMP_DIRS }); + disposables.registerGroup(PAGE_DISPOSABLES, { before: WITH_FEATURE_DISPOSABLES }); + disposables.registerGroup(TRACING_DISPOSABLES, { before: PAGE_DISPOSABLES }); + disposables.registerGroup(FINALE, { after: PAGE_DISPOSABLES }); + const onDispose = (disposable: DisposableItem, options?: DisposableOptions) => + disposables.add(disposable, { group: FINALE, ...options }); - const afterEachDisposables = new Set<() => Promise>(); + let alreadyInitialized = false; if (persist) { - const disposables = new Disposables(); - disposables.registerGroup(DISPOSE_OF_TEMP_DIRS, { after: 'default' }); - disposables.registerGroup(WITH_FEATURE_DISPOSABLES, { after: 'default', before: DISPOSE_OF_TEMP_DIRS }); - disposables.registerGroup(PAGE_DISPOSABLES, { before: WITH_FEATURE_DISPOSABLES }); - disposables.registerGroup(TRACING_DISPOSABLES, { before: PAGE_DISPOSABLES }); - dispose = (disposable: DisposableItem, options?: DisposableOptions) => disposables.add(disposable, options); after('dispose suite level page', async function () { this.timeout(disposables.list().totalTimeout); await disposables.dispose(); }); } else { afterEach('dispose all', async function () { - this.timeout(20_000); - for (const disposable of afterEachDisposables) { - await disposable(); - } - afterEachDisposables.clear(); + this.timeout(disposables.list().totalTimeout); + await disposables.dispose(); }); } @@ -323,11 +342,10 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { if (!dedicatedBrowserContext) { dedicatedBrowserContext = await dedicatedBrowser.newContext(browserContextOptions); await enableTestBrowserContext({ + disposables, browserContext: dedicatedBrowserContext, - dispose, suiteTracing, tracing, - tracingDisposables, withFeatureOptions, allowedErrors, capturedErrors, @@ -340,22 +358,22 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { } dedicatedBrowserContext = await browser.newContext(browserContextOptions); + disposables.add(() => dedicatedBrowserContext?.close(), { + group: WITH_FEATURE_DISPOSABLES, + name: `close browser context for feature "${featureName}"`, + timeout: 5000, + }); + await enableTestBrowserContext({ browserContext: dedicatedBrowserContext, - dispose, + disposables, suiteTracing, tracing, - tracingDisposables, withFeatureOptions, allowedErrors, capturedErrors, consoleLogAllowedErrors, }); - dispose(() => dedicatedBrowserContext?.close(), { - group: WITH_FEATURE_DISPOSABLES, - name: `close browser context for feature "${featureName}"`, - timeout: 5000, - }); } if (!executableApp) { @@ -379,7 +397,7 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { const { configName: newConfigName } = runningFeature; - dispose(() => runningFeature.dispose(), { + disposables.add(() => runningFeature.dispose(), { group: WITH_FEATURE_DISPOSABLES, name: `close feature "${featureName}"`, timeout: withFeatureOptions.featureDisposeTimeout ?? 10_000, @@ -393,30 +411,37 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { const featurePage = await dedicatedBrowserContext.newPage(); - afterEachDisposables.add(async () => { - if (takeScreenshotsOfFailed) { - const suiteTracingOptions = typeof suiteTracing === 'boolean' ? {} : suiteTracing; - const testTracingOptions = typeof tracing === 'boolean' ? {} : tracing; - const outPath = - (typeof takeScreenshotsOfFailed !== 'boolean' && takeScreenshotsOfFailed.outPath) || - `${ - suiteTracingOptions?.outPath ?? testTracingOptions?.outPath ?? process.cwd() - }/screenshots-of-failed-tests`; - - const ctx = mochaCtx(); - - if (ctx?.currentTest?.state === 'failed') { - const testPath = ctx.currentTest.titlePath().join('/').replace(/\s/g, '-'); - const filePath = `${outPath}/${testPath}__${uniqueHash()}.png`; - await featurePage.screenshot({ path: filePath }); - - console.log( - reporters.Base.color('bright yellow', `The screenshot has been saved at ${filePath}`), - ); + disposables.add( + async () => { + if (takeScreenshotsOfFailed) { + const suiteTracingOptions = typeof suiteTracing === 'boolean' ? {} : suiteTracing; + const testTracingOptions = typeof tracing === 'boolean' ? {} : tracing; + const outPath = + (typeof takeScreenshotsOfFailed !== 'boolean' && takeScreenshotsOfFailed.outPath) || + `${ + suiteTracingOptions?.outPath ?? testTracingOptions?.outPath ?? process.cwd() + }/screenshots-of-failed-tests`; + + const ctx = mochaCtx(); + + if (ctx?.currentTest?.state === 'failed') { + const testPath = ctx.currentTest.titlePath().join('/').replace(/\s/g, '-'); + const filePath = `${outPath}/${testPath}__${uniqueHash()}.png`; + await featurePage.screenshot({ path: filePath }); + + console.log( + reporters.Base.color('bright yellow', `The screenshot has been saved at ${filePath}`), + ); + } } - } - await featurePage.close(); - }); + await featurePage.close(); + }, + { + group: FINALE, + name: 'close feature page' + (takeScreenshotsOfFailed ? ' and take screenshot' : ''), + timeout: 10_000, + }, + ); const fullFeatureUrl = (buildFlow ? runningFeature.url : featureUrl) + search; const response = await featurePage.goto(fullFeatureUrl, navigationOptions); @@ -427,7 +452,8 @@ export function withFeature(withFeatureOptions: IWithFeatureOptions = {}) { getMetrics: () => getMetrics(runningFeature, featurePage), }; }, - disposeAfter: dispose, + disposeAfter: onDispose, + onDispose, }; } @@ -468,27 +494,25 @@ async function getMetrics(runningFeature: RunningTestFeature, featurePage: playw async function enableTestBrowserContext({ browserContext, - dispose, + disposables, suiteTracing, tracing, - tracingDisposables, withFeatureOptions, allowedErrors, capturedErrors, consoleLogAllowedErrors, }: { browserContext: playwright.BrowserContext; - dispose: (disposable: DisposableItem, options?: DisposableOptions | undefined) => void; + disposables: Disposables; suiteTracing: boolean | Omit | undefined; tracing: boolean | Tracing | undefined; - tracingDisposables: Set<(testName?: string) => Promise>; withFeatureOptions: IWithFeatureOptions; allowedErrors: (string | RegExp)[]; capturedErrors: Error[]; consoleLogAllowedErrors: boolean; }) { browserContext.on('page', onPageCreation); - dispose(() => browserContext.off('page', onPageCreation), { + disposables.add(() => browserContext.off('page', onPageCreation), { name: 'stop listening for page creation', group: PAGE_DISPOSABLES, timeout: 1000, @@ -502,8 +526,7 @@ async function enableTestBrowserContext({ suiteTracingOptions, testTracingOptions, browserContext, - tracingDisposables, - dispose, + disposables, withFeatureOptions, }); } @@ -527,12 +550,12 @@ async function enableTestBrowserContext({ page.setDefaultNavigationTimeout(30000); page.setDefaultTimeout(10000); const disposeConsoleHook = hookPageConsole(page, isNonReactDevMessage); - dispose(disposeConsoleHook, { + disposables.add(disposeConsoleHook, { name: 'stop listening for console messages', group: PAGE_DISPOSABLES, }); page.on('pageerror', onPageError); - dispose(() => page.off('pageerror', onPageError), { + disposables.add(() => page.off('pageerror', onPageError), { name: 'stop listening for page errors', group: PAGE_DISPOSABLES, timeout: 1000, @@ -544,15 +567,13 @@ async function enableTracing({ suiteTracingOptions, testTracingOptions, browserContext, - tracingDisposables, - dispose, + disposables, withFeatureOptions, }: { suiteTracingOptions: Omit | undefined; testTracingOptions: Tracing; browserContext: playwright.BrowserContext; - tracingDisposables: Set<(testName?: string) => Promise>; - dispose: (disposable: DisposableItem, options?: DisposableOptions | undefined) => void; + disposables: Disposables; withFeatureOptions: IWithFeatureOptions; }) { const screenshots = suiteTracingOptions?.screenshots ?? testTracingOptions.screenshots ?? true; @@ -561,26 +582,22 @@ async function enableTracing({ const name = testTracingOptions.name; await browserContext.tracing.start({ screenshots, snapshots }); - tracingDisposables.add((testName) => { - return browserContext.tracing.stop({ - path: ensureTracePath({ - outPath, - fs, - name: - process.env.TRACING && process.env.TRACING !== 'true' - ? process.env.TRACING - : name ?? (testName ? normalizeTestName(testName) : 'nameless-test'), - }), - }); - }); - dispose( - async () => { - for (const tracingDisposable of tracingDisposables) { - await tracingDisposable(mochaCtx()?.currentTest?.title); - } - tracingDisposables.clear(); + disposables.add( + () => { + const testName = mochaCtx()?.currentTest?.title; + return browserContext.tracing.stop({ + path: ensureTracePath({ + outPath, + fs, + name: + process.env.TRACING && process.env.TRACING !== 'true' + ? process.env.TRACING + : name ?? (testName ? normalizeTestName(testName) : 'nameless-test'), + }), + }); }, { + group: TRACING_DISPOSABLES, name: 'stop tracing', timeout: withFeatureOptions?.saveTraceTimeout ?? 10000, }, diff --git a/packages/test-kit/src/with-local-fixture.ts b/packages/test-kit/src/with-local-fixture.ts index 8ad2d3519..16886eb55 100644 --- a/packages/test-kit/src/with-local-fixture.ts +++ b/packages/test-kit/src/with-local-fixture.ts @@ -1,7 +1,7 @@ import { nodeFs as fs } from '@file-services/node'; import { createTestDir } from '@wixc3/testing-node'; import { spawnSync, type SpawnSyncOptions } from 'node:child_process'; -import { withFeature, type IFeatureExecutionOptions, type IWithFeatureOptions } from './with-feature.js'; +import { withFeature, type IFeatureExecutionOptions, type IWithFeatureOptions, FINALE } from './with-feature.js'; export interface IWithLocalFixtureOptions extends IWithFeatureOptions { fixturePath?: string; @@ -12,17 +12,17 @@ export interface IWithLocalFixtureOptions extends IWithFeatureOptions { * and optionally copies a fixture to it as a "project". */ export function withLocalFixture(suiteOptions: IWithLocalFixtureOptions) { - const { getLoadedFeature: originalGetLoadedFeature, disposeAfter } = withFeature(suiteOptions); + const { getLoadedFeature: originalGetLoadedFeature, onDispose } = withFeature(suiteOptions); async function getLoadedFeature(testOptions: IWithLocalFixtureOptions = suiteOptions) { const { fixturePath = suiteOptions.fixturePath, runOptions = suiteOptions.runOptions } = testOptions; if (runOptions && runOptions.projectPath) { throw new Error( - `runOptions["projectPath"] shouldn't be provided. It will get overriden by returned projectPath.`, + `runOptions["projectPath"] shouldn't be provided. It will get overridden by returned projectPath.`, ); } - const projectPath = createTestDir('local-test', {}, disposeAfter); + const projectPath = createTestDir('local-test', FINALE, onDispose); if (fixturePath) { await fs.promises.copyDirectory(fixturePath, projectPath);