testing-library / playwright-testing-library

🔍 Find elements in Playwright with queries from Testing Library
MIT License
249 stars 9 forks source link

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

Closed ScubaDaniel closed 2 years ago

ScubaDaniel commented 2 years ago

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.

jrolfs commented 2 years ago

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/test@4.3.0-beta.1

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@playwright.dev');

  // Assert via `Locator` APIs 🎉
  await expect(emailInputLocator).toHaveValue('email@playwright.dev');
})

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.

ScubaDaniel commented 2 years ago

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 = "doctorwho@tardis.net";
  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 |
jrolfs commented 2 years ago

@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.

ScubaDaniel commented 2 years ago

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!

ghost commented 2 years ago

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)
ScubaDaniel commented 2 years ago

(Removed comment as it was user error)

jrolfs commented 2 years ago

@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

ScubaDaniel commented 2 years ago

@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?");
ghost commented 2 years ago

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
    });
ScubaDaniel commented 2 years ago

@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?

jrolfs commented 2 years ago

@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.

ScubaDaniel commented 2 years ago

@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.

sebinsua commented 2 years ago

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!

ScubaDaniel commented 2 years ago

@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?

jrolfs commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

sebinsua commented 2 years ago

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.

jrolfs commented 2 years ago

@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.

sebinsua commented 2 years ago

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 commented 2 years ago

@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.

gajus commented 2 years ago

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?

sebinsua commented 2 years ago

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.)

jrolfs commented 2 years ago

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 commented 2 years ago

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?

gajus commented 2 years ago

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.

jrolfs commented 2 years ago

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?

gajus commented 2 years ago

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

jrolfs commented 2 years ago

Ah, that's right as page is passed via the fixture so no need to augment Playwright's type, derp.

jrolfs commented 2 years ago

Alright, I got the configuration sorted in #450. I'll take a look at the findBy* next.

sebinsua commented 2 years ago

Would you be interested in helping me maintain this stuff @sebinsua?

@jrolfs Sure. I would be interested. I just saw you released a beta, too, so thanks for that.

Are you working on top of the beta branch? If you don't get around to findBy* I'll take a look, but I'm less experienced with this codebase than you so would be slower. Can we just pass through to waitFor internally or do we need to do something more complicated? Within Playwright code, I sometimes do things like locator.waitFor() or await expect(locator).toBeVisible() or something like await expect.poll(() => locator.count()).not.toBe(0).

I wonder what would be more appropriate -- do testing library queries normally check that an element is within in the DOM or that it is visible? One benefit of the await expect(locator) approach is that we can use a custom assertion message, but perhaps using a waitFor is closer to the semantics of Testing Library (based on the description of FindBy combining queries with testing library's own waitFor).

Edit: I'm currently rewriting my tests to use the new beta package, so this will give me awareness of any problems and maybe an understanding on how to combine Playwright APIs into something findBy*-like.

jrolfs commented 2 years ago

Great, I updated you to "triage."

Yes, I'm working off of beta. I should be able to have something out in the next couple days for the findBy* stuff, but do let me know if anything comes out of your experience with the latest beta. So far it's looking like locator.waitFor() with the queryBy*'s should do the trick — I'm planning on still preserving the default 1000ms asyncUtilTimeout from @testing-library/dom which can be configured per 115f8fe811ae9124e457d8ac8d5b074dac8e0b9b. I was thinking it probably also makes sense to expose the state option as asyncUtilExpectedState or something like that.

I'll add you as a reviewer as soon as I have a PR up. Thanks!

sebinsua commented 2 years ago

Yes, from my experience today, I've also been getting it to work with queryBy*/queryAllBy* in combination with locator.waitFor().

One problematic thing I've found is that locator.waitFor() errors if the Locator has matched multiple elements. This causes a strict-mode violation error (see the strictness section in the docs).

I think this means that to implement findAllBy* you'll need to use a locator.first().waitFor() before returning the queryAllBy*. I can't say with absolute certainty, but I think that internally testing-library works the same way as their own waitFor function resolves when the inner callback returns/resolves instead of throwing exceptions. (This code is complicated...)

Btw, the normal testing-library seems to implement findBy* using getBy* however in our case this isn't possible as that immediately throws if it gets 0 elements and we don't have an idiomatic way of retrying. However, ideally we should throw the correct underlying error if 0 elements are found (particularly if Testing Library has provided a more informative error message with suggestions). By default, our findBy* method's waitFor will just time out. Within testing-library they essentially keep saving the error as lastError and then augment this error before rejecting with it on timeout.

I don't think it's possible for us to do exactly this, but I guess we could try-catch to ignore any timeout errors and then call getBy* on timeout errors to procure the correct error message (if any exists), but otherwise return the result of queryBy*. I'm unsure whether this has ramifications I've not considered...

Unrelated to this, but because we don't have a waitForElementToBeRemoved function, I've been implementing this in Playwright with the following pattern await expect(queryAllBy*(...)).toHaveCount(0). This works however I ran into a confusing problem in which I was using within(parentLocator).queryAllByLabelText('Label') and waiting for the count to reach 0 but it wasn't working as parentLocator had been created with a getBy* and therefore threw an error once there were 0 elements. I had to fix this by making sure that all parent locators are created with queryBy. (We don't need to fix this right now but this is an example of a semantic mismatch between Playwright and Testing Library that makes the DX confusing.)

I'm planning on still preserving the default 1000ms asyncUtilTimeout from @testing-library/dom which can be configured per https://github.com/testing-library/playwright-testing-library/commit/115f8fe811ae9124e457d8ac8d5b074dac8e0b9b. I was thinking it probably also makes sense to expose the state option as asyncUtilExpectedState or something like that.

Sound reasonable. Unless we just make it use the Playwright waitFor options? Do you know if there is a way to default it from Playwright's own timeout configuration values?

One other thing -- is there anyway that we could allow users to configure getElementError? I'm finding the length of the error messages incredibly long as it prints out a bunch of HTML and CSS and I have to scroll up a long way to find the actual error message.

jrolfs commented 2 years ago

One problematic thing I've found is that locator.waitFor() errors if the Locator has matched multiple elements. This causes a strict-mode violation error (see the strictness section in the docs).

Shit, I forgot about this. This kinda makes me want to try another avenue I was considering... essentially implementing the findBy* the same way that @testing-library/dom does with the waitFor from @testing-library/dom. The other reason I was considering this was to surface the Testing Library error instead of the Playwright locator.waitFor() timeout message (~I was also considering catching the timeout error and then running a final getBy* to produce the error~ lol, oops I totally missed you mentioned this exact thing).

Sound reasonable. Unless we just make it use the Playwright waitFor options? Do you know if there is a way to default it from Playwright's own timeout configuration values?

Hrm, so you did get me thinking that perhaps the Playwright defaults would make the most sense here, but unfortunately, I don't see any way of reading from the Playwright Test configuration in the documentation. Also, I do still want to keep supporting "vanilla" (sans @playwright/test) Playwright in some capacity going forward. I think I'll probably stick with an independent asyncUtilTimeout value.

Unrelated to this, but because we don't have a waitForElementToBeRemoved function, I've been implementing this in Playwright with the following pattern await expect(queryAllBy(...)).toHaveCount(0). This works however I ran into a confusing problem in which I was using within(parentLocator).queryAllByLabelText('Label') and waiting for the count to reach 0 but it wasn't working as parentLocator had been created with a getBy and therefore threw an error once there were 0 elements. I had to fix this by making sure that all parent locators are created with queryBy. (We don't need to fix this right now but this is an example of a semantic mismatch between Playwright and Testing Library that makes the DX confusing.)

Do you think implementing waitForElementToBeRemoved would sidestep this sufficiently? I'll have to sit with this a bit, but let me know if you think there's something you think we should change with the within() implementation/behavior.

One other thing -- is there anyway that we could allow users to configure getElementError? I'm finding the length of the error messages incredibly long as it prints out a bunch of HTML and CSS and I have to scroll up a long way to find the actual error message.

Okay, so this is something I've wanted to address since I created this library. I'd be curious if you have an idea of what a better default implementation/output would be. I do think we can add support for this option, but I really think we also need a better default because the e2e use case just always produces so much output. I also want to figure out how to preserve the ANSI formatting in the Testing Library errors.


Thank you so much for all of this feedback!

The main project I use this library on is still on the ElementHandle version (and jest-playwright). We do have one other project at my company that's using @playwright/test and the more modern version of this library, but it's just smoke tests and hasn't seen a lot of action in a bit.

jrolfs commented 2 years ago

Alright, findBy* queries are most of the way there in #488 if you wanna take a look. I stuck with Playwright's waitFor and got our getBy* error idea working — I think using Playwright's asynchronous utilities is ideal, if possible (as opposed to the waitFor from Testing Library).

sebinsua commented 2 years ago

I think using Playwright's asynchronous utilities is ideal, if possible (as opposed to the waitFor from Testing Library).

Strongly agree. In general, I think that over time we should look to rely on Playwright's implementation more. They presumably perform better than Testing Library implementations and also are likely to be more correct if they are able to rely on the browser for things which Testing Library has to achieve in longer-winded ways (e.g. dom-accessibility-api).

Do you think implementing waitForElementToBeRemoved would sidestep this sufficiently?

Let's not do this yet. For now await expect(queryAllBy*(...)).toHaveCount(0) works and you just have to keep in mind that you can't base an expectation that the element count will be 0 off a parent locator that is a getBy* or findBy*.

Let me know if you think there's something you think we should change with the within() implementation/behavior.

I wonder if there is a way to throw an error if attempting to do a queryBy* on top of a getBy* or findBy*. I guess we could do something like this, however it would make the implementation more complex. I would ignore the potential problem for now as we don't have full certainty over what makes sense in practice (and it seems that nerfing within to disallow basing a queryBy* off a findBy* would be annoying in the majority of cases).

I'd be curious if you have an idea of what a better default implementation/output would be. I do think we can add support for this option, but I really think we also need a better default because the e2e use case just always produces so much output.

I agree that we might want to change the default getElementError and have a few suggestions.

  1. It outputs huge amounts of inline CSS/HTML in our case. This seems to be because of pretty-dom which sets a length limit of 7000 by default (!!!). My recommendation would be to inline getElementError but without the postfixed prettifiedDOM (and with a comment linking back to their original implementation). Actually, for now I can set DEBUG_PRINT_LIMIT=0 to get the same behaviour...!
  2. I've also found the logic which prints out roles found on the screen is controlled by a hidden config._disableExpensiveErrorDiagnostics option (which they switch on/off during waitFor). I wonder if we could have this be switched on/off via an environment variable? I don't currently know whether it should default to true or false as although it buries the error message it's incredibly helpful!
gajus commented 2 years ago

One other thing to consider: If you are not using Playwright's native APIs, then all these look ups are not going to be surfaced in Playwright traces.

jrolfs commented 2 years ago

@gajus just tried it out and this is what I'm seeing:

trace

It looks good to me...? Let me know if I'm missing something.

gajus commented 2 years ago

I may have misspoke! I thought it won't, but it looks like it does.

jrolfs commented 2 years ago

In case y'all missed it, I published the findBy*/findAllBy* stuff on v4.4.0-alpha.1 a few days ago — I finally sorted the Semantic Release stuff, so it's also out on v4.4.0-beta.3. I'll probably delete the alpha release to prevent confusion.

@ScubaDaniel I know we co-opted your issue with all of this discussion, so I apologize for all the noise. If you're still interested, you can try out the find queries with the aforementioned beta release.

@sebinsua let me know when you get a chance to try this stuff out. I'll probably get the screen idea implemented as the last feature on the beta release channel. Unless you have additional feedback, I just have one last little thing I want to address before releasing this stuff officially on 4.x (I want to improve the error message for the findBy case that fails when an element is hidden. Right now the hard-coded 100ms timeout error will be confusing).

After that, I'm planning on putting together a 5.0 release that consolidates both the @playwright/test and playwright use cases onto the new Locator stuff. All of the different APIs are pretty confusing right now, and as a bonus it will clean up the code considerably. I'll probably throw together a quick codemod for preserving ElementHandle behavior via locator.elementHandle() to ease migration.

sebinsua commented 2 years ago

@jrolfs Strangely the alpha seemed to be published under the package name playwright-testing-library only. Any ideas why? I'll switch to the beta now anyway.

I'm using it. It's working great and I don't have any additional feedback right now.

Also, I'm happy to help out with PRs and things if you'd like that.

jrolfs commented 2 years ago

@sebinsua bleh, a dependency upgrade busted the second publish step, but I thought it went through on alpha. I manually published the Plawright Test package for beta.3, but I've now reverted the offending upgrade so it should be good going forward.

InduKrish commented 2 years ago

hi,I've been trying to use playwright testing library in the test, and realized that the playwright testing library is using element handles which is discourage to use by Playwright official team, based on this thread, looks like element handle is replaced with the locator api. Can you please confirm if i understood correctly?

and Can you please advise when the locator api with the playwright testing library will be officially released?

I have also observed few issues. Can you please clarify?

test("Default page verification", async ({ defaultPage, loginPage, page, queries: {getByTestId} }) => {

               //regular playwright code to login, you can ignore
                await page.goto("https://some website.com");
                await page.locator('input#username').click()
                await page.locator('input#username').fill('xxxx');
                await page.locator('input#password').click()
                await page.locator('input#password').fill('xxxx);
                await Promise.all([
                    page.waitForNavigation(/*{ url: 'xxxx' }*/),
                    await page.locator('text=Submit').click()
                ]);
               //regular playwright code to login, you can ignore
                await page.locator('text=Vacation').first().click();
                await page.locator('text=Settings & Defaults').click();

                // getByTestId works only when I add waitForSelector to wait for that specific locator.
                await page.waitForSelector('[data-testid="Edit Group Codes"]');
                const $form = await getByTestId('Edit Group Codes');
                await $form.click();

Question 1: if i execute const $form = await getByTestId('Edit Group Codes'); without waitForSelector, the test fails with the following error , as the DOM is not completely loaded. wondering why getByTestId() doesnt automatically wait like how the playwright team makes playwright auto wait before performing click action? wondering if there is any other way other than adding waitForSelector to auto wait until DOM loads.

elementHandle.evaluateHandle: TestingLibraryElementError: Unable to find an element by: [data-testid="Edit Group Codes"]

Question 2: const $form = await getByTestId('Edit Group Codes'); await $form.click(); why these lines can't be executed in a single line like --> await page.locator('text=Submit').click() it gives the following error: TypeError: getByTestId(...).click is not a function wondering why it has to be assigned to the variable to perform click action, as it increases no of lines in the test and hard to maintain?

jrolfs commented 2 years ago

Alright y'all, this is (finally) officially released in 4.4.0. I also updated the readme with documentation for the new Locator stuff.

There's still more to come as we work through chaining support and some other potential niceties in #501, but I don't foresee those improvements to be breaking.