playwright-testing-library icon indicating copy to clipboard operation
playwright-testing-library copied to clipboard

Expect assertions can't be used due to returning ElementHandles instead of Locators

Open ScubaDaniel opened this issue 3 years ago • 22 comments

I was really excited to use this library/plugin, but I'm not sure I see the value when the standard Playwright expect assertions aren't supported for ElementHandles? I know this is a documented limitation, but I'm confused on the point of this module when expect assertions can't be used.

Can you clarify the reason you'd want to use ElementHandles instead of Locators? It seems like Playwright discourages ElementHandles.

ScubaDaniel avatar Jun 01 '22 21:06 ScubaDaniel

Hey, @ScubaDaniel thanks for creating an issue. The reason this library uses ElementHandles is that Locators did not exist when it was originally created. We've been working on a new API that returns Locators and leverages more @playwright/test features, there are just a few things I still need to sort out before we release it officially.

yarn add -D @playwright-testing-library/[email protected]

I went ahead and released a beta version for you here. The readme hasn't been updated there yet, though, so here's a snippet from one of my commits with a basic usage example:

This will likely replace the fixtures that provided ElementHandle-based queries in a future major release, but for now the Locator queries are exported as locatorFixtures:

import { test as baseTest } from '@playwright/test'
import {
  locatorFixtures as fixtures,
  LocatorFixtures as TestingLibraryFixtures,
  within
} from '@playwright-testing-library/test/fixture';

const test = baseTest.extend<TestingLibraryFixtures>(fixtures);

const {expect} = test;

test('my form', async ({queries: {getByTestId}}) => {
  // Queries now return `Locator`
  const formLocator = getByTestId('my-form');

  // Locator-based `within` support
  const {getByLabelText} = within(formLocator);

  const emailInputLocator = getByLabelText('Email');

  // Interact via `Locator` API 🥳
  await emailInputLocator.fill('[email protected]');

  // Assert via `Locator` APIs 🎉
  await expect(emailInputLocator).toHaveValue('[email protected]');
})

You can also reference the tests for more examples.

Let me know if you have any issues with that release, and feel free to provide any feedback in the PR → https://github.com/testing-library/playwright-testing-library/pull/403

Ah, one other thing is that the configure API is not yet implemented, but I have it mostly there, so let me know if you need that.

jrolfs avatar Jun 01 '22 22:06 jrolfs

Thanks so much for the quick reply!

I consumed the beta release, but I'm getting an error trying to use it like you've shown. I don't know it if matters, but I'm using Playwright's new Components feature.

Test:

import { test as baseTest } from "@playwright/experimental-ct-react";
import {
  locatorFixtures as fixtures,
  LocatorFixtures as TestingLibraryFixtures,
  within,
} from "@playwright-testing-library/test/fixture";
import React from "react";
import HeaderBanner from "../header-banner";

const test = baseTest.extend<TestingLibraryFixtures>(fixtures);
const { expect } = test;

test("HeaderBanner displays username", async ({ mount, queries: { getByTestId } }) => {
  const username = "[email protected]";
  const component = await mount(<HeaderBanner signInUsername={username} />);
  const headerBanner = getByTestId("headerBanner");

  await expect(headerBanner).toContainText(username);
});

Error:

  1) [chromium] › components\__integration__\header-banner.test.tsx:13:1 › HeaderBanner displays username 

    locator._highlight: Unknown engine "get-by-test-id" while parsing selector get-by-test-id=["headerBanner"]

      18 |   const headerBanner = getByTestId("headerBanner");
      19 |
    > 20 |   await expect(headerBanner).toContainText(username);
         |   ^
      21 | });
      22 |

ScubaDaniel avatar Jun 03 '22 21:06 ScubaDaniel

@ScubaDaniel yeah, it seems like the issue is, in fact, the experimental mount() fixture. I set up a little sandbox application to work through this, and I did find a workaround for now.

Now that I'm looking at your error message again, though, I'm realizing that I actually ran into a different issue. In my case, it seems the issue is that the automatic fixture we use to load @testing-library/dom into the browser context isn't working correctly. To work around this for now, I simply loaded the required contextual JavaScript in the index.html:

Docs: https://playwright.dev/docs/test-components#playwrightindexhtml + https://playwright.dev/docs/test-components#playwrightindexts Example: https://github.com/jrolfs/playwright-testing-library-component-example/blob/main/playwright/index.ts

import * as TestingLibraryDom from '@testing-library/dom';

const reviver = (_: string, value: string) => {
  if (value.toString().includes('__REGEXP ')) {
    const match = /\/(.*)\/(.*)?/.exec(value.split('__REGEXP ')[1]);

    return new RegExp(match![1], match![2] || '');
  }

  return value;
};

const attachTestingLibrary = () => {
  window.TestingLibraryDom = TestingLibraryDom;
  window.__testingLibraryReviver = reviver;
};

if (document.readyState === 'loading') {
  document.addEventListener('DOMContentLoaded', attachTestingLibrary);
} else {
  attachTestingLibrary();
}

With the above workaround, the mount(<App />) stuff seems to work just fine. I do worry that there's something else wrong with your setup, though. I'd suggest pulling down the example at https://github.com/jrolfs/playwright-testing-library-component-example, verify that it works for you as well (should just be yarn install and yarn test), and then compare the versions and configuration between your setup and the example.

Let me know where that gets you. This looks like a pretty sweet feature, so I'd like to make sure we support it eventually (ideally without this workaround). That said, it is experimental so this may actually be an issue on the Playwright side of things.

jrolfs avatar Jun 07 '22 01:06 jrolfs

Thank you so much for all this support, @jrolfs!

Due to another issue with the experimental Playwright feature, we've decided to not use that feature for now. We're pivoting to use Storybook + Playwright instead.

That being said, the beta release you provided is working great now that we're not using the mount function! Thanks so much!

ScubaDaniel avatar Jun 08 '22 18:06 ScubaDaniel

Hi, I've been playing around with 4.3.0-beta.1 and "@playwright/test": "1.22.2" and it's great!

I've found an edge case in the new expect(locator).toHaveScreenshot() method that throws.

This works:

 const form = queries.queryByTestId('form');
 await expect(form).toHaveScreenshot('form.png');

This throws:

const form = queries.queryByTestId('form');
const { queryByLabelText } = within(form);
const day = queryByLabelText('Day');
await expect(form).toHaveScreenshot('form-omit-day.png'', {
  mask: [day],
});
Error: Screenshot comparison failed:

      Timeout 10000ms exceeded.

    Call log:
      - expect.toHaveScreenshot with timeout 10000ms
      -   verifying given screenshot expectation
      - waiting for selector "query-by-test-id=["form"]"
      -   selector resolved to visible <form method="post" novalidate="" data-testid="form">…</form>
      - taking element screenshot
      -   disabled all CSS animations
      -   waiting for element to be visible and stable
      -   element is visible and stable
      - failed to take screenshot - TypeError: Cannot read properties of undefined (reading 'queryByTestId')
        at Object.queryAll (<anonymous>:5136:38)
        at InjectedScript._queryEngineAll (<anonymous>:3889:49)
        at InjectedScript.querySelectorAll (<anonymous>:3876:30)
        at InjectedScript.maskSelectors (<anonymous>:4505:26)
        at eval (eval at evaluate (:178:30), <anonymous>:4:16)
        at UtilityScript.evaluate (<anonymous>:180:17)
        at UtilityScript.<anonymous> (<anonymous>:1:44)

ghost avatar Jun 15 '22 10:06 ghost

(Removed comment as it was user error)

ScubaDaniel avatar Jun 17 '22 17:06 ScubaDaniel

@crazyvan25 can you open a new issue for the edge case you came across? It would be super helpful if you could put together a simple reproduction case similar to https://github.com/jrolfs/playwright-testing-library-component-example

jrolfs avatar Jun 22 '22 18:06 jrolfs

@jrolfs I think I found a potential issue.

I'm trying to use the following, but it doesn't work: https://playwright.dev/docs/api/class-locator#locator-wait-for

  const heading = getByRole("heading");
  await heading.waitFor();
  await expect(heading).toContainText("Stay signed in?");

ScubaDaniel avatar Jun 27 '22 19:06 ScubaDaniel

Hi @jrolfs, does the new API work with multiple pages?

I have the following use case where clicking a link opens up a new browser page:

test.only('Opens up a new page', async ({
        context,
        page,
        queries,
    }) => {
        await page.goto(SAR_URL);

        const [nextPage] = await Promise.all([
            context.waitForEvent('page'),
            await page.click(sarSelectors.downloadLink),
        ]);

        await expect(nextPage).toHaveURL(SIGNIN_REAUTHENTICATE_URL);

        const allPages = context.pages();
        expect(allPages.length).toBe(2);

       const locator = queries.queryByLabelText('Password');
       await expect(locator).toBeVisible(); // this times out
    });

ghost avatar Jul 05 '22 10:07 ghost

@jrolfs Is there a reason that findByRole doesn't exist in this library?

I also find that when I try to use findByText I'm getting TypeError: findByText is not a function. Any idea why that could be?

ScubaDaniel avatar Jul 05 '22 16:07 ScubaDaniel

@ScubaDaniel heh, you had me worried for a second there as the *ByRole queries are the most important part of Testing Library imo — the issue you're coming up against is that I don't think the findBy* queries really make sense with Locator's and the underlying selector engine API that we use to implement the Locator-based queries doesn't support asynchronous calls.

You can see my TODO here... 😬 https://github.com/testing-library/playwright-testing-library/blob/beta/test/fixture/locators.test.ts#L151

I think there's two action items here:

  1. Figure out the asynchronous story for @playwright/test + our locator queries
  2. Clearly document this so it's not confusing that findBy* doesn't exist

In looking back at your previous comment, it seems like you ended up looking for the findBy* methods after running into this problem trying to do it the Playwright way.

I'll try to take a look at this issue, as I was planning on supporting the findBy* cases using @playwright/test's built-in asynchronous support. Hopefully, once this is resolved, you should have no need for the findBy* queries.

@crazyvan25 I suspect your issue is actually related to this, I'll let you know when I have a chance to dig into this stuff.

jrolfs avatar Jul 06 '22 00:07 jrolfs

@jrolfs that makes sense! Thanks again for getting back so soon!

Our team has been using findBy* to check that a single element exists, per Kent C Dodd's direction for checking the presence of DOM nodes. I guess I'd still like to have a way to have a short syntax to have that check, but I suppose I could also build a fixture for that! Though I'm also not opposed to having findBy* returning an ElementHandle, as that just logically makes sense.

ScubaDaniel avatar Jul 07 '22 16:07 ScubaDaniel

Though I'm also not opposed to having findBy* returning an ElementHandle, as that just logically makes sense.

Why not have it return a Promise<Locator> if-and-only-if the .length of a resolved Locator#elementHandles() equals 1 but reject otherwise.

.findBy*: Returns a Promise which resolves when an element is found which matches the given query. The promise is rejected if no element is found or if more than one element is found after a default timeout of 1000ms. If you need to find more than one element, use .findAllBy*.

See: https://testing-library.com/docs/queries/about/

That way a .findBy* is kind of an async assertion that a Locator points towards something on the page but you're still given a Locator back so can continue to use Playwright's copious expect(locator: Locator) APIs.

Edit: I don't know if this makes sense to others. It feels relatively consistent to me for it to work this way, and not to have an exception regarding returning ElementHandles. Of course you could still call .elementHandle() or .elementHandles() on your underlying Locactor instance if you want these!

sebinsua avatar Jul 07 '22 19:07 sebinsua

@jrolfs Another question: I seem to be getting errors while debugging in VSC with the Playwright extension now, when I wasn't getting them before... It's the same error I got with the experimental "component" feature before dropping that:

const checkbox = await getByLabelText(/^Don't show this again$/);
await checkbox.setChecked(params.dontShowThisAgain);

Unknown engine "get-by-label-text" while parsing selector get-by-label-text=["__REGEXP /^Don't show this again$/"]

Oddly this doesn't cause any problems when running it - only debugging in VSC. Unfortunately that's going to discourage our team from using it if we can't debug without changing syntax... Any ideas why this is happening?

ScubaDaniel avatar Jul 07 '22 20:07 ScubaDaniel

@sebinsua are you suggesting we essentially "pass" the Promise returned from findBy* from the selector engine to the consumer of the Testing Library API? I kinda doubt it's as simple as that in implementation, but I'm just trying to understand the suggestion.

Edit: btw, I really like this idea as it gets us closer to 1:1 with Testing Library while still allowing us to (like you said)

continue to use Playwright's copious expect(locator: Locator) APIs.

jrolfs avatar Jul 07 '22 20:07 jrolfs

@ScubaDaniel as a workaround, have you tried awaiting an assertion like the second example from the Plawright Test docs here? Given the constraints of the selector engine API (no async/Promise, I took this to be the "idiomatic Playwright Test way" ... but I'm curious if a) it works and b) it sufficiently conveys the intent you were going for with findBy*.

jrolfs avatar Jul 07 '22 20:07 jrolfs

Just trying to find a way forward in the interim as I'm pretty tight on time at the moment trying to get an internal library release out. If we come up with something from @sebinsua's suggestion, though, I think I can find the time to give that a go.

jrolfs avatar Jul 07 '22 20:07 jrolfs

@ScubaDaniel ah, never mind my suggested workaround... I just tried it and I think it's irrelevant because I think it still expects the Locator to reference an element on the page, it just polls the assertion for said Locator.

jrolfs avatar Jul 08 '22 01:07 jrolfs

Oop, derp I was using getBy*, queryBy* works: https://github.com/testing-library/playwright-testing-library/pull/449/files#diff-22b6144c5d1a4e3a0345055b859237146c3f2112bf2ffa2e3d131e33ca1d42cfR150-R152

jrolfs avatar Jul 08 '22 06:07 jrolfs

are you suggesting we essentially "pass" the Promise returned from findBy* from the selector engine to the consumer of the Testing Library API?

Not quite. I think my suggestion was that we attempt to access the Locator's elementHandles()s method internally to find out whether it points towards anything and if so asynchronously return the Locator but otherwise throw an exception.

To be honest, I don't know whether it makes sense, apart from as a short-hand assertion?

It's kind of ironic as within normal testing-library the getBy* is synchronous but findBy* is lazy but here I'm suggesting that it be a less lazy version of getBy* as it could wait-and-assert and only then return a Promise<Locator>.

Maybe best to not do it unless this makes sense, as we could end up with something that doesn't make sense to people.

sebinsua avatar Jul 08 '22 21:07 sebinsua

@sebinsua yeah, I re-read your comment more closely after thinking about it for a bit and I think I'm on the same page now. It is really tricky to communicate though, I think on account of mixing these laziness paradigms. For that reason, just like you, I was thinking maybe we should continue to steer clear of findBy* in favor of @playwright/test's asynchronous assertions, but after trying it out I think there may still be a place for findBy*.

Here you can see a simple asynchronous assertion that waits for the deferred DOM change as suggested in the docs.

But, here you can see one that I skipped because this slightly more involved use case breaks down. In that specific case, it was because of Locator strictness, but it got me thinking...

Testing Library queries are themselves an assertion, but not always the entire assertion being made in a test. Hence the findBy* queries which all you to assert the existence of something (often correct role etc.) asynchronously which then is often followed by further interaction. Trying to accomplish this the Playwright way kinda breaks down:

await expect(queryByLabelText(/Send me teh spam/)).toBeVisible(); // <--- we've waited, but `expect` evaluates to `undefined`, also we have to use `query` here when we really do mean `get`

const locator = getByLabelText(/Send me teh spam/); // <--- now we're just repeating ourselves

expect(locator).toBeChecked();

Your suggestion would look like this ↯ — right?

//                    a little weird since all the other queries return a `Locator` synchronously because locators
//              ⇣⇣⇣⇣⇣ are lazy, but still consistent with how you would use, say, React Testing Library, right?
const locator = await findByLabelText(/Send me teh spam/); Library

expect(locator).toBeChecked();

I think it actually makes sense in the context of Testing Library and is probably what we'd want. We'll just have to handle the waitFor bits ourselves as you suggested. I'm hoping that .elementHandle() isn't strict like the Locator assertions are, so that findAllBy* works the same — and then we'd just need to wire it up to the configured timeout (either global via configuration or local via the options param).

@ScubaDaniel tldr; I do think we should implement findBy*, but I'm not sure when I'll be able to have something for you there. Hopefully you can use the await expect() example above as a workaround in the meantime? I will try and reproduce the Code extension issue as well.

jrolfs avatar Jul 08 '22 22:07 jrolfs

I think it actually makes sense in the context of Testing Library and is probably what we'd want.

Yes, that is what I meant. And, I agree that it is probably what we want because on a practical level this is how testing library "feels like" it works outside of Playwright (although the Playwright methods have generally been more lazy than this and have only been asserting something at the point of calling expect(...)).

If we're unsure about the API, you could expose it with an unstable_ prefix (like how React exposes its experimental methods).

sebinsua avatar Jul 11 '22 14:07 sebinsua

@jrolfs Sorry to bug you but how far away from an official release are you, and what version of the package contains all of these changes?

I need to write some E2E tests soon, and I'm trying to weigh up whether I produce a greater technical debt by using a beta package, or by continuing to build upon the custom selector engine I originally created -- if I continue to use the latter, I'll need to rewrite all of my tests once this is released.

sebinsua avatar Aug 12 '22 10:08 sebinsua

I need to write some E2E tests soon, and I'm trying to weigh up whether I produce a greater technical debt by using a beta package, or by continuing to build upon the custom selector engine I originally created -- if I continue to use the latter, I'll need to rewrite all of my tests once this is released.

Can you link to what you've got?

gajus avatar Aug 13 '22 18:08 gajus

I need to write some E2E tests soon, and I'm trying to weigh up whether I produce a greater technical debt by using a beta package, or by continuing to build upon the custom selector engine I originally created -- if I continue to use the latter, I'll need to rewrite all of my tests once this is released.

Can you link to what you've got?

I'm essentially using the implementation in this Gist but with the automatic fixtures update from @petetnt. Also, I had to change the names of some of the selectors, as a recent version of Playwright added their own 'role' selector.

What I don't yet have is a @testing-library style API on top of this (e.g. getByText, queryByLabelText, etc.)

sebinsua avatar Aug 19 '22 10:08 sebinsua

I was hoping to get the configure API and the findBy* stuff we discussed here in there first, but we just had our first 👶 about a week ago and I was pretty busy at work getting ready for my leave. I think I'll have some chunks of free time here and there during my leave, but I can't make any guarantees.

Do you think you could take a crack at the findBy* implementation @sebinsua? I'll try to finish up configure per my notes in the draft PR I opened a while back. I'll also pay more attention here and if you open a PR against beta I'll review it asap.

jrolfs avatar Aug 25 '22 02:08 jrolfs

I'd really like to get this released for the tests you're looking to write. Would you be interested in helping me maintain this stuff @sebinsua?

jrolfs avatar Aug 25 '22 02:08 jrolfs

Note that this library is mostly redundant now that Playwright added ByRole selectors.

https://playwright.dev/docs/selectors#role-selector

If helpful, this is how we implemented it:

export const test = base.extend<{ page: Page }>({
  page: async ({ page: playwrightPage }, use, testInfo) => {
    const revocable = Proxy.revocable(page, {
      get(target, property, receiver) {
        if (property === 'findByRole') {
          return (role: string, name: RegExp | string) => {
            return target.locator(
              `role=${role}[name=${
                typeof name === 'string' ? '"' + name + '"' : String(name)
              }]`
            );
          };
        }

        return Reflect.get(target, property, receiver);
      },
    });

    await use(revocable.proxy);

    revocable.revoke();
  },
});

and then use it like any other selector page.findByRole('button', 'Sign Up'), etc.

gajus avatar Aug 25 '22 19:08 gajus

Nice, thanks for the update and the snippet @gajus. I think there's still value to this library in order to provide Testing Library parity in Playwright, but I'll probably add something to the readme pointing out the role selector as an alternative.

I like to be pretty selective when considering Proxy, but this is a clever/convenient use case. Do you use TypeScript? Were you able to reliably augment/extend the Page type with your custom methods?

jrolfs avatar Aug 25 '22 21:08 jrolfs

I like to be pretty selective when considering Proxy, but this is a clever/convenient use case. Do you use TypeScript? Were you able to reliably augment/extend the Page type with your custom methods?

It is as simple as {findByRole ...} & Page

gajus avatar Aug 26 '22 03:08 gajus