1021060 - Testing framework for requestAutocomplete on Desktop

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[:Paolo Amadini]

We'll have xpcshell, mochitest-chrome, and browser-chrome tests for the requestAutocomplete feature. We'll probably want to share some logic and provide support for Electrolysis, which is not currently present in Form Manager tests.

All the frameworks will require various functions that should be shared:
* Initialization in the parent process
* Initialization in the content process
* Common utility functions for the parent and/or content process
* Execution of the test in either the parent or content process
* Asynchronous execution of all the tests (add_task)
* Termination matching initialization

This bug is about ensuring that these points are taken into account, and most of the testing code is actually shared.

Comment 1

[:Paolo Amadini]

Attachment: Preliminary work

This is some preliminary work aimed at obtaining a consistent testing framework where we can share our initialization, termination, checks, and utility functions.

Comment 2

[:Paolo Amadini]

To build the patch and run the example tests:

./mach build toolkit/components/formautofill/
./mach xpcshell-test toolkit/components/formautofill/test/
./mach mochitest-chrome toolkit/components/formautofill/test/
./mach mochitest-browser toolkit/components/formautofill/test/

Comment 3

[:Paolo Amadini]

Attachment: Refined version

This version implements the utility functions and adds tests for them. These shared functions have been documented and moved to the TestUtils object.

It also adds the "add_termination_task" function to the three types of tests.

I think the three infrastructure test files can now be kept in the tree after this patch lands, as examples and tests for the infrastructure itself. When bug 946708 is addressed, the tests can be moved to a shared location.

This is ready for review so that we can use it as the base for implementing the actual feature tests.

Comment 4

[:Paolo Amadini]

Comment on attachment 8436872 [details] [diff] [review]
Refined version

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

::: toolkit/components/formautofill/test/xpcshell/head.js
@@ +32,5 @@
> +function waitForTime(aDelay) {
> +  let deferred = promise.defer();
> +  setTimeout(deferred.resolve, aDelay);
> +  return deferred.promise;
> +}

Hm, this is a leftover from cut-and-paste. I've already removed it locally.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8436872 [details] [diff] [review]
Refined version

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

Delete the mode lines in the JS files (or at least switch them to JS).

Overall this is fine but I had enough comments/questions that I may as well look at your answers quickly in your new revision. I'll attach a patch with many of the fixes completed.

Thanks for setting this all up!

::: toolkit/components/formautofill/test/browser.ini
@@ +1,2 @@
> +[DEFAULT]
> +support-files = head_common.js

Delete this file

::: toolkit/components/formautofill/test/browser/browser.ini
@@ +1,2 @@
> +[DEFAULT]
> +support-files = head.js

Add ../head_common.js

::: toolkit/components/formautofill/test/browser/head.js
@@ +30,5 @@
> +    let stack = Components.stack.caller;
> +    ok(actual, "[" + stack.name + " : " + stack.lineNumber + "] " + actual +
> +               " == true");
> +  },
> +  equal: function (actual, expected) {

Note: It sounds like we're going to rename equal to |is| soon to match SimpleTest IIUC so that may be a problem.

@@ +42,5 @@
> +//// Shared infrastructure
> +
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/" +
> +  "toolkit/components/formautofill/test/head_common.js", this);

Add "browser/" before "head_common.js" after the manifest change

::: toolkit/components/formautofill/test/chrome.ini
@@ +1,2 @@
> +[DEFAULT]
> +support-files = head_common.js

Delete this file

::: toolkit/components/formautofill/test/chrome/chrome.ini
@@ +1,2 @@
> +[DEFAULT]
> +support-files = head.js

Add ../head_common.js so you can delete ../chrome.ini

::: toolkit/components/formautofill/test/chrome/head.js
@@ +73,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// Shared infrastructure
> +
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/" +

"/browser/" should be "/chrome/" here otherwise you're referring to the b-c version

::: toolkit/components/formautofill/test/chrome/test_infrastructure.html
@@ +1,1 @@
> +<!DOCTYPE HTML><html><head><meta charset="utf-8"></head><body>

I'm not used to the lack of indentation but I suppose it makes sense to keep the focus on the test itself.

Nit: lowercase "HTML" is preferred: http://www.w3.org/TR/html-polyglot/#doctype

::: toolkit/components/formautofill/test/head_common.js
@@ +161,5 @@
> +    this._fileCounter++;
> +
> +    // Get a file reference under the temporary directory for this test file.
> +    let path = OS.Path.join(OS.Constants.Path.tmpDir, leafName);
> +    Assert.ok(!(yield OS.File.exists(path)));

Can this method use OS.File.openUnique? It feels weird to depend on DownloadPaths and here and it seems like this functionality should be upstreamed like bug 946708 plans to do.

Did you have ideas in mind of why we would need temporary files for our tests? If not, I'd rather not add all the dependencies for something we may not use.

@@ +182,5 @@
> +{
> +  // We must manually enable the feature while testing.
> +  Services.prefs.setBoolPref("dom.forms.requestAutocomplete", true);
> +  add_termination_task(function* () {
> +    Services.prefs.clearUserPref("dom.forms.requestAutocomplete");

Is this better than pushPrefEnv or is that not available in all 3 suites?

::: toolkit/components/formautofill/test/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +BROWSER_CHROME_MANIFESTS += [
> +    'browser.ini',

Delete the ini entries for the files in this directory and you could fold this into ../moz.build as I believe it's preferred to keep moz.build files flatter.

::: toolkit/components/formautofill/test/xpcshell.ini
@@ +1,2 @@
> +[DEFAULT]
> +support-files = head_common.js

Delete this file

::: toolkit/components/formautofill/test/xpcshell/head.js
@@ +35,5 @@
> +  return deferred.promise;
> +}
> +
> +function run_test()
> +{

Nit: brace on the same line as the function name throughout the patch

@@ +45,5 @@
> +//// Shared infrastructure
> +
> +let (commonFile = do_get_file("../head_common.js", false)) {
> +  let uri = Services.io.newFileURI(commonFile);
> +  Services.scriptloader.loadSubScript(uri.spec, this);

Not necessary anymore as this can be done in the manifest

::: toolkit/components/formautofill/test/xpcshell/xpcshell.ini
@@ +1,2 @@
> +[DEFAULT]
> +head = head.js

Add ../head_common.js after head.js here

Comment 6

[Matthew N. [:MattN]]

Attachment: Patch on top of Paolo's with many review comments addressed

Comment 7

[Matthew N. [:MattN]]

Attachment: Patch on top of Paolo's with many review comments addressed v.2

Oops, you can't use a new line for the head line of xpcshell.ini

Comment 8

[Matthew N. [:MattN]]

Also, as I said in bug 1023484 a minute ago, I thought we decided we want mochitest-plain instead of mochitest-chrome since the latter doesn't run on Android/B2G IIUC[1] but I could be wrong. I was thinking that mochitest-plain would test the web-developer facing side of the feature. e.g. the various events and that the fields are filled appropriately.

[1] "Mochitest-chrome and browser-chrome do not run on Android." from https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Testing_Mobile.2FAndroid.3F

Comment 9

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #5)
> > +  equal: function (actual, expected) {
> 
> Note: It sounds like we're going to rename equal to |is| soon to match
> SimpleTest IIUC so that may be a problem.

Renaming will definitely be easy.

My primary concern is that the names are too similar, but we can just make sure that the test fails in case Assert.equal(1 + 1, 2) is renamed to Assert.ok(1 + 1, 2).

> ::: toolkit/components/formautofill/test/head_common.js
> @@ +161,5 @@
> > +    this._fileCounter++;
> > +
> > +    // Get a file reference under the temporary directory for this test file.
> > +    let path = OS.Path.join(OS.Constants.Path.tmpDir, leafName);
> > +    Assert.ok(!(yield OS.File.exists(path)));
> 
> Can this method use OS.File.openUnique? It feels weird to depend on
> DownloadPaths and here and it seems like this functionality should be
> upstreamed like bug 946708 plans to do.

OS.File.openUnique creates the file, but when you need to pass the path of a non-existing file to, say, the JSON storage module, if you created and deleted it immediately on Windows you're subject to a subtle race condition that originates intermittent failures.

> Did you have ideas in mind of why we would need temporary files for our
> tests? If not, I'd rather not add all the dependencies for something we may
> not use.

Storage recovery tests generally need this, but we can remove the function and add it back when it is needed if you prefer.

> > +  Services.prefs.setBoolPref("dom.forms.requestAutocomplete", true);
> > +  add_termination_task(function* () {
> > +    Services.prefs.clearUserPref("dom.forms.requestAutocomplete");
> 
> Is this better than pushPrefEnv or is that not available in all 3 suites?

I believe this is not available in xpcshell, since it's a SpecialPowers function.

Comment 10

[:Paolo Amadini]

Attachment: Folded patch

Thanks a lot for the patch, it saved a lot of work!

Comment 11

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #8)
> I was thinking that
> mochitest-plain would test the web-developer facing side of the feature.
> e.g. the various events and that the fields are filled appropriately.

Yeah, we need to figure out how to make that work with the shared functions.

Comment 12

[:Paolo Amadini]

OK, I did some research, and it seems that if we want the tests to run on Android and B2G we'll definitely need some more effort to support e10s, even though Android does not use it. Tests are also triggered on remote machines, from what I understand by reading bug 797164, at least on B2G.

In order to unblock the landing of the first tests in bug 1023484 and the rest of the feature development, I suggest landing this Desktop patch first and converting mochitest-chrome to mochitest-plain with e10s support in bug 1023862.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8438360 [details] [diff] [review]
Folded patch

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

I know Brian doesn't like the //////////////////////////////////////////////////////////////////////////////// style and I also find it foreign. It's also more work to create so perhaps we can stick with JavaDoc style? "/**"

I would prefer we wait on making the chrome directory and just write mochitest-plain tests (without all of the helpers for now) as we plan to do as I'd like to have coverage for e10s and Android from the beginning.

::: toolkit/components/formautofill/test/browser/browser_infrastructure.js
@@ +7,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Tests

I don't think this heading is necessary here or the other tests

::: toolkit/components/formautofill/test/browser/head.js
@@ +9,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

Drop the heading?

::: toolkit/components/formautofill/test/chrome/head.js
@@ +9,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

Drop the heading?

::: toolkit/components/formautofill/test/head_common.js
@@ +7,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

Drop the heading?

@@ +60,5 @@
> +   * @return {Promise}
> +   * @resolves When pending events have been processed.
> +   * @rejects Never.
> +   */
> +  waitForTick: function () {

Remove space after |function| throughout the patch

@@ +171,5 @@
> +//// Initialization and termination functions common to all tests
> +
> +add_task(function* test_common_initialize() {
> +  // We must manually enable the feature while testing.
> +  Services.prefs.setBoolPref("dom.forms.requestAutocomplete", true);

You forgot to also flip dom.forms.autocomplete.experimental

::: toolkit/components/formautofill/test/xpcshell/head.js
@@ +9,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

Drop the heading?

Comment 14

[:Paolo Amadini]

Attachment: Attempt to use Task in mochitest-plain

For simplicity I've not renamed files or folders, but here is the relevant portion of the output when I launch this updated test as mochitest-plain:

 0:14.30 [4244] WARNING: Denied access to property |value| on Xrayed Object: Value not same-origin with target (@resource://gre/modules/Task.jsm:290): file /Users/paolo/m-c/js/xpconnect/wrappers/XrayWrapper.cpp, line 421
 0:14.30 *************************
 0:14.30 A coding exception was thrown and uncaught in a Task.
 0:14.30 Full message: TypeError: errorEvent is undefined
 0:14.30 Full stack: test_disabled_globally@http://mochi.test:8888/tests/toolkit/components/formautofill/test/chrome/test_requestAutocomplete_disabled.html:30:3
 0:14.30 onLoad/<@http://mochi.test:8888/tests/toolkit/components/formautofill/test/chrome/head.js:56:9
 0:14.30 onLoad@http://mochi.test:8888/tests/toolkit/components/formautofill/test/chrome/head.js:52:3

Comment 15

[Matthew N. [:MattN]]

Hey Bobby,

We have some mochitests that need chrome privileges (e.g. for Task.jsm and some setup in the browser chrome) but we'd like them to also run on Android/e10s/B2G where mochitest-chrome doesn't. Is there something like enablePrivilege we can use without big modifications to toolkit modules like Task.jsm? See comment 14 for an example failure.

Thanks

Comment 16

[Bobby Holley (:bholley)]

(In reply to Matthew N. [:MattN] from comment #15)
> We have some mochitests that need chrome privileges (e.g. for Task.jsm and
> some setup in the browser chrome)

How do you do "setup in the browser chrome" on platforms like Android where there's no such thing as browser chrome?

> Is there something like enablePrivilege we can use without big modifications to toolkit modules like
> Task.jsm? See comment 14 for an example failure.

Fundamentally, mochitests are supposed to be running in the same environment as web content. We have some hacks with SpecialPowers to occasionally twiddle some knobs, but they're generally a best-effort sort of thing, and completely unsupported as a general-purpose tool.

The right solution is just to make mochitest-chrome (or some subset of it) run on these platforms. Unlike mochitest-browser, mochitest-chrome is the same thing as mochitest-plain except the pages are loaded as privileged. The pages don't have to be XUL, and there's no technical reason why this can't happen on platforms like Android and B2G.

Comment 17

[:Paolo Amadini]

(In reply to Bobby Holley (:bholley) from comment #16)
> (In reply to Matthew N. [:MattN] from comment #15)
> > We have some mochitests that need chrome privileges (e.g. for Task.jsm and
> > some setup in the browser chrome)
> 
> How do you do "setup in the browser chrome" on platforms like Android where
> there's no such thing as browser chrome?

What we need to do is to run some code with chrome privileges that sets up mock UI objects and expected responses before each test case. This way we can provide mock UI responses, and have the API behavior tested consistently without any platform-specific UI actually showing.

Since API behavior and related setup can become quite complex, we would like to share the mock setup code between all our frameworks, and not make some ad-hoc functions (like adding new SpecialPowers extensions) that do the same thing but work differently in mochitest-plain.

Having the same API tests in all platforms allows developers to catch regressions soon, compared to having tests only in Desktop which would need Android developers to check their code on a different platform too.

The APIs work together with web content in windows, thus we can't just use xpcshell for those back-end tests.

> > Is there something like enablePrivilege we can use without big modifications to toolkit modules like
> > Task.jsm? See comment 14 for an example failure.
> 
> Fundamentally, mochitests are supposed to be running in the same environment
> as web content. We have some hacks with SpecialPowers to occasionally
> twiddle some knobs, but they're generally a best-effort sort of thing, and
> completely unsupported as a general-purpose tool.

I don't think "occasionally" reflects reality... but that's a separate discussion.

> The right solution is just to make mochitest-chrome (or some subset of it)
> run on these platforms. Unlike mochitest-browser, mochitest-chrome is the
> same thing as mochitest-plain except the pages are loaded as privileged. The
> pages don't have to be XUL, and there's no technical reason why this can't
> happen on platforms like Android and B2G.

I agree, though from what I read in bug 797164, enabling mochitest-chrome on other platforms seems to be quite a lot of work.

Is there some shortcut we can take here to achieve the same effect? We're still in the process of prototyping the feature, but "real" work will start in a week or so, and I'd like to have something for consistent back-end testing by then.

If you think there are ways to run our subset of mochitest-chrome on Android only, without this taking months, we can also go ahead with our current mochitest-chrome infrastructure on Desktop, and extend to Android as soon as support for it is ready.

Comment 18

[Bobby Holley (:bholley)]

In general, I think we very much need to be able to run privileged stuff on all platforms without resorting to SpecialPowers craziness. SpecialPowers wrappers are black magic, and the more we rely on them for complicated things, the more we're likely to have pain in the long run.

Specifically, trying to use intricate things like Task.jsm via SpecialPowers.wrap() is destined for failure. The wrappers are going to spread all over the place and violate assumptions due to the fact that they aren't a fully-transparent abstraction (see things like bug 718543).

So I've re-opened bug 797164.

In the mean time though, you'll probably need to use/add functionality to SimpleTest/SpecialPowers to get where you need to go. Can you use something like MockObjects.js?

Comment 19

[:Paolo Amadini]

(In reply to Bobby Holley (:bholley) from comment #18)
> Can you use something like MockObjects.js?

No, that doesn't work for us. As you've said, we'd need to implement yet another special case in SpecialPowers, which doesn't seem scalable even if requestAutocomplete was a small project.

Comment 20

[:Paolo Amadini]

We'll need to run mochitest-chrome files on Android. Nick, any idea of how we could do that? Is there a bug on file already?

In case achieving mochitest-chrome execution is too complicated, is there a way to wrap a test page with chrome privileges inside a mochitest-plain, maybe in an iframe that loads the test from a "chrome" URL?

Comment 21

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #20)
> We'll need to run mochitest-chrome files on Android. Nick, any idea of how
> we could do that? Is there a bug on file already?

I filed bug 1026290 yesterday.

Comment 22

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #21)
> I filed bug 1026290 yesterday.

Cool, thanks! I'm leaving the needinfo for Nick on this bug open, about the possible "wrapping" in mochitest-plain, which might not be needed if bug 1026290 is easy enough.

While we don't have conclusive data to tell whether we'll use chrome or plain mochitests eventually, it seems clear to me that we should land our mochitest-chrome tests here, to have a base for the work in bug 1026290, and correct the course in bug 1023862 if needed.

Comment 23

[:Paolo Amadini]

Attachment: Adjusted patch

I had to re-add an additional xpcshell.ini in the "test" folder, otherwise the head_common.js file wouldn't be copied in a clobber build (the "../" reference doesn't work in "support-files"). Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=0790f49a917b

Comment 24

[:Paolo Amadini]

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

Comment 25

[Nick Alexander :nalexander [he/him]]

Sorry for the delayed reply.

(In reply to :Paolo Amadini from comment #20)
> We'll need to run mochitest-chrome files on Android. Nick, any idea of how
> we could do that? Is there a bug on file already?

I'm not sure.  I'm going to redirect to gbrown, who is the most likely to know.

> In case achieving mochitest-chrome execution is too complicated, is there a
> way to wrap a test page with chrome privileges inside a mochitest-plain,
> maybe in an iframe that loads the test from a "chrome" URL?

We do something like this in our Robocop JavascriptTest framework.  The result is an HTML page loaded with a script running in a (sandboxed) chrome JS context.  Said script has access, IIRC, to Components.  At the time, I expected to use SpecialPowers for this, but found it didn't naturally map to my use case.

We also do something like this using our "RobocopExtender" add-on, which provides chrome:// content and scripts to our Robocop tests.  wesj is the expert here, but I don't think it's terribly complicated -- it's just an add-on loaded as part of the Robocop test profile.

Both of these approaches serve the same function as mochitest-chrome, I think, but look rather different when implemented.  I have nothing for or against mochitest-chrome; gbrown will have better context and more meaningful opinions here.  I will say that most Fennec front-end tests require both a chrome JS context and Java to meaningfully test things, so in my opinion, mochitest-chrome provides relatively little value for Fennec front-end devs.

Comment 26

[Geoff Brown [:gbrown]]

Sorry, I don't have much to add. Note that there was a recent discussion about mochitest-chrome at https://groups.google.com/forum/#!topic/mozilla.dev.platform/AkX7jmLRQwg. It looks like the right people are cc'd in bug 1026290.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84f04ef3add0