Moztrap 5931 #133

Merged
merged 1 commit into from Sep 25, 2013

Conversation

Projects
None yet
6 participants
@sashakruglov
Contributor

sashakruglov commented Jul 28, 2013

This pull requests addresses issue #87.
Test case in Moztrap: https://moztrap.mozilla.org/manage/case/5931/

Also I did few other things in this PR:

  • moved all imports from tests to module-level (PEP-8 compliance)
  • made all tests destructive
  • factor out part of test code into separate method for better re-use over several tests
  • introduced new login option for returning users (interface to bidpom module)

I can later squash commits before merging it.

+ Assert.equal(len(filters), 1)
+ self.check_pinned_filter(filters[0], is_pinned=True)
+
+ def check_pinned_filter(self, filter_item, is_pinned):

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Aug 12, 2013

Contributor

nice that you added this method 👍

@AndreiH

AndreiH Aug 12, 2013

Contributor

nice that you added this method 👍

@AndreiH

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Aug 12, 2013

Contributor

The changes LGTM and tested this locally with great results 👍 . Thanks @sashakruglov, nice pull!

Contributor

AndreiH commented Aug 12, 2013

The changes LGTM and tested this locally with great results 👍 . Thanks @sashakruglov, nice pull!

+ def test_that_pinning_filter_persists_for_session(self, mozwebqa_logged_in, product, element):
+ # create suite, cases and test run
+ product_version_name = u'%s %s' % (product['name'], product['version']['name'])
+ case, suite, run = self.create_and_run_test(mozwebqa_logged_in, product, element)

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I don't see run being used anywhere. Did you add it to the create_and_run_test method just for completeness?

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I don't see run being used anywhere. Did you add it to the create_and_run_test method just for completeness?

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

yes, I don't need it, but I can't unpack result of self.create_and_run_test without it.

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

yes, I don't need it, but I can't unpack result of self.create_and_run_test without it.

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

Thanks, that is good catch, I'll remove it here and in return of create_and_run_test.

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

Thanks, that is good catch, I'll remove it here and in return of create_and_run_test.

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

But why did you add it to the result of create_and_run_test if it's not used anywhere in the suite?

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

But why did you add it to the result of create_and_run_test if it's not used anywhere in the suite?

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

I might thought that I will need it while working on pull and then forgot to remove

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

I might thought that I will need it while working on pull and then forgot to remove

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

I added check for run name in test, so it should be OK to have this variable

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

I added check for run name in test, so it should be OK to have this variable

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

Ok, this is fine by me.

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

Ok, this is fine by me.

+ # because we are going to check the same thing several times
+ # let's define a function and call it each time we need this check
+
+ def check_test_run_results(results):

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I'm not crazy about this function being defined within a test function. Was that intentional?

I think this will be confusing to some people. What do you think about moving it outside of the test function?

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I'm not crazy about this function being defined within a test function. Was that intentional?

I think this will be confusing to some people. What do you think about moving it outside of the test function?

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

yes, it's intentional. this function is not used by any other test, why should be defined in class level then? it's OK in python to define one function inside another function.

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

yes, it's intentional. this function is not used by any other test, why should be defined in class level then? it's OK in python to define one function inside another function.

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I understand why you did it, and yes, it's allowed in Python, but I'm just a bit worried that it might confuse other contributors. It's uncommon (in our test suites anyway), so while it does make sense it might do more harm than good. @zacc and/or @davehunt what do you think?

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

I understand why you did it, and yes, it's allowed in Python, but I'm just a bit worried that it might confuse other contributors. It's uncommon (in our test suites anyway), so while it does make sense it might do more harm than good. @zacc and/or @davehunt what do you think?

This comment has been minimized.

Show comment Hide comment
@davehunt

davehunt Aug 14, 2013

Member

It's not something we do elsewhere, but I'm fine with it.

@davehunt

davehunt Aug 14, 2013

Member

It's not something we do elsewhere, but I'm fine with it.

pages/login_page.py
+ self.wait_for_element_to_be_visible(*self._browserid_locator)
+ self.selenium.find_element(*self._browserid_locator).click()
+ sign_in_page = SignIn(self.selenium, timeout=self.timeout, expect='returning')
+ sign_in_page.click_sign_in_returning_user(expect='remember')

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

This is causing the browser to hang for me when running test_that_pinning_filter_persists_for_session. I'm not sure why, but the browser just hangs, and when I kill it and look through the trace [1], I can see that this is the last line of code from moztrap-tests that has been executed.

Can anyone else confirm or deny this behaviour?

[1] http://pastebin.mozilla.org/2839732

@bobsilverberg

bobsilverberg Aug 12, 2013

Collaborator

This is causing the browser to hang for me when running test_that_pinning_filter_persists_for_session. I'm not sure why, but the browser just hangs, and when I kill it and look through the trace [1], I can see that this is the last line of code from moztrap-tests that has been executed.

Can anyone else confirm or deny this behaviour?

[1] http://pastebin.mozilla.org/2839732

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

Looks like some trouble in browserid module, It hangs in lambda

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

Looks like some trouble in browserid module, It hangs in lambda

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 12, 2013

Contributor

It passed for me locally, but I have encounter another issue with wait_for_ajax.
It fails constantly.

@sashakruglov

sashakruglov Aug 12, 2013

Contributor

It passed for me locally, but I have encounter another issue with wait_for_ajax.
It fails constantly.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 13, 2013

Collaborator

Could someone else please try running test_that_pinning_filter_persists_for_session. I'm not sure why it's failing for me. It does seem like a problem in bidpom itself, looking at the trace, but something like that would likely cause bidpom tests to fail.

I will also run an adhoc of this pull request on CI.

Collaborator

bobsilverberg commented Aug 13, 2013

Could someone else please try running test_that_pinning_filter_persists_for_session. I'm not sure why it's failing for me. It does seem like a problem in bidpom itself, looking at the trace, but something like that would likely cause bidpom tests to fail.

I will also run an adhoc of this pull request on CI.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 13, 2013

Collaborator

Ok, I have seen what the issue is, and jrgm in #Identity explained how Persona works, which explains the issue. The screen that you see when logging back in, after entering the password, the one with "This is not my computer", is time based. It only shows up if the second login happens within 60 seconds of the first login, so sometimes it appears and sometimes it doesn't. The test fails and the browser hangs when the screen does not appear. This seems to be a problem with bidpom, so I will raise an issue for that there.

Collaborator

bobsilverberg commented Aug 13, 2013

Ok, I have seen what the issue is, and jrgm in #Identity explained how Persona works, which explains the issue. The screen that you see when logging back in, after entering the password, the one with "This is not my computer", is time based. It only shows up if the second login happens within 60 seconds of the first login, so sometimes it appears and sometimes it doesn't. The test fails and the browser hangs when the screen does not appear. This seems to be a problem with bidpom, so I will raise an issue for that there.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 13, 2013

Collaborator

I opened https://github.com/mozilla/bidpom/issues/67 about the bidpom issue. I will look into it.

Collaborator

bobsilverberg commented Aug 13, 2013

I opened https://github.com/mozilla/bidpom/issues/67 about the bidpom issue. I will look into it.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 13, 2013

Collaborator

Looking over the code here, I am a bit uncomfortable with how Moztrap-Tests needs to understand how bidpom works and interact with it in such a specific way.

I think it would be better if Moztrap-tests could simply attempt to log in using bidpom, regardless of whether it's an initial login or a "returning user" login, and then bidpom would know how to address the different scenarios.

Perhaps the issue is that bidpom cannot currently do that, and that is why the code was added to this repo. I'm going to look into it, and hopefully we'll be able to get rid of some of that extra code in here.

Collaborator

bobsilverberg commented Aug 13, 2013

Looking over the code here, I am a bit uncomfortable with how Moztrap-Tests needs to understand how bidpom works and interact with it in such a specific way.

I think it would be better if Moztrap-tests could simply attempt to log in using bidpom, regardless of whether it's an initial login or a "returning user" login, and then bidpom would know how to address the different scenarios.

Perhaps the issue is that bidpom cannot currently do that, and that is why the code was added to this repo. I'm going to look into it, and hopefully we'll be able to get rid of some of that extra code in here.

@sashakruglov

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Aug 13, 2013

Contributor

so, what is left unfixed?

Contributor

sashakruglov commented Aug 13, 2013

so, what is left unfixed?

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Aug 13, 2013

Collaborator

@sashakruglov I have submitted a pull request for bidpom [1] to address the issue we are seeing. It also introduces a new helper method sign_in_returning_user, which, if the pull request is accepted, could be used inside login_returning_user which would simplify it a bit.

Other than the fact that we need that bidpom issue addressed, and the pull updated once the bidpom changes are in, I think it's good to go.

Feel free to review the bidbpom pull request if you want to help move things along. :)

[1] mozilla/bidpom#68

Collaborator

bobsilverberg commented Aug 13, 2013

@sashakruglov I have submitted a pull request for bidpom [1] to address the issue we are seeing. It also introduces a new helper method sign_in_returning_user, which, if the pull request is accepted, could be used inside login_returning_user which would simplify it a bit.

Other than the fact that we need that bidpom issue addressed, and the pull updated once the bidpom changes are in, I think it's good to go.

Feel free to review the bidbpom pull request if you want to help move things along. :)

[1] mozilla/bidpom#68

@AndreiH

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Sep 16, 2013

Contributor

Pull mozilla/bidpom#68 was merged and the issue should be fine now
Tested this and the code looks good to me :)
@bobsilverberg I will let you do the honours if you have nothing else to add

Contributor

AndreiH commented Sep 16, 2013

Pull mozilla/bidpom#68 was merged and the issue should be fine now
Tested this and the code looks good to me :)
@bobsilverberg I will let you do the honours if you have nothing else to add

@teodosia

This comment has been minimized.

Show comment Hide comment
@teodosia

teodosia Sep 16, 2013

Contributor

Lgtm too.

Contributor

teodosia commented Sep 16, 2013

Lgtm too.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Sep 16, 2013

Collaborator

Although the bidpom PR was merged, Moztrap-tests has not been updated to use the latest version, so you likely tested this without those changes. I'm going to look into updating it soon, at which point the PR could be updated to use the new method.

Collaborator

bobsilverberg commented Sep 16, 2013

Although the bidpom PR was merged, Moztrap-tests has not been updated to use the latest version, so you likely tested this without those changes. I'm going to look into updating it soon, at which point the PR could be updated to use the new method.

@AndreiH

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Sep 16, 2013

Contributor

yeah, you're right. waiting for the update then

Contributor

AndreiH commented Sep 16, 2013

yeah, you're right. waiting for the update then

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Sep 18, 2013

Collaborator

This is now waiting on a merge of #135, after which we can test it with the new bidpom.

Collaborator

bobsilverberg commented Sep 18, 2013

This is now waiting on a merge of #135, after which we can test it with the new bidpom.

@sashakruglov

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Sep 23, 2013

Contributor

So, I need to migrate this test to new bidpom interface?

Contributor

sashakruglov commented Sep 23, 2013

So, I need to migrate this test to new bidpom interface?

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Sep 23, 2013

Collaborator

Yes @sashakruglov. Please use the new helper method that does not require any expectations. Thanks.

Collaborator

bobsilverberg commented Sep 23, 2013

Yes @sashakruglov. Please use the new helper method that does not require any expectations. Thanks.

@@ -35,8 +36,6 @@ def username_text(self):
def click_logout(self):
self.selenium.find_element(*self._logout_locator).click()
- from pages.login_page import MozTrapLoginPage

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Sep 23, 2013

Contributor

It seems that user is not redirected to LoginPage after logout.

There are only two tests that do logout - one in test_homepage.py and one in this PR,
none of them use MozTrapLoginPage returned by this method, so I decided to remove it.

@sashakruglov

sashakruglov Sep 23, 2013

Contributor

It seems that user is not redirected to LoginPage after logout.

There are only two tests that do logout - one in test_homepage.py and one in this PR,
none of them use MozTrapLoginPage returned by this method, so I decided to remove it.

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Sep 23, 2013

Contributor

Oh, I see, pages that require authentication redirect user to LoginPage on logout, so maybe it was hasty decision to remove it.

@sashakruglov

sashakruglov Sep 23, 2013

Contributor

Oh, I see, pages that require authentication redirect user to LoginPage on logout, so maybe it was hasty decision to remove it.

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Sep 24, 2013

Contributor

I think that was added for a reason

@AndreiH

AndreiH Sep 24, 2013

Contributor

I think that was added for a reason

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Sep 24, 2013

Contributor

Perhaps, but it's not used anywhere anyway

@sashakruglov

sashakruglov Sep 24, 2013

Contributor

Perhaps, but it's not used anywhere anyway

@AndreiH

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Sep 24, 2013

Contributor

I think this is ready to be merged, lgtm

Contributor

AndreiH commented Sep 24, 2013

I think this is ready to be merged, lgtm

@AlinT

This comment has been minimized.

Show comment Hide comment
@AlinT

AlinT Sep 24, 2013

Contributor

Lgtm also
@bobsilverberg I'll leave the merge to you, if you think it's ready.

Contributor

AlinT commented Sep 24, 2013

Lgtm also
@bobsilverberg I'll leave the merge to you, if you think it's ready.

@bobsilverberg

This comment has been minimized.

Show comment Hide comment
@bobsilverberg

bobsilverberg Sep 24, 2013

Collaborator

Yes, LGTM too and passes. r+

Passed on CI via adhoc job too [1].

@sashakruglov Please squash your commits and we can merge.

[1] http://qa-selenium.mv.mozilla.com:8080/job/moztrap.dev.saucelabs.adhoc/70/

Collaborator

bobsilverberg commented Sep 24, 2013

Yes, LGTM too and passes. r+

Passed on CI via adhoc job too [1].

@sashakruglov Please squash your commits and we can merge.

[1] http://qa-selenium.mv.mozilla.com:8080/job/moztrap.dev.saucelabs.adhoc/70/

@sashakruglov

This comment has been minimized.

Show comment Hide comment
@sashakruglov

sashakruglov Sep 24, 2013

Contributor
Contributor

sashakruglov commented Sep 24, 2013

@AndreiH

This comment has been minimized.

Show comment Hide comment
@AndreiH

AndreiH Sep 25, 2013

Contributor

Run it myself a couple of times on CI with success
http://qa-selenium.mv.mozilla.com:8080/view/MozTrap/job/moztrap.dev.saucelabs.adhoc/73/
Merging this.
Thanks again @sashakruglov !

Contributor

AndreiH commented Sep 25, 2013

Run it myself a couple of times on CI with success
http://qa-selenium.mv.mozilla.com:8080/view/MozTrap/job/moztrap.dev.saucelabs.adhoc/73/
Merging this.
Thanks again @sashakruglov !

AndreiH added a commit that referenced this pull request Sep 25, 2013

@AndreiH AndreiH merged commit f528671 into mozilla:master Sep 25, 2013

@sashakruglov sashakruglov deleted the sashakruglov:moztrap_5931 branch Sep 25, 2013

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