Closed
Bug 1146296
Opened 9 years ago
Closed 9 years ago
Add IdentityPopup.open() as helper method
Categories
(Testing :: Firefox UI Tests, defect)
Testing
Firefox UI Tests
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: whimboo, Assigned: goma)
References
()
Details
(Whiteboard: [lang=py])
Attachments
(1 file, 1 obsolete file)
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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
Yes, I definitely would like to work on this bug :) Thanks
Flags: needinfo?(gbrmachado)
Reporter | ||
Comment 11•9 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)
Comment 12•9 years ago
|
||
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•9 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)
Comment 14•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
https://github.com/mozilla/firefox-ui-tests/pull/217
Attachment #8643855 -
Flags: feedback?(hskupin)
Reporter | ||
Comment 18•9 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•9 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•9 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•9 years ago
|
||
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•9 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•9 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
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•8 years ago
|
Product: Mozilla QA → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•