566267 - Simplify mochitest-chrome tests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

I decided to make it simpler to add new app update mochitest-chrome tests by making the code more general since I have a couple of new ones to add in the near future.

Comment 1

[Robert Strong (they/them - no direct email)]

I want this to land before the new app update tests for bug 544442 so adding dependency

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch - almost done

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

I am also going to rename several of the tests in a second patch

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: rename test files (checked in)

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 446103 [details] [diff] [review]
patch rev2

First pass, everything except the xul test files.

I think it would be worth adding a comment explaining what properties test objects can have and what they do.

>diff --git a/toolkit/mozapps/update/test/chrome/utils.js b/toolkit/mozapps/update/test/chrome/utils.js

>- * the provisions above, a recipient may use your version of this file under
>- * the terms of any one of the MPL, the GPL or the LGPL.
>- *
>- * ***** END LICENSE BLOCK *****
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/

Note that to do this you have to have the permission of everyone who has ever contributed code to the test files. You have mine, are there any others?

>+function debugDump(msg) {
>+  if (DEBUG_DUMP) {
>+    dump("*** " + msg + "\n");
>+  }
> }

In some ways I wonder if you might as well just turn this on, there doesn't seem to be a lot of cost and it'll vastly help if any intermittent failures crop up.

>+/**
>+ * Default test run function that can be used by most tests. This will close an
>+ * existing Update Window if it is found before continuing.
>+ */
>+function runTestDefault() {
>+  debugDump("Entering runTestDefault");
>+
>+  SimpleTest.waitForExplicitFinish();
>+
>+  if (closeUpdateWindow()) {
>+    // Give the Update Window time to close before continuing.
>+    setTimeout(continueRunTestDefault, 1000);
>+    return;

Could we use gWindowObserver to catch this rather than an arbitrary wait time. Is it an error for a window to be left over from a previous test?

>     win.addEventListener("load", function onLoad() {
>-      if (win.location != URI_UPDATE_PROMPT_DIALOG)
>+      // Defensive measure to prevent windows we shouldn't see from breaking
>+      // the test.
>+      if (win.location != URI_UPDATE_PROMPT_DIALOG) {
>+        // This should never happen.
>+        ok(false, "Unexpected load event - win.location = " + location +
>+           "... returning early");
>         return;
>+      }
> 
>+      try {
>+        // Defensive measure to prevent wizard pages and windows we shouldn't
>+        // see from breaking the test.
>+        if (win.document.documentElement.currentPage.pageid != PAGEID_DUMMY) {
>+          // This should never happen.
>+          ok(false, "Unexpected load event - pageid != dummy... " +
>+             "returning early");
>+          return;
>+        }
>+      }
>+      catch (e) {
>+        // This should never happen.
>+        ok(false, "Unexpected load event... returning early");
>+        return;
>+      }

What exceptions are you expecting here? Maybe include the exception in the message.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> (From update of attachment 446103 [details] [diff] [review])
> First pass, everything except the xul test files.
> 
> I think it would be worth adding a comment explaining what properties test
> objects can have and what they do.
Agreed... I'll do this tonight

> >diff --git a/toolkit/mozapps/update/test/chrome/utils.js b/toolkit/mozapps/update/test/chrome/utils.js
> 
> >- * the provisions above, a recipient may use your version of this file under
> >- * the terms of any one of the MPL, the GPL or the LGPL.
> >- *
> >- * ***** END LICENSE BLOCK *****
> >+/* Any copyright is dedicated to the Public Domain.
> >+ * http://creativecommons.org/publicdomain/zero/1.0/
> 
> Note that to do this you have to have the permission of everyone who has ever
> contributed code to the test files. You have mine, are there any others?
I'm the only one too create or patch these tests so far

> >+function debugDump(msg) {
> >+  if (DEBUG_DUMP) {
> >+    dump("*** " + msg + "\n");
> >+  }
> > }
> 
> In some ways I wonder if you might as well just turn this on, there doesn't
> seem to be a lot of cost and it'll vastly help if any intermittent failures
> crop up.
These tests have been really solid so far and I've only needed to use this when writing tests so I'd prefer to only turn this on if necessary (e.g. random orange) to keep the log noise down.

> >+/**
> >+ * Default test run function that can be used by most tests. This will close an
> >+ * existing Update Window if it is found before continuing.
> >+ */
> >+function runTestDefault() {
> >+  debugDump("Entering runTestDefault");
> >+
> >+  SimpleTest.waitForExplicitFinish();
> >+
> >+  if (closeUpdateWindow()) {
> >+    // Give the Update Window time to close before continuing.
> >+    setTimeout(continueRunTestDefault, 1000);
> >+    return;
> 
> Could we use gWindowObserver to catch this rather than an arbitrary wait time.
> Is it an error for a window to be left over from a previous test?
I've tries a couple of different approaches and this is so far the most sane in that doing this in the prior test ends up reporting it in the next test. This condition has only happened when I am writing tests that fail so this should never happen in production but by having this check in production it will be much easier to diagnose problems if it should ever happen.

> 
> >     win.addEventListener("load", function onLoad() {
> >-      if (win.location != URI_UPDATE_PROMPT_DIALOG)
> >+      // Defensive measure to prevent windows we shouldn't see from breaking
> >+      // the test.
> >+      if (win.location != URI_UPDATE_PROMPT_DIALOG) {
> >+        // This should never happen.
> >+        ok(false, "Unexpected load event - win.location = " + location +
> >+           "... returning early");
> >         return;
> >+      }
> > 
> >+      try {
> >+        // Defensive measure to prevent wizard pages and windows we shouldn't
> >+        // see from breaking the test.
> >+        if (win.document.documentElement.currentPage.pageid != PAGEID_DUMMY) {
> >+          // This should never happen.
> >+          ok(false, "Unexpected load event - pageid != dummy... " +
> >+             "returning early");
> >+          return;
> >+        }
> >+      }
> >+      catch (e) {
> >+        // This should never happen.
> >+        ok(false, "Unexpected load event... returning early");
> >+        return;
> >+      }
> 
> What exceptions are you expecting here? Maybe include the exception in the
> message.
Once again, these should never happen in production but by having this check in production it will be much easier to diagnose problems if it should ever happen.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 446103 [details] [diff] [review] [details])
>...
> > >+/**
> > >+ * Default test run function that can be used by most tests. This will close an
> > >+ * existing Update Window if it is found before continuing.
> > >+ */
> > >+function runTestDefault() {
> > >+  debugDump("Entering runTestDefault");
> > >+
> > >+  SimpleTest.waitForExplicitFinish();
> > >+
> > >+  if (closeUpdateWindow()) {
> > >+    // Give the Update Window time to close before continuing.
> > >+    setTimeout(continueRunTestDefault, 1000);
> > >+    return;
> > 
> > Could we use gWindowObserver to catch this rather than an arbitrary wait time.
> > Is it an error for a window to be left over from a previous test?
> I've tries a couple of different approaches and this is so far the most sane in
> that doing this in the prior test ends up reporting it in the next test. This
> condition has only happened when I am writing tests that fail so this should
> never happen in production but by having this check in production it will be
> much easier to diagnose problems if it should ever happen.
For the time being I'm going to remove this and try to come up with a better way as time permits.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

Addresses comments... I removed the defensive check in the try catch since there is already the win.location != URI_UPDATE_PROMPT_DIALOG check so it had better damn well be a wizard. I went ahead and used a timer to make sure the test finishes within 30 seconds to prevent subsequent tests from failing which works out really well. I've also added comments for the possible test properties in utils.js.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 446109 [details] [diff] [review]
rename test files (checked in)

Pushed the rename patch to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/d23d34393be4

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 446687 [details] [diff] [review]
patch rev3

I'll resubmit after updating to include the pushed rename patch

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3 updated for file renames

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 446927 [details] [diff] [review]
patch rev3 updated for file renames

>diff --git a/toolkit/mozapps/update/test/chrome/test_0022_check_billboard_license.xul b/toolkit/mozapps

>+}, {
>+  pageid: PAGEID_LICENSE,
>+  extraCheckFunction: checkRadioGroupSelectedIndex,
>+  expectedRadioGroupSelectedIndex: 1,
>+  extraDelayedCheckFunction: checkRemoteContentState,
>+  expectedRemoteContentState: "loading",
>+  extraDelayedFinishFunction: addRemoteContentLoadListener
>+}, {
>+  pageid: PAGEID_LICENSE,
>+  extraStartFunction: waitForRemoteContentLoaded,
>+  expectedRemoteContentState: "loaded",
>+  extraCheckFunction: checkRadioGroupSelectedIndex,
>+  expectedRadioGroupSelectedIndex: 1,
>+  extraDelayedFinishFunction: addRadioGroupSelectListenerAndClick,
>+  radioClick: "accept"
>+}, {
>+  pageid: PAGEID_LICENSE,
>+  extraCheckFunction: checkRadioGroupSelectedIndex,
>+  expectedRadioGroupSelectedIndex: 0,
>+  buttonClick: "extra1"
>+}, {
>+  pageid: PAGEID_FOUND_BILLBOARD,
>+  extraCheckFunction: checkRemoteContentState,
>+  expectedRemoteContentState: "loaded",
>+  buttonClick: "next"
>+}, {
>+  pageid: PAGEID_LICENSE,
>+  extraCheckFunction: checkRadioGroupSelectedIndex,
>+  expectedRadioGroupSelectedIndex: 0,
>+  extraDelayedFinishFunction: addRadioGroupSelectListenerAndClick,
>+  radioClick: "decline"
>+}, {
>+  pageid: PAGEID_LICENSE,
>+  extraCheckFunction: checkRadioGroupSelectedIndex,
>+  expectedRadioGroupSelectedIndex: 1,
>+  extraDelayedFinishFunction: addRadioGroupSelectListenerAndClick,
>+  radioClick: "accept"

You need to check that the next button is visible but disabled at this point. Same in test_0032_available_basic_license.xul and test_0042_available_billboard_license.xul

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 446927 [details] [diff] [review]
patch rev3 updated for file renames

>diff --git a/toolkit/mozapps/update/test/chrome/utils.js b/toolkit/mozapps/update/test/chrome/utils.js
>--- a/toolkit/mozapps/update/test/chrome/utils.js
>+++ b/toolkit/mozapps/update/test/chrome/utils.js
>@@ -1,204 +1,682 @@
>...
>+ * buttonStates (optional)
>+ *   A javascript object representing the expected hidden and disabled attribute
>+ *   values for the buttons of the current wizard page. The values are checked
>+ *   in the delayedCallback function. For information about the structure of
Changed delayedCallback to delayedDefaultCallback

>+ *   this object refer to the getExpectedButtonStates and checkButtonStates
>+ *   functions.
>- * ***** END LICENSE BLOCK *****
>+ * buttonClick (optional)
>+ *   The current wizard page button to click at the end of the delayedCallback
Changed delayedCallback to delayedDefaultCallback

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 446927 [details] [diff] [review]
patch rev3 updated for file renames

The rest looks pretty good to me.

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #13)
> (From update of attachment 446927 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/update/test/chrome/test_0022_check_billboard_license.xul b/toolkit/mozapps
>...
> You need to check that the next button is visible but disabled at this point.
> Same in test_0032_available_basic_license.xul and
> test_0042_available_billboard_license.xul
The button states get checked in delayedDefaultCallback everytime including the next button being visible and disabled for that case.

Comment 17

[Robert Strong (they/them - no direct email)]

(In reply to comment #16)
> (In reply to comment #13)
> > (From update of attachment 446927 [details] [diff] [review] [details])
> > >diff --git a/toolkit/mozapps/update/test/chrome/test_0022_check_billboard_license.xul b/toolkit/mozapps
> >...
> > You need to check that the next button is visible but disabled at this point.
> > Same in test_0032_available_basic_license.xul and
> > test_0042_available_billboard_license.xul
> The button states get checked in delayedDefaultCallback everytime including the
> next button being visible and disabled for that case.
I also double checked that they were checked.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: Patch for checkin

Changes delayedCallback to delayedDefaultCallback, adds a couple of more details to the is checks, and adds a couple of debugDump messages.

Comment 19

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/efec25f5029f