microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
66.46k stars 3.63k forks source link

[BUG] ipcRenderer SendSync returning null / undefined - Electron #10105

Closed evertonlperes closed 2 years ago

evertonlperes commented 2 years ago

Context:

Code Snippet

import path from 'path';
import { ElectronApplication, _electron, Page } from 'playwright';
import { test, expect } from '@playwright/test';

/**
 * Using test.describe.serial make the test execute step by step, as described on each `test()` order
 * Playwright executes test in parallel by default and it will not work for our app backend loading process.
 * */

test.describe.serial('POC Playwright - RD, () => {
  let page: Page;
  let electronApp: ElectronApplication;
  const mainTitleSelector = '[data-test="mainTitle"]';

  test.beforeAll(async({ browser }) => {
    electronApp = await _electron.launch({ args: [path.join(__dirname, '../')] });
    const appPath = await electronApp.evaluate(async({ app }) => {
      return await app.getAppPath();
    });

    app = await browser.newPage();
    console.log('Log from appPath ---> ', appPath);
  });

  test.afterAll(async() => {
    await electronApp.close();
  });

  test('should open the main app', async() => {
    page = await electronApp.firstWindow();
    await page.waitForSelector('.progress', { state: 'visible' });
    await delay(20000); // Wait a bit

    const versionApp = await app.$eval('.versionInfo', el => el.textContent);

    expect(versionApp).toBe('Version: (checking...)');
  });

  test('should get General page content', async() => {
    const generalGreetings = await page.$eval(mainTitleSelector, el => el.textContent.trim());

    expect(generalGreetings).toBe('Welcome to RD');
  });

  test('should navigate to K Settings', async() => {
    const kVersionDndSelector = '.labeled-input';
    const kMemorySliderSelector = '[id="memoryInGBWrapper"]';

    try {
      page.click(`.nav li[item="/Page2"] a`);
      await page.waitForSelector('.contents');
    } catch (err) {
      console.log('Error during K Settings navigation. Error --> ', err);
    }

    const kSettingsTitle = await page.$eval(mainTitleSelector, el => el.textContent.trim());
    const kVersionDnd = await page.$eval(kVersionDndSelector, el => el.textContent.trim());

    expect(k8sVersionDnd).toBe('K version');
    expect(k8sSettingsTitle).toBe('K Settings');
  });
});

function delay(time: number | undefined) {
  return new Promise((resolve) => {
    setTimeout(resolve, time);
  });
}

Create Window config

 createWindow('preferences', `${ webRoot }/index.html`, {
    width:          940,
    height:         600,
    webPreferences: {
      devTools:           !app.isPackaged,
      nodeIntegration:    true,
      contextIsolation:   false,
      enableRemoteModule: process.env?.NODE_ENV === 'test'
    },
  });

Log playwight_debug_ipcnull.log

Describe the bug

There's intermittence during the E2E tests where the main app hangs during the ipcRenderer.sendSync() process, returning Cannot read property '[item]' of null. It happens quite often, I'd say 70% of the time, executing it through playwright. The weird behaviour is: running it on debug mode PWDEBUG=1 (without any page.pause()), the error does not shows up on the same frequency. Tried to include some delays after the app start the first page, but it does not work. I thought it could be related with some racing conditions between the app vs playwright or something like that. I haven't seen any output from the logs that could assist to find out what's going on :/

This is the first time running E2E testing on Electron + Playwright.

Any thoughts will be very appreciated.

mxschmitt commented 2 years ago

I would recommend to upgrade to Electron 12.2.2+ since Playwright has now higher Electron requirements see here: https://github.com/microsoft/playwright/pull/8759#issuecomment-933548946

Once this is ensured we'll take a look.

evertonlperes commented 2 years ago

Thanks for replying @mxschmitt Just tweaked the Electron version from 12.0.18 to 12.2.2 and it was possible to replicate the same issue:

 npm ls electron
r-d@latest /Users/evertonperes/projects/rd
├── electron@12.2.2
└─┬ spectron@14.0.0
  └─┬ @electron/remote@1.2.1
    └── electron@12.2.2 deduped

Screen Shot 2021-11-08 at 9 32 13 AM

Update: Did a step forward and tweaked the Electron version to 13.6.1 and still facing the same issue (see playwright_electron_13_pw.log).

r-d@latest /Users/evertonperes/projects/rd
├── electron@13.6.1
└─┬ spectron@15.0.0
  └─┬ @electron/remote@1.2.1
    └── electron@13.6.1 deduped

Log wdio.log chromedriver.log playwright_electron_13_pw.log

pavelfeldman commented 2 years ago

Let's fix your script a little before we get to the actual issue.

test.describe.serial('POC Playwright - RD, () => {
  let page: Page;
  let electronApp: ElectronApplication;
  // const mainTitleSelector = '[data-test="mainTitle"]'; // <-- Use locators, not selectors for your page object model
  const mainTitle: Locator;

  test.beforeAll(async({ browser }) => { // <-- REVIEW when you say `browser` here, we actually run separate chrome for your, remove it
    electronApp = await _electron.launch({ args: [path.join(__dirname, '../')] });
    const appPath = await electronApp.evaluate(async({ app }) => {
      return await app.getAppPath();
    });

    // app = await browser.newPage(); <-- REVIEW this was creating a page in that separate chrome, unrelated to Electron.
    console.log('Log from appPath ---> ', appPath);
  });

  test.afterAll(async() => {
    await electronApp.close();
  });

  test('should open the main app', async() => {
    page = await electronApp.firstWindow();
    mainTitle = page.locator('[data-test="mainTitle"]');
    // await page.waitForSelector('.progress', { state: 'visible' }); // <-- visible is default, also you don't seem to need it
    // await delay(20000); // <-- REVIEW never ever do that
    // const versionApp = await app.$eval('.versionInfo', el => el.textContent); // <-- REVIEW never use ElementHandles
    // expect(versionApp).toBe('Version: (checking...)'); // <-- REVIEW don't expect 

    // REVIEW These two lines will wait for version info to become Version (checking)
    const versionInfo = page.locator('.versionInfo');
    await expect(versionInfo).toHaveText('Version: (checking...)');

  });

  test('should get General page content', async() => {
    // const generalGreetings = await page.$eval(mainTitleSelector, el => el.textContent.trim());
    // expect(generalGreetings).toBe('Welcome to RD');
    // REVIEW This one line will wait for expected value
    await expect(mainTitle).toHaveText('Welcome to RD');
  });

  test('should navigate to K Settings', async() => {
    const kVersionDndSelector = '.labeled-input';
    const kMemorySliderSelector = '[id="memoryInGBWrapper"]';

    try {
      page.click(`.nav li[item="/Page2"] a`); // <-- always await promises, otherwise you have unhandled promise rejection
      await page.waitForSelector('.contents');
    } catch (err) {
      console.log('Error during K Settings navigation. Error --> ', err);
    }

//    const kSettingsTitle = await page.$eval(mainTitleSelector, el => el.textContent.trim());
//    const kVersionDnd = await page.$eval(kVersionDndSelector, el => el.textContent.trim());
    await expect(page.locator('.labeled-input')).toHaveText('K version');
    await expect(mainTitle).toHaveText('K Settings');

//    expect(k8sVersionDnd).toBe('K version');
//    expect(k8sSettingsTitle).toBe('K Settings');
  });
});

function delay(time: number | undefined) {
  return new Promise((resolve) => {
    setTimeout(resolve, time);
  });
}
pavelfeldman commented 2 years ago

You get the idea, use locators :) This will get rid of various races and unhandled exceptions, but i don't think it'll fix your issue. Once the script is good we can look further.

evertonlperes commented 2 years ago

@pavelfeldman Really appreciate your help and your code comments, here the changes that I did based on your comments:

import path from 'path';
import { ElectronApplication, _electron, Page, Locator } from 'playwright';
import { test, expect } from '@playwright/test';

/**
 * Using test.describe.serial make the test execute step by step, as described on each `test()` order
 * Playwright executes test in parallel by default and it will not work for our app backend loading process.
 * */
let page: Page;
let electronApp: ElectronApplication;

test.describe.serial('POC Playwright - RD', () => {
  let mainTitle: Locator;
  const mainTitleSelector = '[data-test="mainTitle"]'; // <-- Keeping this for global selector across all the tabs

  test.beforeAll(async() => {
    electronApp = await _electron.launch({ args: [path.join(__dirname, '../')] });
    const appPath = await electronApp.evaluate(async({ app }) => {
      return await app.getAppPath();
    });

    console.log('Log from appPath ---> ', appPath);
  });

  test.afterAll(async() => {
    await electronApp.close();
  });

  test('should open the main app', async() => {
    page = await electronApp.firstWindow();
    await page.waitForSelector('.progress');

    const versionInfo = page.locator('.versionInfo');

    await expect(versionInfo).toHaveText('Version: (checking...)');
  });

  test('should get General page content', async() => {
    mainTitle = page.locator(mainTitleSelector);

    await await expect(mainTitle).toHaveText('Welcome to RD');
  });

  test('should navigate to K Settings and check elements', async() => {

    await navigateTo('Ks'); // <-- helper to navigate through other tabs

    // Collecting data from selectors
    const kSettingsTitle = page.locator(mainTitleSelector);

    await expect(k8sSettingsTitle).toHaveText('K Settings');
  });

/**
 * Navigate to a specific tab
 * @param path
 */
async function navigateTo(path: string) {
  try {
    return await Promise.all([
      page.click(`.nav li[item="/${ path }"] a`),
      page.waitForNavigation({ url: `**/${ path }` })
    ]);
  } catch (err) {
    console.log(`Cannot navigate to ${ path }. Error ---> `, err);
  }
}

Using locator for sure it's the best approach (I can see the difference). Even though using locator, I still getting the error Cannot read property '[item]' of null :(

JoelEinbinder commented 2 years ago

While you provided a lot of logs, none show the error you are talking about. If you have a stack trace to show that the error is coming from playwright, I can look at the playwright code.

evertonlperes commented 2 years ago

TLDR from stack trace:

        at /Users/evertonperes/projects/rd/e2e/playwright.spec.ts:78:33
        at WorkerRunner._runTestWithBeforeHooks (/Users/evertonperes/projects/rd/node_modules/@playwright/test/lib/workerRunner.js:478:7)

Here's the entire logs from PW execution:

Using config at /Users/evertonperes/projects/rd/e2e/play-config.ts

Running 6 tests using 1 worker

Log from appPath --->  /Users/evertonperes/projects/rd
  ✓  playwright.spec.ts:30:3 › POC Playwright - RD › should open the main app (12s)
  ✓  playwright.spec.ts:44:3 › POC Playwright - RD › should get General page content (17ms)
  ✘  playwright.spec.ts:50:3 › POC Playwright - RD › should navigate to K Settings and check e
  ✘  playwright.spec.ts:88:5 › POC Playwright - RD › should navigate to Supporting Utilities anc check
  -  playwright.spec.ts:102:3 › POC Playwright - RD › should navigate to Images page
  -  playwright.spec.ts:116:3 › POC Playwright - RD › should navigate to Troubleshooting and check elem

  1) playwright.spec.ts:50:3 › POC Playwright - RD › should navigate to K Settings and check elements

    Error: expect(received).toBeVisible()

    Call log:
      - waiting for selector ".labeled-input"

      76 |
      77 |     await expect(kSettingsTitle).toHaveText('K Settings');
    > 78 |     await expect(kVersionDnd).toBeVisible();
         |                                 ^
      79 |     await expect(kMemorySlider).toBeVisible();
      80 |     await expect(kCpuSlider).toBeVisible();
      81 |     await expect(kPort).toBeVisible();

        at /Users/evertonperes/projects/rd/e2e/playwright.spec.ts:78:33
        at WorkerRunner._runTestWithBeforeHooks (/Users/evertonperes/projects/rd/node_modules/@playwright/test/lib/workerRunner.js:478:7)

  2) playwright.spec.ts:88:5 › POC Playwright - RD › should navigate to Supporting Utilities anc check elements

    electronApplication.close: Timeout 30000ms exceeded.

      25 |
      26 |   test.afterAll(async() => {
    > 27 |     await electronApp.close();
         |                       ^
      28 |   });
      29 |
      30 |   test('should open the main app', async() => {

        at /Users/evertonperes/projects/rd/e2e/playwright.spec.ts:27:23

  3) playwright.spec.ts:102:3 › POC Playwright - RD › should navigate to Images page ==

    electronApplication.close: Timeout 30000ms exceeded.

      25 |
      26 |   test.afterAll(async() => {
    > 27 |     await electronApp.close();
         |                       ^
      28 |   });
      29 |
      30 |   test('should open the main app', async() => {

        at /Users/evertonperes/projects/rd/e2e/playwright.spec.ts:27:23

  4) playwright.spec.ts:116:3 › POC Playwright - RD › should navigate to Troubleshooting and check elements

    electronApplication.close: Timeout 30000ms exceeded.

      25 |
      26 |   test.afterAll(async() => {
    > 27 |     await electronApp.close();
         |                       ^
      28 |   });
      29 |
      30 |   test('should open the main app', async() => {

        at /Users/evertonperes/projects/rd/e2e/playwright.spec.ts:27:23

  Slow test: playwright.spec.ts (18s)

  2 failed
    playwright.spec.ts:50:3 › POC Playwright - RD › should navigate to K Settings and check elements
    playwright.spec.ts:88:5 › POC Playwright - RD › should navigate to Supporting Utilities and check elements
  2 skipped
  2 passed (56s)
RD: main process exited with status 1
1

Image from the Electron app: Screen Shot 2021-11-18 at 4 09 22 PM

A bit more context, the property kubernetes should be a value from collected from a method ipcRenderer.sendSync('read-config'). The config variable is an array (strings and integers) with a bunch of k8s config, such as cpu qty, memory qty and etc.

I'm suspecting it could be related to race conditions between Electron and PW, but I'm not 100% sure, because this error is intermittent (~60% failure rate).

Hope it helps a bit and happy to give you more information if you need.

JoelEinbinder commented 2 years ago

This doesn't look like it has anything to do with playwright. Your app doesn't load so playwright correctly reports that the items are not visible. Unfortunately I cannot help debug your app, so I don't know whats going on with your kubernetes variable. Closing this as is not actionable from us.