From 81694b7401c458964431c0b202bca99d5cfcb059 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 29 Sep 2023 10:45:31 -0700 Subject: [PATCH] test: unflake some tests (#27354) --- .../src/client/channelOwner.ts | 2 +- .../playwright-core/src/client/connection.ts | 5 +- tests/library/browsercontext-proxy.spec.ts | 47 +++++++++---------- tests/library/tracing.spec.ts | 27 ++++++++--- 4 files changed, 46 insertions(+), 35 deletions(-) diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index 9a76cb9307..eac9d27400 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -147,7 +147,7 @@ export abstract class ChannelOwner { + async sendMessageToServer(object: ChannelOwner, method: string, params: any, stackTrace: ParsedStackTrace | null, wallTime: number | undefined): Promise { if (this._closedErrorMessage) throw new Error(this._closedErrorMessage); if (object._wasCollected) @@ -121,15 +121,16 @@ export class Connection extends EventEmitter { const { apiName, frames } = stackTrace || { apiName: '', frames: [] }; const guid = object._guid; + const type = object._type; const id = ++this._lastId; const converted = { id, guid, method, params }; // Do not include metadata in debug logs to avoid noise. debugLogger.log('channel:command', converted); const location = frames[0] ? { file: frames[0].file, line: frames[0].line, column: frames[0].column } : undefined; const metadata: channels.Metadata = { wallTime, apiName, location, internal: !apiName }; - this.onmessage({ ...converted, metadata }); if (this._tracingCount && frames && type !== 'LocalUtils') this._localUtils?._channel.addStackToTracingNoReply({ callData: { stack: frames, id } }).catch(() => {}); + this.onmessage({ ...converted, metadata }); return await new Promise((resolve, reject) => this._callbacks.set(id, { resolve, reject, stackTrace, type, method })); } diff --git a/tests/library/browsercontext-proxy.spec.ts b/tests/library/browsercontext-proxy.spec.ts index 5353bf4ac9..f9f724d3e9 100644 --- a/tests/library/browsercontext-proxy.spec.ts +++ b/tests/library/browsercontext-proxy.spec.ts @@ -319,7 +319,7 @@ it('should isolate proxy credentials between contexts', async ({ contextFactory, } }); -it('should exclude patterns', async ({ contextFactory, server, browserName, headless, proxyServer }) => { +it('should exclude patterns', async ({ contextFactory, server, proxyServer }) => { proxyServer.forwardTo(server.PORT); // FYI: using long and weird domain names to avoid ATT DNS hijacking // that resolves everything to some weird search results page. @@ -329,56 +329,53 @@ it('should exclude patterns', async ({ contextFactory, server, browserName, head proxy: { server: `localhost:${proxyServer.PORT}`, bypass: '1.non.existent.domain.for.the.test, 2.non.existent.domain.for.the.test, .another.test' } }); - const page = await context.newPage(); - await page.goto('http://0.non.existent.domain.for.the.test/target.html'); - expect(proxyServer.requestUrls).toContain('http://0.non.existent.domain.for.the.test/target.html'); - expect(await page.title()).toBe('Served by the proxy'); - proxyServer.requestUrls = []; - const nonFaviconUrls = () => { return proxyServer.requestUrls.filter(u => !u.includes('favicon')); }; { + proxyServer.requestUrls = []; + const page = await context.newPage(); + await page.goto('http://0.non.existent.domain.for.the.test/target.html'); + expect(proxyServer.requestUrls).toContain('http://0.non.existent.domain.for.the.test/target.html'); + expect(await page.title()).toBe('Served by the proxy'); + await page.close(); + } + + { + proxyServer.requestUrls = []; + const page = await context.newPage(); const error = await page.goto('http://1.non.existent.domain.for.the.test/target.html').catch(e => e); expect(nonFaviconUrls()).toEqual([]); expect(error.message).toBeTruthy(); - - // Make sure error page commits. - if (browserName === 'chromium') - await page.waitForURL('chrome-error://chromewebdata/'); - else if (browserName === 'firefox') - await page.waitForURL('http://1.non.existent.domain.for.the.test/target.html', { waitUntil: 'commit' }); + await page.close(); } { + proxyServer.requestUrls = []; + const page = await context.newPage(); const error = await page.goto('http://2.non.existent.domain.for.the.test/target.html').catch(e => e); expect(nonFaviconUrls()).toEqual([]); expect(error.message).toBeTruthy(); - - // Make sure error page commits. - if (browserName === 'chromium') - await page.waitForURL('chrome-error://chromewebdata/'); - else if (browserName === 'firefox') - await page.waitForURL('http://2.non.existent.domain.for.the.test/target.html', { waitUntil: 'commit' }); + await page.close(); } { + proxyServer.requestUrls = []; + const page = await context.newPage(); const error = await page.goto('http://foo.is.the.another.test/target.html').catch(e => e); expect(nonFaviconUrls()).toEqual([]); expect(error.message).toBeTruthy(); - - // Make sure error page commits. - if (browserName === 'chromium') - await page.waitForURL('chrome-error://chromewebdata/'); - else if (browserName === 'firefox') - await page.waitForURL('http://foo.is.the.another.test/target.html', { waitUntil: 'commit' }); + await page.close(); } { + proxyServer.requestUrls = []; + const page = await context.newPage(); await page.goto('http://3.non.existent.domain.for.the.test/target.html'); expect(nonFaviconUrls()).toContain('http://3.non.existent.domain.for.the.test/target.html'); expect(await page.title()).toBe('Served by the proxy'); + await page.close(); } await context.close(); diff --git a/tests/library/tracing.spec.ts b/tests/library/tracing.spec.ts index 4596c8cf00..9ac0eba2e8 100644 --- a/tests/library/tracing.spec.ts +++ b/tests/library/tracing.spec.ts @@ -237,14 +237,24 @@ test('should respect tracesDir and name', async ({ browserType, server, mode }, }); test('should not include trace resources from the previous chunks', async ({ context, page, server, browserName, mode }, testInfo) => { - test.skip(browserName !== 'chromium', 'The number of screenshots is flaky in non-Chromium'); - test.skip(mode.startsWith('service'), 'The number of screenshots is flaky'); await context.tracing.start({ screenshots: true, snapshots: true, sources: true }); await context.tracing.startChunk(); await page.goto(server.EMPTY_PAGE); - await page.setContent(''); - await page.click('"Click"'); + await page.setContent(` + + + `); + await page.click('"Click"', { force: true }); // Give it enough time for both screenshots to get into the trace. await new Promise(f => setTimeout(f, 3000)); await context.tracing.stopChunk({ path: testInfo.outputPath('trace1.zip') }); @@ -252,11 +262,13 @@ test('should not include trace resources from the previous chunks', async ({ con await context.tracing.startChunk(); await context.tracing.stopChunk({ path: testInfo.outputPath('trace2.zip') }); + let jpegs: string[] = []; { const { resources } = await parseTraceRaw(testInfo.outputPath('trace1.zip')); const names = Array.from(resources.keys()); expect(names.filter(n => n.endsWith('.html')).length).toBe(1); - expect(names.filter(n => n.endsWith('.jpeg')).length).toBeGreaterThan(0); + jpegs = names.filter(n => n.endsWith('.jpeg')); + expect(jpegs.length).toBeGreaterThan(0); // 1 source file for the test. expect(names.filter(n => n.endsWith('.txt')).length).toBe(1); } @@ -266,8 +278,9 @@ test('should not include trace resources from the previous chunks', async ({ con const names = Array.from(resources.keys()); // 1 network resource should be preserved. expect(names.filter(n => n.endsWith('.html')).length).toBe(1); - expect(names.filter(n => n.endsWith('.jpeg')).length).toBe(0); - // 0 source file for the second test. + // screenshots from the previous chunk should not be preserved. + expect(names.filter(n => jpegs.includes(n)).length).toBe(0); + // 0 source files for the second test. expect(names.filter(n => n.endsWith('.txt')).length).toBe(0); } });