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.
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.
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
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.
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.
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
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.
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.
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.
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
Status: ASSIGNED → NEW
Flags: needinfo?(kavitmht) → needinfo?(gbrmachado)
QA Contact: kavitmht → hskupin
Yes, I definitely would like to work on this bug :) Thanks
That's great! So Chris or Joel, can one of you mentor Gabriel here?
Assignee: nobody → gbrmachado
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.
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??
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.
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
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
Created attachment 8643855 [details] [review] Link to github pull request https://github.com/mozilla/firefox-ui-tests/pull/217
Attachment #8643855 - Flags: feedback?(hskupin)
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+
Are the tests running ok on linux? I've been finding some errors when I run the program, even on the master branch.
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.
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)
Comment on attachment 8655953 [details] [review] Link to Github Pull Request r=me with the remaining instance replaced.
Attachment #8655953 - Flags: review?(hskupin) → review+
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
You need to log in before you can comment on or make changes to this bug.