Fix failing test due to increase number of addons #76

Merged
merged 5 commits into from Oct 19, 2012

Conversation

Projects
None yet
4 participants
@AlinT
Contributor

AlinT commented Oct 18, 2012

Due to the fact that number of the addons has increased and only 10 add-ons are displayed at one time, i've modified the count method so that it treats this case.

pages/dashboard_page.py
@@ -35,7 +36,11 @@ def addons_count_label(self):
return self.selenium.find_element(*self._addons_public_counter).text
def addons_element_count(self):
- return len(self.selenium.find_elements(*self.Addon._base_locator))
+ elements = len(self.selenium.find_elements(*self.Addon._base_locator))
+ while self.is_element_visible(*self._next_locator):

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

This will not count the elements on the last page because the next button is not visible on the last page :)

@zacc

zacc Oct 18, 2012

Contributor

This will not count the elements on the last page because the next button is not visible on the last page :)

@zacc

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

Can you include in the description the test that is being affected so that we can see how this property is being used in the test?

Contributor

zacc commented Oct 18, 2012

Can you include in the description the test that is being affected so that we can see how this property is being used in the test?

pages/dashboard_page.py
+ elements = len(self.selenium.find_elements(*self.Addon._base_locator))
+ while self.is_element_visible(*self._next_locator):
+ self.selenium.find_element(*self._next_locator).click()
+ elements = elements + len(self.selenium.find_elements(*self.Addon._base_locator))

This comment has been minimized.

Show comment Hide comment
@AlinT

AlinT Oct 18, 2012

Contributor

That will not count the elements, but this will.

@AlinT

AlinT Oct 18, 2012

Contributor

That will not count the elements, but this will.

@zacc

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

@AlinT is right.. this lgtm :)

Contributor

zacc commented Oct 18, 2012

@AlinT is right.. this lgtm :)

pages/dashboard_page.py
- return len(self.selenium.find_elements(*self.Addon._base_locator))
+ addons_count = len(self.selenium.find_elements(*self.Addon._base_locator))
+ while self.is_element_visible(*self._next_locator):
+ self.selenium.find_element(*self._next_locator).click()

This comment has been minimized.

Show comment Hide comment
@davehunt

davehunt Oct 18, 2012

Member

A method or property in a page object should represent the page, however this method is loading additional pages and combining the result. 👎

@davehunt

davehunt Oct 18, 2012

Member

A method or property in a page object should represent the page, however this method is loading additional pages and combining the result. 👎

This comment has been minimized.

Show comment Hide comment
@AlinT

AlinT Oct 18, 2012

Contributor

The method there represents the addons count from several pages(The number of the pages does not count, because it will always take the sum of the number of addons from all the public addons pages).
Well although this method or property does not represent the page, it points to the addons section of the page and provides the number of addons required for the test.
If the testcase is the problem maybe it should be revised, also I don't see any flakiness in the test or in what it tests.

@AlinT

AlinT Oct 18, 2012

Contributor

The method there represents the addons count from several pages(The number of the pages does not count, because it will always take the sum of the number of addons from all the public addons pages).
Well although this method or property does not represent the page, it points to the addons section of the page and provides the number of addons required for the test.
If the testcase is the problem maybe it should be revised, also I don't see any flakiness in the test or in what it tests.

This comment has been minimized.

Show comment Hide comment
@davehunt

davehunt Oct 18, 2012

Member

The method there represents the addons count from several pages

That's the problem. A page object represents a single page.

@davehunt

davehunt Oct 18, 2012

Member

The method there represents the addons count from several pages

That's the problem. A page object represents a single page.

@bebef1987

This comment has been minimized.

Show comment Hide comment
@bebef1987

bebef1987 Oct 18, 2012

Contributor

this can be done in the test...
or a in a basetest class

Contributor

bebef1987 commented Oct 18, 2012

this can be done in the test...
or a in a basetest class

pages/dashboard_page.py
+ self.selenium.find_element(*self._previous_locator).click()
+
+ @property
+ def is_next_button_disabled(self):

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

this should be is_next_button_enabled because it's returning True for it being visible.

@zacc

zacc Oct 18, 2012

Contributor

this should be is_next_button_enabled because it's returning True for it being visible.

+ self.selenium.find_element(*self._next_locator).click()
+ return DashboardPage(self.testsetup)
+
+ def click_previous(self):

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

should this return a new instance of DashboardPage like click_next does?

@zacc

zacc Oct 18, 2012

Contributor

should this return a new instance of DashboardPage like click_next does?

This comment has been minimized.

Show comment Hide comment
@AlinT

AlinT Oct 18, 2012

Contributor

updated

@AlinT

AlinT Oct 18, 2012

Contributor

updated

@zacc

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 18, 2012

Contributor

OK I've run this again and it passes. Glad we went with the better solution, thanks @davehunt for persisting :) . Do you want to do the final review/button press?

Contributor

zacc commented Oct 18, 2012

OK I've run this again and it passes. Glad we went with the better solution, thanks @davehunt for persisting :) . Do you want to do the final review/button press?

@davehunt

This comment has been minimized.

Show comment Hide comment
@davehunt

davehunt Oct 18, 2012

Member

Much better. LGTM! 😺

Member

davehunt commented Oct 18, 2012

Much better. LGTM! 😺

@zacc

This comment has been minimized.

Show comment Hide comment
@zacc

zacc Oct 19, 2012

Contributor

OK merging!

Contributor

zacc commented Oct 19, 2012

OK merging!

zacc pushed a commit that referenced this pull request Oct 19, 2012

zacc
Merge pull request #76 from AlinT/addon_count_fail
Fix failing test due to increase number of addons

@zacc zacc merged commit b6392ba into mozilla:master Oct 19, 2012

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