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

Adds functions to test for link correctness, validity, and visibility #153

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

casschin commented Dec 10, 2012

When you add new tests, there's a lot of reused code that I thought could be turned into functions. This should reduce some of the code.

I'm adding it to only this test to see if this method is ok to use before adding it to other pages.

I also renamed the tests for the footers to match the naming scheme of the other tests on for the page.

casschin added some commits Dec 10, 2012

@casschin casschin Adds functions to test for link correctness, validity, and visibility
When you add new tests, there's a lot of reused code that I thought
could be turned into functions. This should reduce some of the code.

I also renamed the tests for the footers to match the naming scheme of
the other tests on for the page.

I'm adding it to only this test to see if this method is ok to use
before adding it to other pages of the test.
93e71d6
@casschin casschin Fixes footer test name typo 486e928

@bebef1987 bebef1987 commented on the diff Dec 10, 2012

tests/test_about.py
@pytest.mark.nondestructive
def test_major_link_urls_are_valid(self, mozwebqa):
about_page = AboutPage(mozwebqa)
about_page.go_to_page()
- bad_urls = []
- for link in about_page.major_links_list:
- url = about_page.link_destination(link.get('locator'))
- if not about_page.is_valid_link(url):
- bad_urls.append('%s is not a valid url' % url)
- Assert.equal(0, len(bad_urls), '%s bad urls found: ' % len(bad_urls) + ', '.join(bad_urls))
+ about_page.are_links_valid(about_page.major_links_list)
@bebef1987

bebef1987 Dec 10, 2012

Contributor

new line at the end of the file

Collaborator

bobsilverberg commented Dec 10, 2012

I like the idea and the implementation. My only concern is that it may obfuscate the logic of the test for less experienced people working on the tests, which is something we try to avoid.

Contributor

bebef1987 commented Dec 10, 2012

Collaborator

bobsilverberg commented Dec 10, 2012

+1 to my liking of the idea/code or +1 to my concern? Or both?

Contributor

bebef1987 commented Dec 10, 2012

+1 for the concern we take out the asserts from the test so it might be a little hard to understand them...
For us it's ok but the tests are also used by untrained eyes

Contributor

casschin commented Dec 11, 2012

okay face

Is the update to the footer tests valid? If so, I can remove the function commits.

Collaborator

m8ttyB commented Dec 11, 2012

I'd suggest taking the discussion to the mailing list - mozwebqa@mozilla.org - for the rest of the group to discuss. @casschin's implementation is a fairly radical notion that departs from how webqa's projects have implemented POM.

I do really like the notion of removing duplicate code and adding helper methods. My initial reservation is that we shouldn't move the assertion logic out of the tests and into the page object because it obfuscates the code.

@m8ttyB m8ttyB commented on the diff Dec 11, 2012

tests/test_about.py
about_page = AboutPage(mozwebqa)
about_page.go_to_page()
Assert.contains(about_page.footer.expected_footer_logo_destination,
about_page.footer.footer_logo_destination)
Assert.contains(about_page.footer.expected_footer_logo_img,
about_page.footer.footer_logo_img)
- bad_links = []
- for link in AboutPage.Footer.footer_links_list:
- url = about_page.link_destination(link.get('locator'))
- if not url.endswith(link.get('url_suffix')):
- bad_links.append('%s does not end with %s' % (url, link.get('url_suffix')))
- Assert.equal(0, len(bad_links), '%s bad links found: ' % len(bad_links) + ', '.join(bad_links))
@m8ttyB

m8ttyB Dec 11, 2012

Collaborator

Epic-good find. That's a lot of duplicate code!

Contributor

teodosia commented Jan 7, 2013

Looks like there area merge conflicts. Could you please update the pull and solve them? Thanks!

Contributor

casschin commented Jan 9, 2013

I was under the impression that this pull request was being rejected. Is that not the case anymore?

Collaborator

bobsilverberg commented Jan 16, 2013

@casschin It was great to meet you last night. Yes, I think this pull request is no longer relevant, due to the abstraction ideas being rejected. I am going to close it, but please feel free to find something else to work on. We look forward to seeing your next pull request. 🎱

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