Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Support chaining queries #501

Merged
merged 4 commits into from
Sep 28, 2022
Merged

✨ Support chaining queries #501

merged 4 commits into from
Sep 28, 2022

Conversation

jrolfs
Copy link
Member

@jrolfs jrolfs commented Sep 11, 2022

Implement query chaining per response to @gajus comment #498 (comment).

I think this came out to be pretty cool/useful/powerful.

Synchronous

test('chaining synchronous queries', async ({screen}) => {
  const locator = screen.getByRole('figure').within().getByText('Some image')

  expect(await locator.textContent()).toEqual('Some image')
})

Synchronous + Asynchronous

test('chaining multiple asynchronous queries between synchronous queries', >
  const locator = await screen // <--- including any `find*` queries makes the whole chain asynchronous
    .getByTestId('modal-container') // Get "modal container" (sync)
    .within()
    .findByRole('dialog') // Wait for modal to appear (async, until `asyncUtilTimeout`)
    .within()
    .findByRole('alert') // Wait for alert to appear in modal (async as well)
    .within()
    .getByRole('button', {name: 'Close'}) // Find close button in alert (sync)

  expect(await locator.textContent()).toEqual('Close')
})

Todo

  • Pass configuration through the chain
  • Test for chained asynchronous queries

@jrolfs jrolfs changed the base branch from main to beta September 11, 2022 07:13
@jrolfs jrolfs requested review from gajus and sebinsua September 11, 2022 07:19
@jrolfs jrolfs force-pushed the feature/chaining branch 2 times, most recently from 3eefc90 to d188bb7 Compare September 11, 2022 07:35
@jrolfs jrolfs force-pushed the feature/chaining branch 3 times, most recently from b273764 to e0ce6d3 Compare September 14, 2022 08:20
@jrolfs jrolfs marked this pull request as ready for review September 14, 2022 08:20
@sebinsua
Copy link
Collaborator

sebinsua commented Sep 15, 2022

Sorry, I've taken a while to get to reviewing these. I like how you've implemented the LocatorPromise concept!

I have a couple of ideas that I'll run past you regarding the API.


1.

Do we need the .within() API, or could we chain without it? Although, a user probably doesn't chain as often as you do in the examples it can seem redundant/noisey. If we didn't need it at all, we'd be less consistent with Testing Library, but be providing a simpler API with one less API. If Screen is a Page & LocatorQueries, could we make all methods that used to produce Locator produce WithinReturn<Locator> and redefine WithinReturn to create a Locator augmented with LocatorQueries?

I guess the issue I see with this is that Locator has quite a few methods like .first(), .last(), .locator() and .page() which can produce non-augmented Locator or Page instances. If we started to manually augment each of these, where does it end? We might end up playing whack-a-mole every time Playwright adds a new method trying to consistently augment each of them...

OK, I've convinced myself that being explicit and having a .within() method is a good idea for now at least... I think my suggestion has the potential to be a good idea but an imperfect implementation or hastily chosen API is dangerous so we would need to tread carefully (unless we have a way of implementing it which is highly maintainable).

Do other's agree/disagree?


2.

I've changed some of my code to use .findByRole(...) recently and something I've noticed which is quite annoying is that it's quite jarring that it returns Promise<Locator>.

In practice, you sometimes end up writing code like this:

await screen.findByRole('button', { name: 'Apply' });
await screen.getByRole('button', { name: 'Apply' }).click();

// -- or --

const applyButton = await screen.findByRole('button', { name: 'Apply' });
await applyButton.click();

But wishing that you could continue to chain Locator methods onto the Promise<Locator>, like so:

await screen.findByRole('button', { name: 'Apply' }).click();

Could the concept of LocatorPromise providing a .within() method be extended to support calling other Locator methods directly upon the LocatorPromise or would this cause problems?

@jrolfs
Copy link
Member Author

jrolfs commented Sep 16, 2022

@sebinsua heh, once again, your questions/propositions follow my internal dialog/thought process pretty closely...

1.

Supporting chaining without within() would be possible, but I stuck with within() both for Testing Library consistency and for reasons similar to the concerns you shared. Adding a single .within() to the Locator API felt more... Locator-y and disambiguates the query methods from the rest of the Locator methods.

I think my suggestion has the potential to be a good idea but an imperfect implementation or hastily, chosen API is dangerous so we would need to tread carefully (unless, we have a way of implementing it which is highly maintainable)

I might try to see if I can come up with something in TypeScript that would provide exhaustive checking as to whether we've "augmented" every method that returns a Locator... that might get us pretty close to "a way of implementing it which is highly maintainable," in that compilation would fail until we handled any new Locator methods in a future Playwright release.

2.

This also occurred to me, but the tricky bit here is that not all of the methods on Locator return a Promise. How do we feel about the trade-off of making the handful of synchronous methods on Locator (locator(), first(), etc.) asynchronous in order to preserve laziness (preventing the need for first await)?

I guess I could probably use the same technique I did with within() to "propagate" the Promise "down the chain," but just like the within() thing above, it might get pretty gnarly in practice. It will probably also add additional cognitive overhead and nuance to the Playwright APIs. With within() we are at least isolating this concept to query chaining.

Edit: another point for keeping Promise<Locator> that just occurred to me is that I can see find* queries often used to wait for a particular piece of the page to be ready before continuing the rest of a test. If we don't require a user to await at that point in time, then when that waiting happens is less clear. Maybe allowing both is more powerful, but I see it as another potential point of confusion as well.


Let me know what you think about the next steps given my responses above. Do you think we should get this released on beta more or less as is so that we can iterate, or do you think it's worth seeing through these ideas?

@gajus — please do let us know if you have any thoughts here as well :D

@sebinsua
Copy link
Collaborator

sebinsua commented Sep 16, 2022

Regarding 2., I didn't mean that the findBy* queries wouldn't return a Promise, I was thinking that as well as within there would be methods that chained all other public methods of Locator, effectively meaning that each would return a Promise. So, you would still be able to await screen.findByRole('button', { name: 'Apply' }) but could also await screen.findByRole('button', { name: 'Apply' }).click().

I don't really understand the implementation of this well enough to comment now, but presumably in some way click would .then the LocatorPromise before calling .click() against the Locator. I'd expect the synchronous methods to become asynchronous in this case, however, if we had awaited the findBy* on its own, I'd expect the methods to be untouched on the Locator returned as expected.

@jrolfs
Copy link
Member Author

jrolfs commented Sep 19, 2022

Ahaaaa, ok that's an interesting idea. It still requires find* to always be await-ed, but allows chaining a single interaction onto the LocatorPromise. I like it, and I don't think the implementation should be too bad. I'll give it a shot soon, thanks.

@markerikson
Copy link

Just wanted to say that I'm using Playwright Test Library for the first time today, and I would really like to have this :)

We've got a bunch of "page object"-style helper methods, like:

export async function togglePausePane(screen: Screen) {
  return screen.queryByRole("button", { name: "motion_photos_paused" }).click();
}

and right now we're passing around screen everywhere, which we got out of the {page, screen, within} arg to the test function.

We aren't passing within around, and I wasn't able to figure out a way to use the global within export properly. So, being able to do something like screen.queryByTestId("abcd").within().whatever() would be great!

@jrolfs
Copy link
Member Author

jrolfs commented Sep 28, 2022

@markerikson, that's a compelling reason to get this chaining stuff in there (as if we hadn't already convinced ourselves). I also have a project with quite a few of those sorts of helper functions that currently take a Page instance.

I apologize for the confusion with within() — the global export is currently reserved for the legacy ElementHandle query API. The next major release will consolidate around the new Locator query API and hopefully resolve the confusion associated with that legacy API.

I'm going to go ahead and merge this as is, @sebinsua. I think we can address the additional ergonomics around LocatorPromise can as a separate feature/pull request:

But wishing that you could continue to chain Locator methods onto the Promise, like so:

await screen.findByRole('button', { name: 'Apply' }).click();

Could the concept of LocatorPromise providing a .within() method be extended to support calling other Locator methods directly upon the LocatorPromise or would this cause problems?

Rest assured, I still plan on implementing this and adding more inline documentation (and a bit of refactoring) to ensure everything stays maintainable :)

Edit: oh yeah, I also wanted to mention that you can rest assured that the Locator API will stay pretty stable into 5.0. The only thing I currently plan on changing is the name of the locatorFixture export which should be a single-line change.

// Synchronous
```ts
test('chaining synchronous queries', async ({screen}) => {
  const locator = screen.getByRole('figure').within().getByText('Some image')

  expect(await locator.textContent()).toEqual('Some image')
})
```

// Synchronous + Asynchronous
```ts
test('chaining multiple asynchronous queries between synchronous queries', async ({screen}) => {
  const locator = await screen
    .getByTestId('modal-container')
    .within()
    .findByRole('dialog')
    .within()
    .findByRole('alert')
    .within()
    .getByRole('button', {name: 'Close'})

  expect(await locator.textContent()).toEqual('Close')
})
```
@jrolfs jrolfs merged commit f837a8e into main Sep 28, 2022
@jrolfs jrolfs deleted the feature/chaining branch September 28, 2022 08:22
@github-actions
Copy link

🎉 This PR is included in version 4.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markerikson
Copy link

Thanks!

It looks like this got published as part of playwright-testing-library, but not @playwright-testing-library/test . Could you bump that package as well?

(tbh I'm not sure I actually understand what the real difference is between those two packages.)

@jrolfs
Copy link
Member Author

jrolfs commented Sep 28, 2022

Ugh, lame, sorry about that. I'll have to see why that didn't fail the build.

The biggest difference is that one works with playwright the library and one works with @playwright/test the library and runner combined. Playwright has a build process with which they release many different package "flavors" from the same codebase instead of a more modular set of packages. We, therefore, need to do the same thing to some extent to support both playwright and @playwright/test.

@jrolfs
Copy link
Member Author

jrolfs commented Sep 28, 2022

@markerikson, alright, I manually published @playwright-testing-library/test@4.5.0. Sorry about that.

Thanks for the feedback on the confusion around the packages btw — honestly, I'd attribute some of that confusion to Playwright / Playwright Test, but I may try to clarify the difference in the readme further.

@markerikson
Copy link

@jrolfs thank you! as a fellow lib maintainer, I really appreciate the fast response :)

@jrolfs
Copy link
Member Author

jrolfs commented Sep 28, 2022

@markerikson sure thing, let me know if you run into any other issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants