Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Integration with text selector #13

Closed
thernstig opened this issue Apr 3, 2020 · 9 comments
Closed

Integration with text selector #13

thernstig opened this issue Apr 3, 2020 · 9 comments
Labels
enhancement New feature or request

Comments

@thernstig
Copy link

Nice to see expect-playwright 😃 Considering starting using it, but I could not find info about this and was curious about a few things.

Playwright supports text selector engines like so:

    const text = await page.waitFor('text=Unauthorized');
    expect(await text.evaluate((el) => el.innerText)).toBe('Unauthorized');

That I would have wished to replace with:

await expect(page).toHaveText('Unauthorized'); // using the underlying text selector
  1. The first thing I noticed is that the example at https://github.com/mxschmitt/expect-playwright#why-do-i-need-it does not show the above (nicer) way to get a text element that Playwright already supports.

  2. I also wrote another issue that text selectors should be case insensitive and "partial match" by default, which landed in another MR, see new description here https://github.com/microsoft/playwright/blob/master/docs/selectors.md#text

  3. The second thing I know is that an issue I wrote [Feature] Native shadow DOM support microsoft/playwright#1375 just landed its first merge request feat(text selector): pierce shadow roots microsoft/playwright#1619. The text selector now pierces open shadow roots.

NOTE Text engine searches for elements inside open shadow roots, but not inside closed shadow roots or iframes.

Should/are all these features taking into consideration with these new expect functions?

@mxschmitt
Copy link
Collaborator

Hey :)

actually you can do that already like shown here: playwright-community/playwright-jest-examples#1

regarding 1. true, I could definitely improve the examples section or refer to my other repo of the link above. (if you want to fire up a PR, feel free)

regarding 2. definitely something to consider, do you have API design ideas, how the methods should be called and what you would use most frequently?

Definitely my goal is to have all the helper expect statements reusable in there to provide them to the users either via Jest directly or without Jest.

Thank you!

@mxschmitt mxschmitt added the enhancement New feature or request label Apr 3, 2020
@thernstig
Copy link
Author

Hi Max,

All the above depends a bit on your internal implementations of the current functions.

  1. I.e. your example for toEqualText here https://github.com/mxschmitt/playwright-jest-examples/pull/1/files seems to indicate it is an exact match. Although https://github.com/mxschmitt/expect-playwright#toequaltext does not mention "exact" in the description, one could imagine this is the case from the wording "toEqual", but it would still be good to indicate in the README. Maybe saying exactly which underlying version of the text selector you use?

  2. Doesn't https://github.com/mxschmitt/expect-playwright#tohavetext use the text=Something version of the text selector, indicating case-insensitivity and substring match? If so you should maybe mention that for that already existing selector?

I'd also recommend tracking this to see if it lands microsoft/playwright#1658. if it lands, it would change the logic of your toEqualText selector.

@mxschmitt
Copy link
Collaborator

Hi :)

  1. I think we speak about two different things. toEqualText, does make a strict comparison with the value / exact match with it. But internally it just will use page.$(selector) to get the element from the DOM. So from my understanding you can use whatever selector with whatever custom selector engine in the end of Playwright.

  2. I'm doing the comparison of the text currently by myself (not in the browsers engine), so I don't use the text based selectors there. Also they are currently case sensitive, same for toHave (which is basically a .includes in the end).

These text selectors are definitely kinda confusing right now in Playwright in my mind which one's are case sensitive or which ones not. Also not sure if they can contain spaces e.g.

@thernstig
Copy link
Author

I think the internal implementation in the matchers here should use the text selectors by default, as they pierce the open shadow DOM now. It would make sense to kind of align toEqualText to use the exact matching of the text selector engine (i.e. text="something")and toHaveText to equal the case-insensitive, partial matching (i.e. text=something).

Aligning the nomenclature for the various matchers you add to this project with the Playwright nomenclature/wording for what they do might help? At least for me 😝

@thernstig
Copy link
Author

@mxschmitt I really want to start using this package, but I do have concerns between how the current expect functions are implemented for text selections. It'd be nice to completely align them and have 3 separate functions to alight to the exact playwright three different options for the text selector engine.

@mxschmitt
Copy link
Collaborator

@thernstig are you on the Playwright slack? Feel free to ping me there, so we can talk about that. :)

@thernstig
Copy link
Author

@mxschmitt Let me try to install this package next week and test some things, so I have better feedback.

@thernstig
Copy link
Author

@mxschmitt After lots of other issues at hand, I've come back to this. I think my deduction from all this is that maybe this should be closed. The reasons are that await expect(page).toHaveSelector('text=Playwright') works fine. Even though I'd of course have wished to use toHaveText if it was using the Playwright text based selectors. Reason being that it would be one less API to learn in essence, making it simpler for users.

So If I'd given one suggestion, it would have been to only keep toHaveText but indicate it uses the text selector engine and there are three options to use toHaveText("some text"), toHaveText('"some text'") (or toHaveText("'some text'") or toHaveText("/some text/i") to be consistent with the Playwright text selectors.

Or possibly to keep toHaveText and toEqualText and add toEqualTextRegex or something to match all Playwright text selector options.

Another thing is that this package seem to use textContent which means hidden elements will be included in the results, whereas I'd assume a user wants the visible text only. Which the Playwright text selectors give.

I'll leave this issue open for a while if you want to comment/keep it open and I am also still available to join slack to discuss more if you want!

@thernstig
Copy link
Author

@mxschmitt Closing this since it garnered no reply.

The conclusionis that we are using await expect(page).toHaveSelector('text=Playwright') for now to make it non-ambiuous to users to exactly what is being done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants