Add IdentityPopup.open() as helper method

RESOLVED FIXED in Firefox 43

Status

Testing
Firefox UI Tests
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: whimboo, Assigned: goma)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [lang=py], URL)

Attachments

(1 attachment, 1 obsolete attachment)

52 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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

3 years ago
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.
(Reporter)

Comment 2

3 years ago
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.
Assignee: nobody → kavitmht
Status: NEW → ASSIGNED

Comment 3

3 years ago
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.
(Reporter)

Comment 4

3 years ago
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

3 years ago
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
QA Contact: hskupin → kavitmht

Comment 6

3 years ago
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.
(Reporter)

Comment 7

3 years ago
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.
Flags: needinfo?(kavitmht)
(Reporter)

Comment 8

3 years ago
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.
(Reporter)

Comment 9

3 years ago
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.
Assignee: kavitmht → nobody
Mentor: hskupin
Status: ASSIGNED → NEW
Flags: needinfo?(kavitmht) → needinfo?(gbrmachado)
QA Contact: kavitmht → hskupin
(Assignee)

Comment 10

3 years ago
Yes, I definitely would like to work on this bug :)
Thanks
Flags: needinfo?(gbrmachado)
(Reporter)

Comment 11

3 years ago
That's great! So Chris or Joel, can one of you mentor Gabriel here?
Assignee: nobody → gbrmachado
Flags: needinfo?(jmaher)
Flags: needinfo?(cmanchester)
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.
Flags: needinfo?(jmaher)
Flags: needinfo?(cmanchester)
(Assignee)

Comment 13

3 years ago
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??
Flags: needinfo?(cmanchester)
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.
Flags: needinfo?(cmanchester)
(Assignee)

Comment 15

3 years ago
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
(Reporter)

Comment 16

3 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 17

3 years ago
Created attachment 8643855 [details] [review]
Link to github pull request

https://github.com/mozilla/firefox-ui-tests/pull/217
Attachment #8643855 - Flags: feedback?(hskupin)
(Reporter)

Comment 18

3 years ago
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.
Attachment #8643855 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 19

3 years ago
Are the tests running ok on linux?
I've been finding some errors when I run the program, even on the master branch.
Flags: needinfo?(hskupin)
(Reporter)

Comment 20

3 years ago
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.
Flags: needinfo?(hskupin)
(Assignee)

Comment 21

3 years ago
Created attachment 8655953 [details] [review]
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)
Attachment #8643855 - Attachment is obsolete: true
Attachment #8655953 - Flags: review?(hskupin)
(Reporter)

Comment 22

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

r=me with the remaining instance replaced.
Attachment #8655953 - Flags: review?(hskupin) → review+
(Reporter)

Comment 23

3 years ago
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.