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
63.77k stars 3.45k forks source link

[Feature] Add getById function #19763

Open gselsidi opened 1 year ago

gselsidi commented 1 year ago

Web standards are still using id's instead of data-testid's, even if that weren't the case i don't believe a testing automation frameworks role is to advocate for change, but instead it should focus on supporting developers in their day to day and provide support for the most used standards and methods, which currently is id.

ZooDoo4U commented 1 year ago

Agreed, honestly can't understand why there is no getById()? Element id are per the HTML Spec... Where is the standards and peer review for data-test-id, or is it data-testID, or datatest-id, or what ever alternative form exists...

ID is clear and clean, and have existed i might be before those pushing for data-testId existed and have for my needs 99.999% of the properties i want.

For convenience here is a link to the HTML spec... https://www.w3.org/TR/2011/WD-html5-20110525/elements.html#the-id-attribute

I don't tend to work on new code bases but legacy and most of the code has ID, before data-testid ever existed. Nor from the life of the apps will the old legacy code be updated to have data-testId, nope gotta use ID...

Would assert IIRC, locators were made 'strict' around 1.13 or so, and would fit per the requirements of an ID for an HTML element...

Would love to hear the reasoning other than it seems the people who did Test Library, who helped influence PW to add all the getByRole() and other getByXXX() is a bit opinionated and more from a subjective perspective at best. No regards to how others my have decided to utilize and maintain things... No concern or regards for legacy apps, but those stuck maintaining and supporting legacy stuff should have a more difficult time.... why?

MindaugasMateika commented 1 year ago

@ZooDoo4U you can set getByTestId to use ids, see https://playwright.dev/docs/api/class-selectors#selectors-set-test-id-attribute. Or you can still use locator syntax like page.locator('#your-id-attribute') so I don't really see your point about making things difficult.

ShaharM7 commented 1 year ago

@MindaugasMateika Excellent idea and also, playwright act like cypress with selectors element and they used only CSS, and you have to find just the id by: "#yourIdseletor"

ZooDoo4U commented 1 year ago

@MindaugasMateika Don't see why it isn't more direct and obvious to use getById() and all will be happy. Guess i'm not a fan of hidden remapping if one calls 'x' i expect it to be 'x' not 'y'. So will it be allows to cay getByRole( "input"...) but that actually gets a

? My point id != data-test-id, or is it test-data-id or testId or data-id or ??? I know what id is and it's implied semantics. Sorry it makes it more difficult because if one works on more than one code base you have to now worry did someone change or stuff break because this project changed the normal behavior an re-map id to something else or did someone accidentally changed it. Id is simple, clear, and not redirected to mean something else than what it is.

What about needing to deal with legacy code bases, most old code i've needed to test has an id property. So let us just use getById()?

Not sure why it is so hard to have getById and be done with it? And ID and test-data-id have different semantics.

Can you show any other common library that when you call 'x' you are actually calling 'y' because it is remapped or altered? Reminds me in my early days doing Cobol, it had an ALTER statement, seems most Cobol implementations still allow it to be used. Think everyone has been fighting to remove it. And anyone even attempting to use one, unless it was in the code prior to your work, can practically guarantee in a code review you'll get comment to remove it in order to check in...

Not sure what the down side is to adding a getById()?

MindaugasMateika commented 1 year ago

@ZooDoo4U there's no downside. If the issue gets enough upvotes then most likely it will be added some time in the future. It wasn't added at first since inspiration for these methods was a testing library.

ZooDoo4U commented 1 year ago

@MindaugasMateika My main point, what other library remaps when calling 'x' you aren't actually getting 'x'.

MindaugasMateika commented 1 year ago

@ZooDoo4U What do you mean by remap? You call .getByTestId('whatever') it searches for data-testid=whatever. If you need to search by something else, you can customize it. Like, customize pretty much everything else in playwright. It doesn't remap anything by itself.

ZooDoo4U commented 1 year ago

Say the stuff i'm automating has no date-test-id when i know it has "ID"... I would never expect to getByRole("div", {options...}) and get a label element would we? Might see room where this might be valid actually...

Remap? Yes

if( string.compare("id","data-test-id") == true) { remapped }

Been there, done that, it shouldn't be the default, opt in if people want... Just don't like someone's cute little "I'll help you out" only to find out more often it let bugs through.

So why not let people who want a strict match use getById() nothing stops one from using an unconventional getByTestID to use data-test-di, which may get id, or any other variant it allows...

Still what library have you used where string.compare("id" ,"data-test-id") are equal?

gselsidi commented 1 year ago

@ZooDoo4U What do you mean by remap? You call .getByTestId('whatever') it searches for data-testid=whatever. If you need to search by something else, you can customize it. Like, customize pretty much everything else in playwright. It doesn't remap anything by itself.

He means when you override it in settings and make getByTestID search for ids or cypress-test or the different forms of data-test.

It’s basically a catch all. Just stop trying to defend an obviously poor implementation it looks ridiculous honestly lol.

We decided to replace the most used selector in the world ID by something super obscure like data-test and all it’s versions, and dropped complete support for automatically locating by ids.

Even when codgen falls back to .locator it ignores id’s. It’s literally bizarro world, like someone purposely went out of their way to make codegen ignore all regular ids.

It’s like forcing people to use some obscure way of doing things for the lulz.

Work arounds are not answers to faulty implementation.

Codegen is a tool for people to learn, learn what exactly bad practices? How to erase the most used selector in history from existence….

Not that it matters because of the obvious workarounds it’s just literally utter insanity defending such lol

gselsidi commented 1 year ago

Don’t understand the issue here, you can have getbyTestId and getByID live side by side.

It’s like you’re offending someone’s love child lol.

ZooDoo4U commented 1 year ago

Why can't both exists. People are free to opt in by the api called? If they want the fix up call getByTestId. If one wants a strict match getById() and all are happy, works, it's clean and one knows the behavior... Not sure why it is so difficult to allow for both and each has it's own semantics. Unless it is more a my way or no way... Would assert opinions should not exist in code bases...?

Is it really that hard to allow for both?

MindaugasMateika commented 1 year ago

No one is saying it's not allowed. As I agreed with you earlier - there's no downside to having both. If the issue gets enough upvotes, perhaps it will be implemented in the future. You can also contribute yourself if you want, it's open source. Let's not clutter this issue anymore.

ZooDoo4U commented 1 year ago

It is not allowed by the omission from the API. Selenium has By.Id? Most other test libraries also have by "ID". Sorry don't have time to setup a dev environment, should be a fairly easy wrapper for one with a PW dev environment to add. Just speaks more about supporting customer needs. Huge difference from when i use to work in Redmond myself.

LongLiveY96 commented 1 year ago

In the actual projects I came across, there are often irregular ids, such as #layer_123, such ids are defined by some kind of index when defined by the front-end, and do not have stability or any business meaning, if codegen locates something like this using the id, it will inevitably modify this selector twice, which ultimately affects the overall development efficiency I would prefer to be able to customize the priority of the selector generation rules, such as getByRole as the highest priority, followed by getByText, rather than adding the getById function.

sand4rt commented 1 year ago

Just sharing my thoughts on why i like the new locators:

  1. Using ID's and (mainly) classes both for testing and development is fragile because they have two different reasons to change. Let's say i want to change the style of an element by changing its ID or class. I don't always want tests to break by default when that happens.

  2. The downside to ID's is that they can only be used once within a page, which discouraged code reuse. Many UI's today are built from reusable components.

  3. The new locator encourages applications to be more accessible because it queries on accessibility handles. In the EU there are even laws coming for that (European Accessibility Act).

  4. The more your tests resemble the way your software is used, the more confidence they can give you. The new locators allows to get the tests closer to using the software the way a user will. This allows the tests to give you more confidence that your application will work when a real user uses it. In other words, search the DOM for nodes in a way similar to how the user finds elements on the page.

It's not always possible to query by accessibility handles. In that case you might think about using getByTestId() and if you have no control over the DOM, locator('.class #id') could be still used (there is still a by ID and class in there).

Testing Library has proven that new accessibility locators are successful for quite a few years (even with no id and class selectors at all). This is well known and respected in the frontend community, if you come from another background, you probably need some time to get used to it i guess..

gselsidi commented 1 year ago

Id’s rarely change if ever so no idea why you think a locator that never changes is fragile.

Unless you have a very static app, those locators are semi useless sometimes, like locating a drop-down by role and name.

Since name changes depending on the value of the drop-down that locator method is actually useless.

I told my developers about hey let’s use test-data ids because this framework supports them the most they all laughed and looked at me weird.

ID’s are best practice, common knowledge, and the most supported locator in history of development and testing for the reason that they rarely change.

ID’s are the least flakey method of selecting something, all the other methods are flakier, yet somehow the argument of more flakey selectors is better lol.

You can try to argue all you want until you’re blue in the face but it won’t change the fact that you’re arguing against long established practices and web standards.

sand4rt commented 1 year ago

I'm not trying to argue here. Just sharing my thoughts about the new locators:)

gselsidi commented 1 year ago

I like them too especially you can filter ect chaining them makes it easier then having to pass in a variable inside a locator.

But not having ids from an implementation standpoint is broken from a technical API level. Even overriding getbydataid it’s not a good implementation.

The function name is supposed to tell you what it’s doing it’s get by dataID but it could really be getby x y z because you can override it with whatever you want.

At least change its naming to just getByAttribute (defaults to data test), would be a better easier naming convention and what the function is actually doing.

But I found codegen sped up my authoring of tests as I could click on all the elements I needed to interact with in the test and have the locators ready 95% of the time.

Now i don’t even bother using it anymore and just manually one by one create the locators.

ZooDoo4U commented 1 year ago

The issue for me is letting people work in the style and manor for the environment they find themselves having to work in. Honestly doing developer support at a little company that does windows and other programs located in Redmond, the new buildings look interesting. At times came across code i'm saying to myself (man this is really stupid) but having to work through their code to see if their code has a bug or is it a product bug... Oh i see, guess they are forced doing it the way they did...

Think there is a bit of this going on. I straddle between test and dev work, have seen some of the javascript librarys angular, react, or others depending on the template code written, as they will do virtual dom updates and need performance/deterministic code to update, id's may be auto generated.. If the dev put in no id's then ya you might get a GUID/uuid for an id... In this case ya using ID for automation would completely argue against it. But wait, the problem is more that the devs couldn't be bothered and were "efficient with their time", or lazy? While the javascript libraries in this case do give warnings for code at times, "element should have id"...

But from experience, if you have a deterministic and know what it is, that is i'd argue the best. Even helping a person in dicord yesterday his code was messing up move because they were new, not understanding locators and were getting multiple elements the getByXXX() was finding. Sure there are ways to mitigate it, but would assert id's are and will be because of he semantics associated per the HTML spec, more durable and reliable. Something has no ID, that is more a bug on the dev code. For us that are explicit and do the work to get unique IDs on elements, we shouldn't be penalized for lazy development practices of others...

pavelfeldman commented 1 year ago

Can you use locator('#id') instead?

sanamhub commented 1 year ago

GetById / GetByElementId and GetByClassName addition could be better rather than using locator (more readable and useful for old codebase test)

sand4rt commented 1 year ago

It is also possible to use custom locators https://playwright.dev/docs/test-fixtures#overriding-fixtures:

import { test as base } from '@playwright/test';

const test = base.extend({
  page: async ({ page }, use) => {
    await use(Object.assign(page, {
      getById: (id) => page.locator(`#${id}`),
      getByClass: (className) => page.locator(`.${className}`)
    }));
  },
});

test('custom locator', async ({ page }) => {
  await page.getById('your-id');
  await page.getByClass('your-class');
});
ZooDoo4U commented 1 year ago

Interesting one more questions for the PW people. If one goes to the https://playwright.dev/docs Why are there no data-testid entries on the page, all "id"... Why? If one says the "testid" are only in debug builds, okay so now i need to fix up tests to work n dev vs prod. Could see i may need to update the URL to test "Dev" vs "Prod" but now i need to make code changes to test.... Not really desirable...

arukiidou commented 1 year ago

from: https://github.com/microsoft/playwright/pull/20140#issuecomment-1400722861

So the outcome is that unless with have 50 upvotes for a dedicated method, we should keep recommending users to use locator('#id').

Everyone, please UP-VOTE! We need 37 more votes.

.getById we can begin to learn.

ZooDoo4U commented 1 year ago

Darn already voted, can we get team BranDumb to help steal the election?

Seriously not sure why we need to vote for this. If one ever needs to test production code for any reason, there are tools to remove data-testid, and needing to change the config file to use id instead is something too easily missed...

Of course i'm referring to this:

https://github.com/testing-library/react-testing-library/issues/479

So very easy without an id, might get code that simply can't be tested...?

bplus commented 12 months ago

Wow, when I went looking for why the vscode test recorder was using labels instead of ids, I didn’t expect to see all this. Very surprised at the number of people advocating against what seems like a very normal request.

ITenthusiasm commented 10 months ago

Also tossing in my thoughts here, and bringing some clarity to why there are people who would oppose getById.

I agree with @pavelfeldman that developers desiring to get an element by id can just use locator("#id"). The method is very convenient and affords little to no barriers. Someone advocating for getById("id") could just as easily advocate for getByClassName("class"), getByAttribute("attribute"), and the like. The list can go on and on. But all of these are redundant and unnecessary when we already have locator("#id"). From a maintainability perspective, it seems wisest to stick to locator(selector) for all such needs.


But the maintainability of this Open Source project actually isn't the main reason why I would prefer not to have a getById method. My reason for advocating against getById would actually be the maintainability of other projects using Playwright.

(TLDR; Keeping getById out of the the Playwright API gives developers one extra safeguard against writing tests that encourage inaccessible applications.)

Some Background

When the Testing Library Tools came around, the approach that they advocated for was very different from what typical Enzyme users were familiar with. The Testing Library tools made it very difficult to test the internals of your components. Originally, I found this bothersome. But later, I found this quite helpful.

The Testing Library tools moved me away from "Function gets called (or State updates) when I do X" towards "UI Looks like Y when user does X". (If we're being honest, this is what we're truly trying to test at the end of the day.) This approach made my tests more reliable (because they were properly verifying what happens when a user interacts with my components/app), and it freed up my components/app up for refactors. Tests didn't break when I renamed a function, changed internal logic, or the like. At this point, I came to like Testing Library a lot more.

But there's another benefit: The Testing Library Family encourages you to write accessible applications, and Kent C. Dodds has given several tips on how to go about using the tool effectively. This is huge to me, because it helps us as developers guarantee that our applications are genuinely accessible to all kinds of users. It also implicitly helps us shy away from using divs for everything, which oftentimes is a hindrance for Screen Reader Users -- and sometimes Keyboard Users. When the Testing Library tools are used correctly, you can structure your tests to be functional even when you overhaul your underlying markup (including HTML attributes), and your tests will still prove that your application is accessible to all users.

For clarity, the data-testid attribute is intended to be something that's used as sparsely as possible (at least as far as the Testing Library Family is concerned). It's effectively the Testing Library team conceding, "We can't argue that there's never a use case where a dev would need to get something by ID. So we'll give devs getByTestId." But the attribute isn't intended to be used as a primary way to help with testing. Similarly, the id attribute isn't intended to be used for testing in the Testing Library Family either -- at all. And Playwright has adopted many things from the Testing Library Tools.

The emerging standard for testing doesn't seem to be getById anymore, but rather getByRole, because it allows for finding accessibly labeled elements by their accessible roles. A11y appears to be becoming a greater concern for more applications; so I anticipate that this standard will only be strengthened over time.

Okay... So What?

Being able to write tests that are more robust and more accessible is great... But what does that have to do with the maintainability and reliability of other projects? In my opinion... a lot.

I have worked for multiple large companies. And repeatedly, whenever a testing tool like Testing Library or Playwright gets introduced, developers start reaching for getByTestId and locator automatically. That isn't so surprising since this is the simplest and easiest way to go about writing a test. However, it results in tests that aren't very valuable because they don't prove compliance with accessibility standards. Whenever business requirements come in to finally make the application accessible, the tests (and oftentimes the related component) inevitably have to be rewritten.

This problem can be avoided if the tests are written with accessibility in mind from the start. And testing tools can help with this by encouraging developers to write reliable tests and discouraging developers from writing unreliable tests. One way this can be done is with documentation and linting rules; another way is by restricting the exposed API.

But Should Testing Frameworks Impose Their Philosophies?

Arguably... Yes... (Someone could certainly argue the other way, though. :sweat_smile:) It's actually very beneficial for developers as a whole if the things that testing frameworks support or champion ultimately result in more accessible applications and tests that are less fragile. Tools that help us do the right thing are always preferred to tools that let us do whatever we want without guidance (or even worse, tools that make developers do the wrong thing). And it's fair (and common) for languages, frameworks, and other tools to strive for the former.

The page.locator method still allows developers to get an element by its id without encouraging it (or even being a large inconvenience). To that extent, I'd personally like the API not to support getById. The less means there are to write tests that aren't accessible, the better.


Of course, all of this is just my own view. People are free to disagree. But I hope this at least makes it more understandable why some developers don't want a getById method to be supported.

HendrikJan commented 7 months ago

It seems that page.locator('#myid').click() doesn't always work (doesn't consistantly retry).
Could that be caused by this issue? Would page.getById('myid') make these tests less flaky?
In that case I would certainly want to have a getById() function.

In our case, we use Vuetify.
In Vuetify, if you add id="someid" to an input-component will set this id on the actual input element inside the component.
This doesn't work for data-testid, and so data-testid is not a solution here.

ITenthusiasm commented 7 months ago

@HendrikJan My assumption would be that getById() would probably be a shortcut for something like locator("#myid"). So getById() probably wouldn't solve that issue.

More than likely, there is a bug in playwright or an even sneakier bug somewhere in your app or in Vuetify. For instance, there might be a bug in how/when ids are applied to elements. I've run into some tricky "Playwright problems" that were really just problems with how/when I was rendering stuff to the DOM. Other times, my understanding of how locator() worked as flawed (because I'm used to Cypress). The more UI abstractions there are, the harder debugging gets.

But the ID query selector (#my-id) is pretty simple and doesn't require complex logic. So my hunch is that it's a bug in Vuetify, the app, or in how Playwright is being used in the codebase -- but not in Playwright itself. Sometimes those are hard to track down. (I've had a really annoying time with some of my bugs.) But hopefully it's more discoverable in your case.

IMO, it would be easier/better to use something like getByRole or getByLabel. (If your inputs don't have accessible labels, there isn't much of a benefit to testing them.) If those don't work, it's almost certainly a bug somewhere in the app or in how Playwright is getting used. (I'd be surprised if Vuetify's components weren't built with a11y support.)

gselsidi commented 7 months ago

Guys please upvote we need 20 more upvotes a total of 50 for some variation of getbyid method to be implemented, and put this dark history behind us lol.

forward this to co-workers and have them up vote so we hit the arbitrarily number limit

arukiidou commented 4 months ago
arukiidou commented 1 month ago

Here is a sufficient number of 👍 . Is this mergeable ? @pavelfeldman

sanamhub commented 1 month ago

So the outcome is that unless with have 50 upvotes for a dedicated method, we should keep recommending users to use locator('#id').

Here's 50 upvotes as you say @pavelfeldman

Also, I don't understand why the team was not considering adding it having such a large voice from community already!

gselsidi commented 1 month ago

So the outcome is that unless with have 50 upvotes for a dedicated method, we should keep recommending users to use locator('#id').

Here's 50 upvotes as you say @pavelfeldman

Also, I don't understand why the team was not considering adding it having such a large voice from community already!

No need to understand besides they were hellbent on not doing it with frivolous replies that made absolutely no sense. Most likely someone’s ego got in the way and they just kept doubling down with non sensical rebuttals.

Then they picked some arbitrary “high” number out of their a$$ thinking it would never get required upvotes.

let’s see what they do now and if they honor their arbitrary special rules

but this is all conspiracy theory 😉

ZooDoo4U commented 1 month ago

Might assert at this point might be nice to be clear.... Meaning when the request was made the getByXXX had only been introduced. I would assert now there should be two methods added:

getById getByDataTestId <or> getById( "[ID|testId]", "idValue")

Kind of in the style of getByRole... In JS you get intelisense for the control type [button|input|...]

Some people actually adhere to the HTML spec where ID is suppose to be unique and it works perfectly fine, doesn't make sense to have getById and need to worry about some overridden indirection that data-test-id was actually implied, if one want data-test-id then use the correct function or the id...