This repository has been archived by the owner. It is now read-only.

Bug 1008085 - [testing] Add Page Objects and Base Class #1

Merged
merged 1 commit into from May 12, 2014

Conversation

Projects
None yet
2 participants
@bitgeeky
Copy link
Member

bitgeeky commented May 9, 2014


@property
def is_the_current_page(self):
if self._page_title:

This comment has been minimized.

@bobsilverberg

bobsilverberg May 9, 2014

Collaborator

I know this was taken from other common code, but looking at it now it's a bit confusing. What happens at line 37 if self._page_title does not evaluate to True? It seems like we actually need to assume that self._page_title will always be set, and therefore we should always include the wait for self.selenium.title. I'm not sure what this if statement buys us.

This comment has been minimized.

@bitgeeky

bitgeeky May 11, 2014

Member

Assert.equal() gives a "not defined" error when one of the objects compared is not defined.
So it makes sense to wait for the page title to appear only when self._page_title is defined and otherwise directly give "not defined" error by doing an Assert.equal() without waiting for page title to appear.
Hence if self._page_title: should be there.

This comment has been minimized.

@bobsilverberg

bobsilverberg May 12, 2014

Collaborator

I don't think I made my point clear. We don't really want a test failing with "not defined" in any case, do we? We should always have self._page_title defined. If it isn't then that's an error that needs to be fixed. So, assuming that we will always have self._page_title defined (after we address any errors that crop up), there's no need for the if. Does that make sense?

This comment has been minimized.

@bitgeeky

bitgeeky May 12, 2014

Member

Its clear now. Agreed ! The tests should be written in a way that they should either pass or fail and not give "not defined" errors.

# set back to where you once belonged
self.selenium.implicitly_wait(self.testsetup.default_implicit_wait)

def get_url_current_page(self):

This comment has been minimized.

@bobsilverberg

bobsilverberg May 9, 2014

Collaborator

This seems like it would be more appropriate as a property. How about just current_url as a property?

This comment has been minimized.

@bitgeeky

bitgeeky May 11, 2014

Member

Agreed ! current_page_url sounds good !


_page_title = 'Mozilla One and Done'

def go_to_page(self):

This comment has been minimized.

@bobsilverberg

bobsilverberg May 9, 2014

Collaborator

I know we haven't done it like this before (at least not that I can remember), but what if we do away with this go_to_page method and include the navigation in the __init__ method? There may be times where we want to instantiate the home page without navigating to it (e.g., if a method from another page object should return the HomePage), so perhaps there can be an argument to the constructor to indicate whether to navigate or not. It would probably make sense for the default value of that argument to be False (i.e., do not navigate).

What do you think, @bitgeeky?

This comment has been minimized.

@bitgeeky

bitgeeky May 11, 2014

Member

@bobsilverberg , NO, TBH it doesn't sounds good to me.
Having a go_to_page() method makes it more clear to navigate to page.
So having a separate parameter and a condition would just increase complexity.

This comment has been minimized.

@bobsilverberg

bobsilverberg May 12, 2014

Collaborator

Fair enough. :)

Because this page is a "starting point" it's ok to have a go_to_page method, but I don't think we should include this method for any pages that are always accessed from a different page.

def click_sign_out(self):
self.selenium.find_element(*self._logout_menu_item_locator).click()

def wait_for_sign_out_visible(self):

This comment has been minimized.

@bobsilverberg

bobsilverberg May 9, 2014

Collaborator

I'm not a big fan of adding wait_for_x helper methods. I think we should look at the times when we will need to wait for something and then put it in that method. For example, we might need to wait for the sign out to be visible after logging in, so it would make sense to just put the wait into the login() method.

We also tend to follow a model where we never add anything to a page object unless it is actually needed for a test, so in that case some of these are premature. I'm OK with leaving some in - the ones you know you're going to be using for the first test you write, but it's a good practice to follow in the future.

This comment has been minimized.

@bitgeeky

bitgeeky May 11, 2014

Member

Agreed !
We can use wait_for_element_visible() in most cases. So this isn't required.

@bobsilverberg

This comment has been minimized.

Copy link
Collaborator

bobsilverberg commented May 12, 2014

Just one comment left to address, thanks @bitgeeky. :)

@bobsilverberg

This comment has been minimized.

Copy link
Collaborator

bobsilverberg commented May 12, 2014

LGTM. Thanks @bitgeeky

bobsilverberg added a commit that referenced this pull request May 12, 2014

Merge pull request #1 from bitgeeky/PageObjects
Bug 1008085 - [testing] Add Page Objects and Base Class

@bobsilverberg bobsilverberg merged commit 35a7f2d into mozilla:master May 12, 2014

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