Closed
Bug 1251600
Opened 8 years ago
Closed 8 years ago
test_page_info_window.py doesn't run test for opening/closing the window via menu
Categories
(Testing :: Firefox UI Tests, defect)
Testing
Firefox UI Tests
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: whimboo, Assigned: zach.munrocape, Mentored)
References
Details
(Whiteboard: [lang=py][good first bug])
Attachments
(1 file, 2 obsolete files)
Just seen this while checking for self.platform. The following test silently skips the test for opening and closing the page info window because it does not compare the platform with lowercase letters. https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/firefox_ui_tests/puppeteer/test_page_info_window.py https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/__init__.py#67 To get started in fixing this please read through https://github.com/mozilla/ateam-bootcamp.
Assignee | ||
Comment 1•8 years ago
|
||
Hello, I have a development environment set up and have read through the A-team guide. I was wondering what the best way to resolve this would be. Given that platform is a all lower case string, should it be compared to "windows_nt" and "darwin"? Thanks for your time. Cheers, Zach
Reporter | ||
Comment 2•8 years ago
|
||
Yes, we would need lowercase for both, and additionally the line for windows_nt should also use self.platform and not self.marionette.session_capabilities.
Assignee | ||
Comment 3•8 years ago
|
||
Thanks for the feedback, Henrik. I'll change the usage to self.platform and submit a patch this evening.
Assignee | ||
Comment 4•8 years ago
|
||
Attached is a patch I would like to submit for review. I tried running the tests with mach.
Comment 5•8 years ago
|
||
(In reply to Zach Munro-Cape from comment #4) > Attached is a patch I would like to submit for review. I tried running the > tests with mach. Can you please change this to use the platformName capability? platform is deprecated: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L142 The background for this change is that the WebDriver specification previously had a requirement that the platform had to be normalised to a certain set of allowed strings. This is no longer the case, and we simply return what Services.sysinfo.getProperty("name") gives us instead.
Comment 6•8 years ago
|
||
I submitted bug 1253186 to issue a warning when clients attempt to use the platform capability.
Assignee | ||
Comment 7•8 years ago
|
||
Thanks. I've updated accordingly.
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Andreas Tolfsen ‹:ato› from comment #5) > (In reply to Zach Munro-Cape from comment #4) > > Attached is a patch I would like to submit for review. I tried running the > > tests with mach. > > Can you please change this to use the platformName capability? platform is > deprecated: > https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver. > js#L142 Andreas, this has nothing to do with Marionette itself. `self.platform` is a property as set by Firefox Puppeteer and which relies on the capability property of Marionette. Zach if you can revert that it would be great. Also a patch as submitted via mozreview would be great. See the following link for details: https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#mozreview-commits
Assignee | ||
Comment 9•8 years ago
|
||
Thanks - I've read that over. However, when I attempt to push to review (or even associate my key) I get a permission denied error. I have noticed that a API key was generated that was associated with reviewboard. However, If I try to push to it, I get a permission denied (publickey) and trying to SSH also fails for this reason. Is this a common issue? The apikey in my hg config is the one that I set up at first for bzexport.
Comment 10•8 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #8) > Andreas, this has nothing to do with Marionette itself. `self.platform` is a > property as set by Firefox Puppeteer and which relies on the capability > property of Marionette. Oh I see. Please disregard my comment above.
Comment 11•8 years ago
|
||
(In reply to Zach Munro-Cape from comment #9) > Thanks - I've read that over. However, when I attempt to push to review (or > even associate my key) I get a permission denied error. > > I have noticed that a API key was generated that was associated with > reviewboard. However, If I try to push to it, I get a permission denied > (publickey) and trying to SSH also fails for this reason. > > Is this a common issue? The apikey in my hg config is the one that I set up > at first for bzexport. What error are you getting, specifically? Have you set "UseRoaming no" in your ~/.ssh/config file?
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37893/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37893/
Attachment #8726274 -
Flags: review?(hskupin)
Reporter | ||
Updated•8 years ago
|
Attachment #8726067 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8726199 -
Attachment is obsolete: true
Reporter | ||
Comment 13•8 years ago
|
||
Zach, great work in getting everything setup without much further help. The patch looks fine to me but before I want to r+ it, do you have a chance to test it on OS X and Windows? For try we only have linux support, so we won't catch regressions for both changes.
Assignee: nobody → zach.munrocape
Status: NEW → ASSIGNED
Flags: needinfo?(zach.munrocape)
Assignee | ||
Comment 14•8 years ago
|
||
w.r.t the permissions, it turns out I got the LDAP and https setup instructions crossed. Thankfully there were some helpful people on IRC :) Thanks for the feedback. Unfortunately, I was working on a Mint VM. I could try to build on my Mac - but might run into some environment setup issues (I just read through the setup guide, and it looks like there are a few conflicting dependencies). Furthermore, I unfortunately do not have a Windows machine. What is the usual workaround in these situations?
Flags: needinfo?(zach.munrocape)
Reporter | ||
Comment 15•8 years ago
|
||
(In reply to Zach Munro-Cape from comment #14) > Thanks for the feedback. Unfortunately, I was working on a Mint VM. I could > try to build on my Mac - but might run into some environment setup issues (I > just read through the setup guide, and it looks like there are a few > conflicting dependencies). The only thing which you will have to do is running `./mach configure`. With the patch on bug 1252586 landed lately we can now use the `--binary` option for `./mach firefox-ui-test` to run a different binary as the self-built one. I believe it will help you a lot. > Furthermore, I unfortunately do not have a Windows machine. I think that I could run a test for you here given that I have a Windows machine. I will do when your testing on Mac was successful. > What is the usual workaround in these situations? Ideally we want tryserver support (https://wiki.mozilla.org/Build:TryServer#Try_Server) for all platforms so you could simply push your changes to that remote end. But for now we have Linux support only. See bug 1237550 which I landed lately. It would still take a bit until TaskCluster will support OS X and Windows. Until then we have to test platform specific changes manually. General changes should be fine with Linux builds only.
Reporter | ||
Comment 16•8 years ago
|
||
Zach, I would like to know who it goes. Do you need any help with or haven't had you time so far for those requested checks? Do you want me to do those? Thanks
Flags: needinfo?(zach.munrocape)
Assignee | ||
Comment 17•8 years ago
|
||
The Mac build was tricky - particularily with my version of autoconf213. It requires an uninstall, install of a older version, and then some symlinking which I don't think I've gotten linked completely. I am thinking of trying again this evening. If you have a Mac build laying around that would be quicker and of help. But moving forward I will need to get the Mac installation set up anyway.
Flags: needinfo?(zach.munrocape)
Reporter | ||
Comment 18•8 years ago
|
||
Oh, you won't have to build Firefox on OS X. We recently landed a patch which let you use --binary with the mach command. You can easily specify a recent Nightly (http://nightly.mozilla.org/) build and run your tests. That's totally fine and saves you a lot of time. The only thing you would have to do is to run `mach configure` first.
Assignee | ||
Comment 19•8 years ago
|
||
Ah that is what you meant! I see. I was trying to pass a binary into configure. I ran it with the nightly Mac build. 92 tests pass, 1 skipped - test_action_locationbar.py.
Assignee | ||
Comment 20•8 years ago
|
||
With respect to the NT test - would any windows build be sufficient?
Assignee | ||
Comment 21•8 years ago
|
||
hello henrik, I was wondering if I could get an update on what do to next or on reviewboard? Thanks, Zach
Reporter | ||
Comment 22•8 years ago
|
||
Hi Zach, sorry that I missed the second comment from you on March 12th. First it's good to hear that everything worked well on OS X. In case you want/can test Windows that would be great. For that run any version of Windows would be sufficient, yes.
Assignee | ||
Comment 23•8 years ago
|
||
No worries :) I ran `./mach configure`, `./mach build`, then `./mach firefox-ui-test --binary path/to/win64/image.img`. 93 tests ran, 92 passed, 1 was skipped.
Reporter | ||
Comment 24•8 years ago
|
||
Comment on attachment 8726274 [details] MozReview Request: Bug 1251600 - Removed capitalization when comaring OS with platform name; r?whimboo https://reviewboard.mozilla.org/r/37893/#review37953 Nothing to complain about. For safety I also triggered a try run now. If that passes, I will land your patch on inbound. Thanks a lot for working on it, and let me know if you are interested in another bug.
Attachment #8726274 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 25•8 years ago
|
||
Seems like the build was a success. Thanks for your help and guidance. I was wondering if there are any other ateam issues up for grabs now that I am familiar with the workflow.
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8726274 [details] MozReview Request: Bug 1251600 - Removed capitalization when comaring OS with platform name; r?whimboo https://reviewboard.mozilla.org/r/37893/#review38085 Just noticed that there is a typo in the commit message. Can you please correct it? Also it would be good to use a wording which visualizes what has been changed. So something like: "Bug 1251600 - Use Puppeteer platform property for comparing platform names. r?whimboo". Thanks.
Attachment #8726274 -
Flags: review+
Reporter | ||
Comment 27•8 years ago
|
||
(In reply to Zach Munro-Cape from comment #25) > Thanks for your help and guidance. I was wondering if there are any other > ateam issues up for grabs now that I am familiar with the workflow. Great! So there is bug 1143928 which would be good to see solved as well. It's halfway done so far and the contributor worked on the first part is not able to continue.
Reporter | ||
Comment 28•8 years ago
|
||
Zach, mind doing the remaining change to the commit message? With it corrected I can easily push through the web interface. Thanks.
Flags: needinfo?(zach.munrocape)
Assignee | ||
Comment 29•8 years ago
|
||
Sorry for the delay - school was a bit hectic. Will do that in the next hour or so. Thanks for catching that.
Flags: needinfo?(zach.munrocape)
Assignee | ||
Comment 30•8 years ago
|
||
I changed the commit message, but the review title didn't update. Do I need to create a new review entirely?
Assignee | ||
Comment 31•8 years ago
|
||
I then pushed to review board, it didn't pick the change up but showed an entry. I used mq to generate the match so `hg commit --amend` doesn't allow me to edit anything.
Reporter | ||
Comment 32•8 years ago
|
||
I'm not an expert of hg and didn't use mq for a while. So not sure how to do it. Might histedit work also for the mq extension?
Reporter | ||
Comment 33•8 years ago
|
||
Ok, I will pull the changes now to my local disk, get the commit message updated, and landed. So don't worry about the remaining hg trouble.
Reporter | ||
Comment 35•8 years ago
|
||
Comment on attachment 8726274 [details] MozReview Request: Bug 1251600 - Removed capitalization when comaring OS with platform name; r?whimboo https://reviewboard.mozilla.org/r/37893/#review39467 I also had to update the path for the patched file given that the firefox-ui-package doesn't exist anymore. Anyway, code-wise this is fine to lets put r+ on it! Thanks a lot.
Attachment #8726274 -
Flags: review+
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c0fa17b1195d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•