From 776b04e5ea1f2adcc82d9459cf593775662ef3e3 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 13 May 2024 18:51:30 -0700 Subject: [PATCH] feat: APIRequestContext dispose reason (#30765) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similarly to page.close, we pass test-runner specific reason to facilitate better error messages. ``` 1) a.test.ts:10:11 › test Error: apiRequestContext.fetch: Fixture { request } from beforeAll cannot be reused in a test. - Recommended fix: use a separate { request } in the test. - Alternatively, manually create APIRequestContext in beforeAll and dispose it in afterAll. See https://playwright.dev/docs/api-testing#sending-api-requests-from-ui-tests for more details. 9 | 10 | test('test', async () => { > 11 | await context.fetch('http://example.com'); | ^ 12 | }); 13 | ``` Closes #29260. --- docs/src/api/class-apirequestcontext.md | 6 ++++++ packages/playwright-core/src/client/fetch.ts | 8 +++++-- .../playwright-core/src/protocol/validator.ts | 4 +++- .../server/dispatchers/networkDispatchers.ts | 4 ++-- packages/playwright-core/src/server/fetch.ts | 8 ++++--- packages/playwright-core/types/types.d.ts | 8 ++++++- packages/playwright/src/index.ts | 12 ++++++++++- packages/protocol/src/channels.ts | 10 ++++++--- packages/protocol/src/protocol.yml | 2 ++ tests/playwright-test/playwright.spec.ts | 21 +++++++++++++++++++ 10 files changed, 70 insertions(+), 13 deletions(-) diff --git a/docs/src/api/class-apirequestcontext.md b/docs/src/api/class-apirequestcontext.md index aaf341c438..0d2be68109 100644 --- a/docs/src/api/class-apirequestcontext.md +++ b/docs/src/api/class-apirequestcontext.md @@ -185,6 +185,12 @@ context cookies from the response. The method will automatically follow redirect All responses returned by [`method: APIRequestContext.get`] and similar methods are stored in the memory, so that you can later call [`method: APIResponse.body`].This method discards all its resources, calling any method on disposed [APIRequestContext] will throw an exception. +### option: APIRequestContext.dispose.reason +* since: v1.45 +- `reason` <[string]> + +The reason to be reported to the operations interrupted by the context disposure. + ## async method: APIRequestContext.fetch * since: v1.16 - returns: <[APIResponse]> diff --git a/packages/playwright-core/src/client/fetch.ts b/packages/playwright-core/src/client/fetch.ts index afe622bb6d..34f0af3eae 100644 --- a/packages/playwright-core/src/client/fetch.ts +++ b/packages/playwright-core/src/client/fetch.ts @@ -85,6 +85,7 @@ export class APIRequest implements api.APIRequest { export class APIRequestContext extends ChannelOwner implements api.APIRequestContext { _request?: APIRequest; readonly _tracing: Tracing; + private _closeReason: string | undefined; static from(channel: channels.APIRequestContextChannel): APIRequestContext { return (channel as any)._object; @@ -99,9 +100,10 @@ export class APIRequestContext extends ChannelOwner { + async dispose(options: { reason?: string } = {}): Promise { + this._closeReason = options.reason; await this._instrumentation.onWillCloseRequestContext(this); - await this._channel.dispose(); + await this._channel.dispose(options); this._tracing._resetStackCounter(); this._request?._contexts.delete(this); } @@ -156,6 +158,8 @@ export class APIRequestContext extends ChannelOwner { return await this._wrapApiCall(async () => { + if (this._closeReason) + throw new Error(this._closeReason); assert(options.request || typeof options.url === 'string', 'First argument must be either URL string or Request'); assert((options.data === undefined ? 0 : 1) + (options.form === undefined ? 0 : 1) + (options.multipart === undefined ? 0 : 1) <= 1, `Only one of 'data', 'form' or 'multipart' can be specified`); assert(options.maxRedirects === undefined || options.maxRedirects >= 0, `'maxRedirects' should be greater than or equal to '0'`); diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index f97cf10969..83cfc633ab 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -207,7 +207,9 @@ scheme.APIRequestContextDisposeAPIResponseParams = tObject({ fetchUid: tString, }); scheme.APIRequestContextDisposeAPIResponseResult = tOptional(tObject({})); -scheme.APIRequestContextDisposeParams = tOptional(tObject({})); +scheme.APIRequestContextDisposeParams = tObject({ + reason: tOptional(tString), +}); scheme.APIRequestContextDisposeResult = tOptional(tObject({})); scheme.APIResponse = tObject({ fetchUid: tString, diff --git a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts index b89dc2c324..ba600f697e 100644 --- a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts +++ b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts @@ -198,9 +198,9 @@ export class APIRequestContextDispatcher extends Dispatcher { + async dispose(params: channels.APIRequestContextDisposeParams, metadata: CallMetadata): Promise { metadata.potentiallyClosesScope = true; - await this._object.dispose(); + await this._object.dispose(params); this._dispose(); } diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index fa38723645..56125f2e5d 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -121,7 +121,7 @@ export abstract class APIRequestContext extends SdkObject { abstract tracing(): Tracing; - abstract dispose(): Promise; + abstract dispose(options: { reason?: string }): Promise; abstract _defaultOptions(): FetchRequestOptions; abstract _addCookies(cookies: channels.NetworkCookie[]): Promise; @@ -483,7 +483,8 @@ export class BrowserContextAPIRequestContext extends APIRequestContext { return this._context.tracing; } - override async dispose() { + override async dispose(options: { reason?: string }) { + this._closeReason = options.reason; this.fetchResponses.clear(); } @@ -552,7 +553,8 @@ export class GlobalAPIRequestContext extends APIRequestContext { return this._tracing; } - override async dispose() { + override async dispose(options: { reason?: string }) { + this._closeReason = options.reason; await this._tracing.flush(); await this._tracing.deleteTmpTracesDir(); this._disposeImpl(); diff --git a/packages/playwright-core/types/types.d.ts b/packages/playwright-core/types/types.d.ts index 99fc9fca1e..70a25c3878 100644 --- a/packages/playwright-core/types/types.d.ts +++ b/packages/playwright-core/types/types.d.ts @@ -15846,8 +15846,14 @@ export interface APIRequestContext { * and similar methods are stored in the memory, so that you can later call * [apiResponse.body()](https://playwright.dev/docs/api/class-apiresponse#api-response-body).This method discards all * its resources, calling any method on disposed {@link APIRequestContext} will throw an exception. + * @param options */ - dispose(): Promise; + dispose(options?: { + /** + * The reason to be reported to the operations interrupted by the context disposure. + */ + reason?: string; + }): Promise; /** * Sends HTTP(S) request and returns its response. The method will populate request cookies from the context and diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index 2c105a2640..6cf690ad50 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -392,7 +392,17 @@ const playwrightFixtures: Fixtures = ({ request: async ({ playwright }, use) => { const request = await playwright.request.newContext(); await use(request); - await request.dispose(); + const hook = (test.info() as TestInfoImpl)._currentHookType(); + if (hook === 'beforeAll') { + await request.dispose({ reason: [ + `Fixture { request } from beforeAll cannot be reused in a test.`, + ` - Recommended fix: use a separate { request } in the test.`, + ` - Alternatively, manually create APIRequestContext in beforeAll and dispose it in afterAll.`, + `See https://playwright.dev/docs/api-testing#sending-api-requests-from-ui-tests for more details.`, + ].join('\n') }); + } else { + await request.dispose(); + } }, }); diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 2024e02b56..d80152d8f0 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -309,7 +309,7 @@ export interface APIRequestContextChannel extends APIRequestContextEventTarget, fetchLog(params: APIRequestContextFetchLogParams, metadata?: CallMetadata): Promise; storageState(params?: APIRequestContextStorageStateParams, metadata?: CallMetadata): Promise; disposeAPIResponse(params: APIRequestContextDisposeAPIResponseParams, metadata?: CallMetadata): Promise; - dispose(params?: APIRequestContextDisposeParams, metadata?: CallMetadata): Promise; + dispose(params: APIRequestContextDisposeParams, metadata?: CallMetadata): Promise; } export type APIRequestContextFetchParams = { url: string, @@ -372,8 +372,12 @@ export type APIRequestContextDisposeAPIResponseOptions = { }; export type APIRequestContextDisposeAPIResponseResult = void; -export type APIRequestContextDisposeParams = {}; -export type APIRequestContextDisposeOptions = {}; +export type APIRequestContextDisposeParams = { + reason?: string, +}; +export type APIRequestContextDisposeOptions = { + reason?: string, +}; export type APIRequestContextDisposeResult = void; export interface APIRequestContextEvents { diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index d3b5f9d00c..8f1fb99cf3 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -330,6 +330,8 @@ APIRequestContext: fetchUid: string dispose: + parameters: + reason: string? APIResponse: diff --git a/tests/playwright-test/playwright.spec.ts b/tests/playwright-test/playwright.spec.ts index 1c0212bf79..ab33f2e6f6 100644 --- a/tests/playwright-test/playwright.spec.ts +++ b/tests/playwright-test/playwright.spec.ts @@ -827,3 +827,24 @@ test('should save trace in two APIRequestContexts', async ({ runInlineTest, serv expect(result.exitCode).toBe(0); expect(result.passed).toBe(1); }); + +test('should explain a failure when using a dispose APIRequestContext', async ({ runInlineTest, server }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + + let context; + + test.beforeAll(async ({ request }) => { + context = request; + }); + + test('test', async () => { + await context.fetch('http://example.com'); + }); + `, + }, { workers: 1 }); + expect(result.exitCode).toBe(1); + expect(result.passed).toBe(0); + expect(result.output).toContain(`Recommended fix: use a separate { request } in the test`); +});