1146296 - Add IdentityPopup.open() as helper method

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We have a lot of places which use the following code:

> self.identity_popup.box.click()
> Wait(self.marionette).until(lambda _: self.identity_popup.is_open)

We should implement an open() method similar to close() for the IdentityPopup class, which hides this logic. Right now I can only see this click as possible opener method, so no distinction would have to be made like in the window open() method.

Comment 1

[Kavit Mehta]

Hi Henrik I am up to try fix this bug. I looked into the repo, but I was unbale to find the IdentityPopup class. Can you tell me in which folder it is located.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you for your interest in working on this bug. I will assign it to you so others know that this bug is taken.

Regarding your question... the class is part of the toolbars ui module and can be found here:
https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_puppeteer/ui/toolbars.py#L343

Let me or others know if you have other questions. I'm out the next days so Chris might help you out meanwhile.

Comment 3

[Kavit Mehta]

Hi Chris and Henrik, so I was going through the code and I want to know what exactly happens when the above two statements get executed. I check the whole source code and I found that these two statements are being called only at two locations. One more thing I want to ask is these two statements need to be called from inside an open() function that will a member function of IdentityPopup class, right?
Thanks.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, that is right. Right now this might be only two locations in our source but we are getting more security tests in the remote folder landed soon, so if you patch lands first we would hvae to update those other tests to make use of this open method. If it takes a bit longer for you, you might have to replace some more instances.

Comment 5

[Kavit Mehta]

Hi Henrik, I have made the changes as you have asked I will send you a pull request link of github. you check it and let me know.

Thanks,
-Kavit

Comment 6

[Kavit Mehta]

Hi Henrik When I create a pull request it gives me a merge error. Is it because of the changes I made or it happens sometimes.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kavit, sorry for being late but I was on PTO for a month. Do you have a link to this pull request? If you see a merge error, its because your local master is outdated and does not correspond to the latest state on upstream master. You will have to merge in all the latest upstream changes.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have now seen your PR https://github.com/mozilla/firefox-ui-tests/pull/151. When you check the results from travis (https://travis-ci.org/mozilla/firefox-ui-tests/jobs/57094015) you can see that all is related to block comment formatting. Just remove all the comments you added, and it should pass this part.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that we haven't gotten any reply from Kavit in the last nearly 3 months, I'm going to put this bug back into the general bucket.

Gabriel, would you have interest to work on it? Given that I will be away the next 4 weeks, we will have to find another mentor for you. Maybe Joel or Chris are interested.

Comment 10

[Gabriel Machado [:goma]]

Yes, I definitely would like to work on this bug :)
Thanks

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's great! So Chris or Joel, can one of you mentor Gabriel here?

Comment 12

[Chris Manchester (limited bugmail, email directly)]

I'd be happy to provide feedback on a pull request, feel free to @ mention me on github or paste the url to a pull request in an attachment on this bug and request feedback or review.

Comment 13

[Gabriel Machado [:goma]]

Hello Chris :)
I have some questions:

A) Looking in the code, it seems that the code mentioned by Henrik in the first post has been changed. I've found just:
        self.locationbar.identity_box.click()
        Wait(self.marionette).until(lambda _: self.identity_popup.is_open)

Instead of:
        self.identity_popup.box.click()
        Wait(self.marionette).until(lambda _: self.identity_popup.is_open)

Should I use the first version in the new method?

B) I've updated some libraries, and now the test is not running ok locally. Is it some problem in my computer or some conflicts with these news libraries??

Comment 14

[Chris Manchester (limited bugmail, email directly)]

The more recent code is the version to use (we sometimes need to update our libraries when something changes in the firefox UI).

If you've changed code and tests started failing, that may be as a result of the code changes. The best thing to do at this point would be to open a pull request, which will automatically test your changes. Then we can take a look at what might be happening.

Comment 15

[Gabriel Machado [:goma]]

The test is failing even on master branch in my computer(without any modification)
I've opened a pull request, and the test is failing on macOs, but it is Ok on Linux.

https://github.com/mozilla/firefox-ui-tests/pull/213

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We do not run the test on Linux because it runs in headless mode and would cause test failures across various tests. This includes all tests which handle popup elements. I will comment on the PR what I think is wrong.

Comment 17

[Gabriel Machado [:goma]]

Attachment: Link to github pull request

https://github.com/mozilla/firefox-ui-tests/pull/217

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8643855 [details] [review]
Link to github pull request

Thanks for the update Gabriel. It definitely goes into the right direction, but you mixed up the usage of self a couple of times. I gave some comments on the PR which should help you in solving the remaining issues.

Comment 19

[Gabriel Machado [:goma]]

Are the tests running ok on linux?
I've been finding some errors when I run the program, even on the master branch.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The tests should work fine now. About 2 days ago we pushed some changes which fixed some failures. So please rebase your PR to latest master and try again. If it still fails please let me know which tests are affected. Thanks.

Comment 21

[Gabriel Machado [:goma]]

Attachment: Link to Github Pull Request

Hey Whimboo. It seems that everything is working fine now. I've tested on my computer and I had just one error(but I'm getting the same error on the master branch too, probably it's related with my Linux Window Manager)

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8655953 [details] [review]
Link to Github Pull Request

r=me with the remaining instance replaced.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The PR got updated meanwhile and I pushed it to master:
https://github.com/mozilla/firefox-ui-tests/commit/98cfdc40e7e32353da0aeac2126e73b803f04e64

Thank you Gabriel for this patch! It's a nice enhancement and I hope you learned something again. Maybe you have interests on another bug?