785545 - unrefactor mozApps tests for maximum readability/reliability

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

The mozApps DOM API tests are aggressively refactored into shared functions that do most of the work of calling API methods and making assertions about their behavior. That makes it hard to understand and reason about the tests, even for someone who is familiar with the way they are written.

But it's especially hard for developers who are unfamiliar with the code, and test code often needs to be understood and updated by such developers, f.e. when tests break unexpectedly.

So these tests should be unrefactored for maximum readability.  In particular, API calls to the APIs being tested, and assertions about the behaviors of those calls, should be inlined into each test file/function, so the sequence of calls and assertions in a given body of testing code is as obvious as possible.

Here's a work in progress that does this unrefactoring (along with a few additional fixes to improve reliability).  The patch is mostly complete, and tests pass, but I haven't yet merged in the fix for bug 785161, and there's some cleanup to do.

This patch is best understood by reading the new versions of the files, as the patches change substantially all of the code.  But it needs to be applied to a tree that does not have the fix for bug 785161 until I update the patch to incorporate those changes.

Comment 1

[[:fabrice] Fabrice Desré]

\o/

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: unrefactors tests

Here's a version of the patch that should be ready for review.  I'll describe it and ask for review just as soon as I make sure I didn't regress anything:

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

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: works around possible race condition on Mac

Some Mac builds failed a couple of tests.  I can't reproduce locally, but after reading through the code, I suspect a race condition in the native API that _isLaunchable uses to determine whether or not an app is installed on Mac, such that _isLaunchable sometimes returns false for apps that have just been installed.

The current code sets allAppsLaunchable, which would work around such a problem, but the first version of my patch didn't do that.  This version does.  It also removes one more file that is no longer used and corrects an assertion message.

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

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: works around one more occurrence of race condition

This version of the patch works around one more (hopefully the last) occurrence of that race condition on Mac, makes test_getNotInstalled.xul more robust against a spurious and misleading failure caused by the failure of another test (such as the race condition failure), and includes a few more minor readability improvements.

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

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 656659 [details] [diff] [review]
patch v3: works around one more occurrence of race condition

The tryserver tests turned green (modulo orangefactor, as confirmed by analysis of the errors and also test reruns), so this patch is ready for review.

As I mentioned in the description of this bug, the primary goal here is to make the mozApps DOM API tests as readable as possible, so it's easier to reason about them when developers need to add more tests or the existing tests break.

The key changes are unrefactoring of all API calls and test assertions, moving them from shared utility functions into the test files/functions themselves, such that one can see exactly which API methods are called, and what behavior is expected of them, by reading the tests.

Along the way, I made a variety of other changes for readability:

* removed redundant tests
* simplified the small amount of code that remains shared and moved it from jshelper.js and apphelper.js to the single file head.js
* applied Mozilla code style conventions (per the style guide as well as my review of other mochitest files)
* made most URLs use the shortest possible origin that points to the test files (http://test) to reduce their length
* consolidated the super_crazy and wild_crazy webapps into a "basic" webapp
* replaced ok(a == b) assertions with is(a, b) for more descriptive assertion messages
* improved the descriptiveness of a variety of other assertion messages

And some for consistency/accuracy:

* moved the non-app file bug_765063.xul from webapps/apps/ up to webapps/
* renamed test_cross_domain.xul to test_cross_origin.xul
* moved/renamed the cross-origin helper page from webapps/apps/include.html to webapps/cross-origin.html
* renamed manifest_with_bom.webapp, which test_utf8.xul uses, to utf8.webapp

And a few for thoroughness:

* made the test function that compares two app objects use isDeeply() instead of comparing a fixed set of their properties
* made test_list_api.xul check all properties of the API objects, not just their methods

I also made at least one fix for reliability: the install tests were asserting that an installed app's installTime is within 3000ms of the time the assertion is made, which is a dangerous assumption, given the propensity of test machines to experience performance issues.  So I made the code instead assert that installTime is between the time the install is initiated and the time its "success" callback is called.

And I replaced blanket auto-confirmation of all installs (i.e. programmatically pressing the Install button in all install confirmation doorhangers) with a confirmNextInstall() function that confirms just a single install, to reduce the potential impact of those auto-confirmations on tests that don't intend to do installs and make it really obvious that tests are doing this.  I'm actually of two minds about this particular change, as it might be too aggressive unrefactoring.  Perhaps a single confirmInstalls() call at the top of each test file that does installs would be better.

Finally, note one fix in MochiTest itself.  As I mentioned before, I use SimpleTest's isDeeply() to compare two app objects that have "progress" properties whose values are set to NaN.  And isDeeply() was reporting that the property values differed, as it was doing a naive comparison of them, and NaN != NaN in JavaScript.  So I made it use isNaN() to compare the values correctly in that case:

+++ b/testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ -781,6 +781,9 @@ SimpleTest._deepCheck = function (e1, e2, stack, seen) {
         ok = SimpleTest._eqArray(e1, e2, stack, seen);
     } else if (typeof e1 == "object" && typeof e2 == "object") {
         ok = SimpleTest._eqAssoc(e1, e2, stack, seen);
+    } else if (typeof e1 == "number" && typeof e2 == "number"
+               && isNaN(e1) && isNaN(e2)) {
+        ok = true;
     } else {
         // If we get here, they're not the same (function references must
         // always simply reference the same function).

(Whether or not we should check the "progress" property, and if it should ever be NaN, is another question; but this change looks correct in any case, and all mochitests pass with it.)

There's more we can do, like adding tests to fill holes in test coverage and potentially converting some of the tests to mochitest-plain (content) tests (as they don't need to be mochitest-chrome tests, and mochitest-plain would better simulate the web context in which the API is used).  But the current set of changes seems like more than enough for one patch.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 656659 [details] [diff] [review]
patch v3: works around one more occurrence of race condition

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

\o/

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: resolves trivial conflicts with recent commits

This version resolves trivial conflicts with two recent commits that each modified a single test file:

http://hg.mozilla.org/mozilla-central/rev/b4eaa2264afe - bug 786296
  dom/tests/mochitest/webapps/test_install_app.xul

http://hg.mozilla.org/mozilla-central/rev/030e483f5ec1 - bug 783573
  dom/tests/mochitest/webapps/apphelper.js

Carrying forward review, and I'm building clobber now to test locally, after which I'll do one last tryserver run before pushing the patch.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Local tests look good; sent to tryserver:

https://tbpl.mozilla.org/?tree=Try&rev=2b5b3a5041df

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: resolve one more trivial merge conflict

This patch resolves one more trivial merge conflict caught by the tryserver run: a reference in the new test extensions/cookie/test/test_app_uninstall.html to the file dom/tests/mochitest/webapps/apps/super_crazy.webapp, which I replaced with basic.webapp:

diff --git a/extensions/cookie/test/test_app_uninstall.html b/extensions/cookie/test/test_app_uninstall.html
index 3f13934..18b3d58 100644
--- a/extensions/cookie/test/test_app_uninstall.html
+++ b/extensions/cookie/test/test_app_uninstall.html
@@ -73,7 +73,7 @@ SpecialPowers.setBoolPref('browser.mozApps.installer.dry_run', true);
 permManager.addFromPrincipal(window.document.nodePrincipal, "webapps-manage",
                              Ci.nsIPermissionManager.ALLOW_ACTION);
 
-var gManifestURL = "http://www.example.com:80/chrome/dom/tests/mochitest/webapps/apps/super_crazy.webapp";
+var gManifestURL = "http://www.example.com:80/chrome/dom/tests/mochitest/webapps/apps/basic.webapp";
 
 confirmNextInstall();


That was the only issue in the tryserver results, and the obvious fix passes my local tests, so I'll push this to inbound.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 658528 [details] [diff] [review]
patch v5: resolve one more trivial merge conflict

https://hg.mozilla.org/integration/mozilla-inbound/rev/57a0543b7f12

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57a0543b7f12