1157319 - Add a "Request Desktop Site" Robocop test

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 3

[u421692]

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.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

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

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 9

[Nick Alexander :nalexander [he/him]]

Mihai: any movement here?

Comment 10

[u421692]

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.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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?

Comment 12

[Amin Bandali [:bandali]]

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.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 14

[Nick Alexander :nalexander [he/him]]

I think mfinkle's comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1157319#c6 is most relevant.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

This broke again. Taking and working on a patch.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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/

Comment 18

[Nick Alexander :nalexander [he/him]]

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.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 20

[:Margaret Leibovic]

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.

Comment 21

[:Margaret Leibovic]

(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?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

THOU SHALL NOT FAIL... again

https://hg.mozilla.org/integration/fx-team/rev/c4a1f1188677

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4a1f1188677