Closed Bug 1251600 Opened 4 years ago Closed 4 years ago

test_page_info_window.py doesn't run test for opening/closing the window via menu

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set

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.
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
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.
Thanks for the feedback, Henrik. I'll change the usage to self.platform and submit a patch this evening.
Attached is a patch I would like to submit for review. I tried running the tests with mach.
(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.
I submitted bug 1253186 to issue a warning when clients attempt to use the platform capability.
(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
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.
(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.
(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?
Attachment #8726067 - Attachment is obsolete: true
Attachment #8726199 - Attachment is obsolete: true
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)
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)
(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.
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)
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)
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.
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.
With respect to the NT test - would any windows build be sufficient?
hello henrik, I was wondering if I could get an update on what do to next or on reviewboard?

Thanks,
Zach
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.
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.
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+
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.
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+
(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.
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)
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)
I changed the commit message, but the review title didn't update. Do I need to create a new review entirely?
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.
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?
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.
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+
https://hg.mozilla.org/mozilla-central/rev/c0fa17b1195d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1271917
You need to log in before you can comment on or make changes to this bug.