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

Already on GitHub? Sign in to your account

proof-of-concept for using __getattr__ in place of properties #178

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Collaborator

bobsilverberg commented Sep 3, 2012

This is a proof-of-concept for replacing page object properties which simply retrieve the text at a locator with getattr.

It bothers me that we need to add a property definition for each of these in every page object, when each property definition does the exact same thing. We can remove all of those property definitions by using the getattr method in this pull request.

I understand that the downside to this is that someone writing a test would not know which properties are available, except by looking at the code for the page object and finding the locators. Perhaps that is reason enough not to entertain using this method, but I thought it worth bringing up as it would certainly save a lot of code.

Please note that this pull request is not meant to be merged - it is just presented as an illustration of an idea.

Member

davehunt commented Sep 3, 2012

I feel that this adds a fragile link between the property name and the locator. It also implies that all properties would be retrieved using .text, which is not the case for many elements, such as form inputs for example. I also rather like the code completion that having the properties provides. Interested to see more feedback on this though, and really encourage these kind of pull requests. 😄

Collaborator

bobsilverberg commented Sep 3, 2012

Thanks for the feedback, @davehunt. Yes, the code completion is a major thing that we'd be losing by taking this approach. While it's true that not all properties use .text, my assumption was that those properties would still receive their own property definition. As long as those properties also have matching locators defined, a developer could still look at the locators in a page object as a guide to which properties exist, but I can also see how this mixing of getattr and defined properties could be very confusing. I just wish there was a way to eliminate all of that duplicate code. :-(

Contributor

zacc commented Sep 6, 2012

It looks good and conceptually sound but seems like a very dev-vy (I just made that up) thing to do. It looks like it'd fit right in a software package where large number of controlled combinations are used and they chase reduced duplication to the nth degree.

For a test I prefer the simpler even if a bit more verbose method. It helps new contributors and non-technical testers on our team understand what we're doing.

Collaborator

bobsilverberg commented Sep 6, 2012

Point taken. Not surprising to find a dev-vy approach from a developer. ;-) You can expect more stuff like that from me, but I won't take the rejection too badly.

Contributor

bebef1987 commented Sep 10, 2012

looks good... but I would remove the .text

We could start using directly the methods available in the selenium class.
So:
Class.method_name_text will become Class.method_name.text

And also if we need a get_attribute foe that method name we will use
Class.method_name.get_attribute('foo')

Member

davehunt commented Sep 10, 2012

@bebef1987 if you're suggesting that we use the Selenium API from within the tests then I would strongly advise against it. One of the main points of the page objects is to provide a layer of abstraction from the Selenium API. Apologies if I've misunderstood your suggestion.

Contributor

bebef1987 commented Sep 10, 2012

yeah I was thinking of something like that... But I see your point here and it's not one of my best suggestions...

Collaborator

bobsilverberg commented Nov 22, 2012

I am going to close this pull request as, while I think the ideas are interesting, it is not really appropriate for merging. Thanks for all of the comments everyone! 🍶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment