Add a "Request Desktop Site" Robocop test

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: nalexander, Assigned: mfinkle, Mentored)

Tracking

Trunk
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox42 fixed)

Details

(Whiteboard: [lang=js,java,html][good next bug])

Attachments

(1 attachment, 1 obsolete attachment)

This is follow-up to Bug 1154960 and Bug 1151469.  We should verify that Request Desktop Site is doing the right thing for mobile/android.  Since there's Java UI involved, we probably want to do this using a Robocop test to actually toggle the switch.

A first cut of this test would:
* load a page that reflects the User Agent string back to the browser;
* verify that the User Agent string is correct;
* toggle the Request Desktop Site setting on;
* possibly reload the page -- although I think RDS should do the right thing and request the reload automatically.  Worth being clear here.
* verify that the page has reloaded and that the User Agent string is updated.
* toggle the Request Desktop Site setting off;
* verify that the page is reloaded and that the User Agent string is returned to its original form.
(Reporter)

Comment 1

4 years ago
mcomella: you know about the Robocop tests more than I do: do we already have such a test?  I see a string helper that's similar but not actually used, AFAICT: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java#187
Flags: needinfo?(michael.l.comella)
(In reply to Nick Alexander :nalexander from comment #1)
> mcomella: you know about the Robocop tests more than I do: do we already
> have such a test?

I agree DESKTOP_SITE_LABEL is unused and I do not see anything that resembles such a test in our list of tests in robocop.ini.
Flags: needinfo?(michael.l.comella)

Comment 3

4 years ago
I would like to take a look at this test if it's ok.
I will try to upload a patch a soon as possible to get an early feedback so I don't keep this bug blocked.
Sure, Mihai - go for it!

I'm not sure what you have set up locally, but you'll need a build (https://wiki.mozilla.org/Mobile/Fennec/Android) and robocop set up (https://wiki.mozilla.org/Auto-tools/Projects/Robocop).

When you make changes in the mobile/android/base/tests/ directory, you should only need to recompile Robocop, not the entire browser, for these changes.

New tests should use the UITest framework, see [1] for some help on writing good UITests. It might be sufficient to just follow other tests though (e.g. [2]).

You can reply here if you have any questions, or find us in #mobile.

Thanks!

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android/UITest
[2]: https://mxr.mozilla.org/mozilla-central/search?string=extends+UITest&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Assignee: nobody → mihai.g.pop
(Reporter)

Comment 5

4 years ago
Here's some detail on how I think this should be done:

There are two User-Agent strings we care about: the HTTP header and the one exposed in the browser.  The former matters more than the latter, since that's what sites will sniff.  Your test page should be a copy of [1], which will dynamically return the user agent header in the page body.  See [2] for details on SJS files.  You could also include a <script> tag which inserts the value of navigator.userAgent in the page body [3].  Those two should be the same (I think?) and should be changed by the Request Desktop Site preference.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/test/mochitests/user_agent.sjs

[2] https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F

[3] https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent
I think it would be simple to create a JavascriptTest based test for this. We really want to test the HTTP header and the navigator.userAgent are changing when we switch to desktop site mode. We should be able to "trick" the browser into switching modes by just manually sending the message like we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#3273

Actually, it would be simpler to just create the tab and pass in the { desktopMode: true } option.

There are several examples in the tree of JavascriptTests that open a tab and load a webpage. At a minimum, we should check the browser.contentWindow.navigator.userAgent for the expected value when in desktop mode.

Maybe testVideoControls.js shows the pattern well enough:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testVideoControls.js

Create the tab at line 39, passing the desired value for the desktopMode option:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testVideoControls.js#39

You could even load the same page, or a blank page. Then, at line 44, you just test the browser.contentWindow.navigator.userAgent and make sure it's the expected value. If you only test one situation, you're done. Or you could add a second add_test method and open a tab with the opposite desktop mode value.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testVideoControls.js#44
I think there's some benefit to using the user pathway to open a link (i.e. actually clicking the button in the toolbar menu) but perhaps this could be a different test? It could fit into testAppMenuPathways.
(In reply to Michael Comella (:mcomella) from comment #7)
> I think there's some benefit to using the user pathway to open a link (i.e.
> actually clicking the button in the toolbar menu) but perhaps this could be
> a different test? It could fit into testAppMenuPathways.

I'm not sold on using the full user flow for tests. Keep it simple.
(Reporter)

Comment 9

4 years ago
Mihai: any movement here?
Flags: needinfo?(mihai.g.pop)

Comment 10

4 years ago
Sorry for keeping this blocked, did not have the time to work on this.
Now, after I've updated the sources, I can't run any tests.
Please feel free to assign this bug.
Assignee: mihai.g.pop → nobody
Flags: needinfo?(mihai.g.pop)
(In reply to Mihai Pop from comment #10)
> Now, after I've updated the sources, I can't run any tests.

A common issue when updating sources is that the robocop APK doesn't get updated so you'll usually have to uninstall it first: `adb uninstall org.mozilla.roboexample.test` Have you tried that?
(Reporter)

Updated

4 years ago
Blocks: 1167208
I might be able to help with this issue.

Nick mentioned on #mobile that there are some instructions available on https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop.
Flags: needinfo?(nalexander)
(In reply to Amin Bandali (:aminb) from comment #12)
> I might be able to help with this issue.

Sure - you've been assigned!

> Nick mentioned on #mobile that there are some instructions available on
> https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop.

I made some more notes in comment 4 for Robocop suggestions.
Assignee: nobody → me
(Reporter)

Comment 14

4 years ago
I think mfinkle's comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1157319#c6 is most relevant.
Flags: needinfo?(nalexander)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1178798
This broke again. Taking and working on a patch.
Assignee: me → mark.finkle
Created attachment 8628059 [details] [diff] [review]
desktopmode-test v0.1

This is a standard JavaScriptTest based test. It opens two tabs:
* One is forced into desktopMode
* One is a normal mobile tab

Each tab loads an SJS script that echos the HTTP request "User-Agent" header back at us in the HTML body of the resultant page. The test will check for the correct UA in the HTTP header (body.innerHTML) and the window.navigator.userAgent property.

Currently, the DesktopMode UA is broken (bug 1178761) and this patch catches it locally. I also applied the fix in bug 1178761 locally, and this test then passes.

\o/
Attachment #8628059 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 18

3 years ago
Comment on attachment 8628059 [details] [diff] [review]
desktopmode-test v0.1

Review of attachment 8628059 [details] [diff] [review]:
-----------------------------------------------------------------

nits and one real concern.  Nifty!  margaret should look at this too.

::: mobile/android/tests/browser/robocop/desktopmode_user_agent.sjs
@@ +1,3 @@
> +function handleRequest(request, response)
> +{
> +  // avoid confusing cache behaviors

nit: prefer full sentences.

::: mobile/android/tests/browser/robocop/testDesktopUserAgent.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

nit: license?

::: mobile/android/tests/browser/robocop/testDesktopUserAgent.js
@@ +35,5 @@
> +  });
> +}
> +
> +// Load a custom sjs script that echos our "User-Agent" header back at us
> +const kUniqueURI = Services.io.newURI("http://mochi.test:8888/tests/robocop/desktopmode_user_agent.sjs", null, null);

k?  I know it's copy paste, but let's kill the prefix.

@@ +44,5 @@
> +
> +  let mobileBrowser;
> +  let desktopBrowser;
> +
> +  do_register_cleanup(function cleanup() {

I don't think these get called.  capella knows more: https://bugzilla.mozilla.org/show_bug.cgi?id=1165556.

You don't need to clean tabs in Robocop tests since the browser gets killed between tests.
Attachment #8628059 - Flags: review+
Created attachment 8628066 [details] [diff] [review]
desktopmode-test v0.2

1. Added license
2. kUniqueURL -> TestURL
3. Removed the cleanup method, although I felt strangely sad doing so. I guess they aren't needed though.
Attachment #8628059 - Attachment is obsolete: true
Attachment #8628059 - Flags: review?(margaret.leibovic)
Attachment #8628066 - Flags: review?(margaret.leibovic)

Comment 20

3 years ago
Comment on attachment 8628066 [details] [diff] [review]
desktopmode-test v0.2

Review of attachment 8628066 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet patch.

::: mobile/android/tests/browser/robocop/testDesktopUserAgent.js
@@ +32,5 @@
> +
> +    browser.addEventListener(eventType, handle, true);
> +    do_print("Now waiting for " + eventType + " event from browser");
> +  });
> +}

Reminds me that I want us to fix bug 1158925.

@@ +35,5 @@
> +  });
> +}
> +
> +// Load a custom sjs script that echos our "User-Agent" header back at us
> +const TestURI = Services.io.newURI("http://mochi.test:8888/tests/robocop/desktopmode_user_agent.sjs", null, null);

Neat hack.
Attachment #8628066 - Flags: review?(margaret.leibovic) → review+

Comment 21

3 years ago
(In reply to Mark Finkle (:mfinkle) from comment #19)
> Created attachment 8628066 [details] [diff] [review]
> desktopmode-test v0.2
> 
> 1. Added license

I don't want to be a license pedant, but isn't our test code supposed to be public domain, not MPL?
(In reply to :Margaret Leibovic from comment #21)
> (In reply to Mark Finkle (:mfinkle) from comment #19)
> > Created attachment 8628066 [details] [diff] [review]
> > desktopmode-test v0.2
> > 
> > 1. Added license
> 
> I don't want to be a license pedant, but isn't our test code supposed to be
> public domain, not MPL?

No idea, but we have some changes to make if that's the case.
https://hg.mozilla.org/mozilla-central/rev/c4a1f1188677
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.