fix(recorder): remove waitForNavigation from codegen (#14166)

2022-05-18 10:02:09 -07:00 · 2022-05-18 10:02:09 -07:00 · d5ea1b38f0
--- a/packages/playwright-core/src/server/recorder/codeGenerator.ts
+++ b/packages/playwright-core/src/server/recorder/codeGenerator.ts
@ -136,7 +136,6 @@ export class CodeGenerator extends EventEmitter {
        return;
      if (signal.name === 'download' && signals.length && signals[signals.length - 1].name === 'navigation')
        signals.length = signals.length - 1;
-      signal.isAsync = true;
      this._lastAction.action.signals.push(signal);
      this.emit('change');
      return;
--- a/packages/playwright-core/src/server/recorder/csharp.ts
+++ b/packages/playwright-core/src/server/recorder/csharp.ts
@ -70,17 +70,7 @@ export class CSharpLanguageGenerator implements LanguageGenerator {

    const lines: string[] = [];
    const actionCall = this._generateActionCall(action, actionInContext.frame.isMainFrame);
-    if (signals.waitForNavigation) {
-      lines.push(`await ${pageAlias}.RunAndWaitForNavigationAsync(async () =>`);
-      lines.push(`{`);
-      lines.push(`    await ${subject}.${actionCall};`);
-      lines.push(`}/*, new ${actionInContext.frame.isMainFrame ? 'Page' : 'Frame'}WaitForNavigationOptions`);
-      lines.push(`{`);
-      lines.push(`    UrlString = ${quote(signals.waitForNavigation.url)}`);
-      lines.push(`}*/);`);
-    } else {
-      lines.push(`await ${subject}.${actionCall};`);
-    }
+    lines.push(`await ${subject}.${actionCall};`);

    if (signals.download) {
      lines.unshift(`var download${signals.download.downloadAlias} = await ${pageAlias}.RunAndWaitForDownloadAsync(async () =>\n{`);
@ -96,7 +86,7 @@ export class CSharpLanguageGenerator implements LanguageGenerator {
      formatter.add(line);

    if (signals.assertNavigation)
-      formatter.add(`  // Assert.AreEqual(${quote(signals.assertNavigation.url)}, ${pageAlias}.Url);`);
+      formatter.add(`await ${pageAlias}.WaitForURLAsync(${quote(signals.assertNavigation.url)});`);
    return formatter.format();
  }

--- a/packages/playwright-core/src/server/recorder/java.ts
+++ b/packages/playwright-core/src/server/recorder/java.ts
@ -81,18 +81,10 @@ export class JavaLanguageGenerator implements LanguageGenerator {
      });`;
    }

-    if (signals.waitForNavigation) {
-      code = `
-      // ${pageAlias}.waitForNavigation(new Page.WaitForNavigationOptions().setUrl(${quote(signals.waitForNavigation.url)}), () ->
-      ${pageAlias}.waitForNavigation(() -> {
-        ${code}
-      });`;
-    }
-
    formatter.add(code);

    if (signals.assertNavigation)
-      formatter.add(`// assertThat(${pageAlias}).hasURL(${quote(signals.assertNavigation.url)});`);
+      formatter.add(`assertThat(${pageAlias}).hasURL(${quote(signals.assertNavigation.url)});`);
    return formatter.format();
  }

--- a/packages/playwright-core/src/server/recorder/javascript.ts
+++ b/packages/playwright-core/src/server/recorder/javascript.ts
@ -75,7 +75,7 @@ export class JavaScriptLanguageGenerator implements LanguageGenerator {
  });`);
    }

-    const emitPromiseAll = signals.waitForNavigation || signals.popup || signals.download;
+    const emitPromiseAll = signals.popup || signals.download;
    if (emitPromiseAll) {
      // Generate either await Promise.all([]) or
      // const [popup1] = await Promise.all([]).
@ -91,26 +91,22 @@ export class JavaScriptLanguageGenerator implements LanguageGenerator {
    if (signals.popup)
      formatter.add(`${pageAlias}.waitForEvent('popup'),`);

-    // Navigation signal.
-    if (signals.waitForNavigation)
-      formatter.add(`${pageAlias}.waitForNavigation(/*{ url: ${quote(signals.waitForNavigation.url)} }*/),`);
-
    // Download signals.
    if (signals.download)
      formatter.add(`${pageAlias}.waitForEvent('download'),`);

-    const prefix = (signals.popup || signals.waitForNavigation || signals.download) ? '' : 'await ';
+    const prefix = (signals.popup || signals.download) ? '' : 'await ';
    const actionCall = this._generateActionCall(action);
-    const suffix = (signals.waitForNavigation || emitPromiseAll) ? '' : ';';
+    const suffix = emitPromiseAll ? '' : ';';
    formatter.add(`${prefix}${subject}.${actionCall}${suffix}`);

    if (emitPromiseAll) {
      formatter.add(`]);`);
    } else if (signals.assertNavigation) {
      if (this._isTest)
-        formatter.add(`  await expect(${pageAlias}).toHaveURL(${quote(signals.assertNavigation.url)});`);
+        formatter.add(`await expect(${pageAlias}).toHaveURL(${quote(signals.assertNavigation.url)});`);
      else
-        formatter.add(`  // assert.equal(${pageAlias}.url(), ${quote(signals.assertNavigation.url)});`);
+        formatter.add(`await ${pageAlias}.waitForURL(${quote(signals.assertNavigation.url)});`);
    }
    return formatter.format();
  }
--- a/packages/playwright-core/src/server/recorder/language.ts
+++ b/packages/playwright-core/src/server/recorder/language.ts
@ -47,15 +47,12 @@ export function sanitizeDeviceOptions(device: any, options: BrowserContextOption
 }

 export function toSignalMap(action: Action) {
-  let waitForNavigation: NavigationSignal | undefined;
  let assertNavigation: NavigationSignal | undefined;
  let popup: PopupSignal | undefined;
  let download: DownloadSignal | undefined;
  let dialog: DialogSignal | undefined;
  for (const signal of action.signals) {
-    if (signal.name === 'navigation' && signal.isAsync)
-      waitForNavigation = signal;
-    else if (signal.name === 'navigation' && !signal.isAsync)
+    if (signal.name === 'navigation')
      assertNavigation = signal;
    else if (signal.name === 'popup')
      popup = signal;
@ -65,7 +62,6 @@ export function toSignalMap(action: Action) {
      dialog = signal;
  }
  return {
-    waitForNavigation,
    assertNavigation,
    popup,
    download,
--- a/packages/playwright-core/src/server/recorder/python.ts
+++ b/packages/playwright-core/src/server/recorder/python.ts
@ -95,18 +95,14 @@ export class PythonLanguageGenerator implements LanguageGenerator {
      download = ${this._awaitPrefix}download_info.value`;
    }

-    if (signals.waitForNavigation) {
-      code = `
-      # ${this._asyncPrefix}with ${pageAlias}.expect_navigation(url=${quote(signals.waitForNavigation.url)}):
-      ${this._asyncPrefix}with ${pageAlias}.expect_navigation() {
-        ${code}
-      }`;
-    }
-
    formatter.add(code);

-    if (signals.assertNavigation)
-      formatter.add(`  # ${this._awaitPrefix}expect(${pageAlias}).to_have_url(${quote(signals.assertNavigation.url)})`);
+    if (signals.assertNavigation) {
+      if (this._isPyTest)
+        formatter.add(`${this._awaitPrefix}expect(${pageAlias}).to_have_url(${quote(signals.assertNavigation.url)})`);
+      else
+        formatter.add(`${this._awaitPrefix}${pageAlias}.wait_for_url(${quote(signals.assertNavigation.url)})`);
+    }
    return formatter.format();
  }

--- a/packages/playwright-core/src/server/recorder/recorderActions.ts
+++ b/packages/playwright-core/src/server/recorder/recorderActions.ts
@ -96,7 +96,6 @@ export type Action = ClickAction | CheckAction | ClosesPageAction | OpenPageActi
 // Signals.

 export type BaseSignal = {
-  isAsync?: boolean,
 };

 export type NavigationSignal = BaseSignal & {
--- a/tests/library/inspector/cli-codegen-1.spec.ts
+++ b/tests/library/inspector/cli-codegen-1.spec.ts
@ -607,39 +607,44 @@ test.describe('cli codegen', () => {
    expect(selector).toBe('text=link');
    const [, sources] = await Promise.all([
      page.waitForNavigation(),
-      recorder.waitForOutput('JavaScript', 'assert'),
+      recorder.waitForOutput('JavaScript', 'waitForURL'),
      page.dispatchEvent('a', 'click', { detail: 1 })
    ]);

-    expect(sources.get('JavaScript').text).toContain(`
+    expect.soft(sources.get('JavaScript').text).toContain(`
  // Click text=link
  await page.locator('text=link').click();
-  // assert.equal(page.url(), 'about:blank#foo');`);
+  await page.waitForURL('about:blank#foo');`);

-    expect(sources.get('Playwright Test').text).toContain(`
+    expect.soft(sources.get('Playwright Test').text).toContain(`
  // Click text=link
  await page.locator('text=link').click();
  await expect(page).toHaveURL('about:blank#foo');`);

-    expect(sources.get('Java').text).toContain(`
+    expect.soft(sources.get('Java').text).toContain(`
      // Click text=link
      page.locator("text=link").click();
-      // assertThat(page).hasURL("about:blank#foo");`);
+      assertThat(page).hasURL("about:blank#foo");`);

-    expect(sources.get('Python').text).toContain(`
+    expect.soft(sources.get('Python').text).toContain(`
    # Click text=link
-    page.locator(\"text=link\").click()
-    # expect(page).to_have_url(\"about:blank#foo\")`);
+    page.locator("text=link").click()
+    page.wait_for_url("about:blank#foo")`);

-    expect(sources.get('Python Async').text).toContain(`
+    expect.soft(sources.get('Python Async').text).toContain(`
    # Click text=link
-    await page.locator(\"text=link\").click()
-    # await expect(page).to_have_url(\"about:blank#foo\")`);
+    await page.locator("text=link").click()
+    await page.wait_for_url("about:blank#foo")`);

-    expect(sources.get('C#').text).toContain(`
+    expect.soft(sources.get('Pytest').text).toContain(`
+    # Click text=link
+    page.locator("text=link").click()
+    expect(page).to_have_url("about:blank#foo")`);
+
+    expect.soft(sources.get('C#').text).toContain(`
        // Click text=link
-        await page.Locator(\"text=link\").ClickAsync();
-        // Assert.AreEqual(\"about:blank#foo\", page.Url);`);
+        await page.Locator("text=link").ClickAsync();
+        await page.WaitForURLAsync("about:blank#foo");`);

    expect(page.url()).toContain('about:blank#foo');
  });
@ -655,45 +660,34 @@ test.describe('cli codegen', () => {

    const [, sources] = await Promise.all([
      page.waitForNavigation(),
-      recorder.waitForOutput('JavaScript', 'waitForNavigation'),
+      recorder.waitForOutput('JavaScript', 'waitForURL'),
      page.dispatchEvent('a', 'click', { detail: 1 })
    ]);

-    expect(sources.get('JavaScript').text).toContain(`
+    expect.soft(sources.get('JavaScript').text).toContain(`
  // Click text=link
-  await Promise.all([
-    page.waitForNavigation(/*{ url: 'about:blank#foo' }*/),
-    page.locator('text=link').click()
-  ]);`);
+  await page.locator('text=link').click();
+  await page.waitForURL('about:blank#foo');`);

-    expect(sources.get('Java').text).toContain(`
+    expect.soft(sources.get('Java').text).toContain(`
      // Click text=link
-      // page.waitForNavigation(new Page.WaitForNavigationOptions().setUrl("about:blank#foo"), () ->
-      page.waitForNavigation(() -> {
-        page.locator("text=link").click();
-      });`);
+      page.locator("text=link").click();
+      assertThat(page).hasURL("about:blank#foo");`);

-    expect(sources.get('Python').text).toContain(`
+    expect.soft(sources.get('Python').text).toContain(`
    # Click text=link
-    # with page.expect_navigation(url=\"about:blank#foo\"):
-    with page.expect_navigation():
-        page.locator(\"text=link\").click()`);
+    page.locator(\"text=link\").click()
+    page.wait_for_url("about:blank#foo")`);

-    expect(sources.get('Python Async').text).toContain(`
+    expect.soft(sources.get('Python Async').text).toContain(`
    # Click text=link
-    # async with page.expect_navigation(url=\"about:blank#foo\"):
-    async with page.expect_navigation():
-        await page.locator(\"text=link\").click()`);
+    await page.locator(\"text=link\").click()
+    await page.wait_for_url("about:blank#foo")`);

-    expect(sources.get('C#').text).toContain(`
+    expect.soft(sources.get('C#').text).toContain(`
        // Click text=link
-        await page.RunAndWaitForNavigationAsync(async () =>
-        {
-            await page.Locator(\"text=link\").ClickAsync();
-        }/*, new PageWaitForNavigationOptions
-        {
-            UrlString = \"about:blank#foo\"
-        }*/);`);
+        await page.Locator(\"text=link\").ClickAsync();
+        await page.WaitForURLAsync(\"about:blank#foo\");`);

    expect(page.url()).toContain('about:blank#foo');
  });
--- a/tests/library/inspector/cli-codegen-2.spec.ts
+++ b/tests/library/inspector/cli-codegen-2.spec.ts
@ -532,10 +532,10 @@ test.describe('cli codegen', () => {
    const [, sources] = await Promise.all([
      // This will click, finish the click, then mouse move, then navigate.
      page.click('button'),
-      recorder.waitForOutput('JavaScript', 'waitForNavigation'),
+      recorder.waitForOutput('JavaScript', 'waitForURL'),
    ]);

-    expect(sources.get('JavaScript').text).toContain(`page.waitForNavigation(/*{ url: '${server.EMPTY_PAGE}' }*/)`);
+    expect(sources.get('JavaScript').text).toContain(`page.waitForURL('${server.EMPTY_PAGE}')`);
  });

  test('should --save-trace', async ({ runCLI }, testInfo) => {
--- a/tests/library/inspector/inspectorTest.ts
+++ b/tests/library/inspector/inspectorTest.ts
@ -115,18 +115,12 @@ class Recorder {
  }

  async waitForOutput(file: string, text: string): Promise<Map<string, Source>> {
-    const sources: Source[] = await this.recorderPage.evaluate((params: { text: string, file: string }) => {
+    const handle = await this.recorderPage.waitForFunction((params: { text: string, file: string }) => {
      const w = window as any;
-      return new Promise(f => {
-        const poll = () => {
-          const source = (w.playwrightSourcesEchoForTest || []).find((s: Source) => s.file === params.file);
-          if (source && source.text.includes(params.text))
-            f(w.playwrightSourcesEchoForTest);
-          setTimeout(poll, 300);
-        };
-        poll();
-      });
-    }, { text, file });
+      const source = (w.playwrightSourcesEchoForTest || []).find((s: Source) => s.file === params.file);
+      return source && source.text.includes(params.text) ? w.playwrightSourcesEchoForTest : null;
+    }, { text, file }, { timeout: 8000, polling: 300 });
+    const sources: Source[] = await handle.jsonValue();
    for (const source of sources)
      this._sources.set(source.file, source);
    return this._sources;