1149276 - Use Assert instead of do_check_ functions in xpcshell tests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

Assert allows logging strings for what is being checked so it is easier to figure out via the logs what is going on.

Also, at least some of the time it would be better to fail the test instead of calling do_throw to differentiate what is failing per bug 1148775.

Comment 1

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

Also fix
> ::: toolkit/mozapps/update/tests/unit_aus_update/uiAutoPref.js
> @@ +87,5 @@
> > +    return { getInterface: XPCOMUtils.generateQI([Ci.nsIDOMWindow]) };
> > +  },
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWindowMediator])
> > +}
> 
> nit: add ';' after '}'

Comment 2

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

Attachment: patch part 1 rev1

I'm going to split this into two parts and get a review now in case the messages used should be changed.

Comment 3

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

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f4453bb5e2

Comment 4

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

Comment on attachment 8590719 [details] [diff] [review]
patch part 1 rev1

Since there haven't been any file missing intermittent failures I'm not going to bother having the path in most of the messages for the exist tests. I'll also combine the first patch with the second patch.

Comment 5

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

Attachment: patch rev1

Stephen, no rush on this. One thing that can be a bit confusing about these tests is that the service tests have code for platforms other than Windows. It is this way so it is easy to diff the all platform tests under unit_base_updater against the service tests under unit_service_updater to make sure that they are testing the same code and expect the same results.

Comment 6

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

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64ca38779c21

Comment 7

[Stephen A Pohl [:spohl]]

Comment on attachment 8591299 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +2578,5 @@
>   *
>   * @param   aCheckString
>   *          The string to check if the update log contains.
>   */
>  function checkUpdateLogContains(aCheckString) {

I wonder if we should change the name of this function (and the description for it) to something like |ensureUpdateLogContains|. "check" (and the description for the function) seems to indicate that the function could return a true or false value, depending on whether or not the string was found. However, we clearly only use this to ensure that the string is in the log. What do you think?

@@ +2841,5 @@
>    let logContents = readFile(appLaunchLog);
>    // It is possible for the log file contents check to occur before the log file
>    // contents are completely written so wait until the contents are the expected
>    // value. If the contents are never the expected value then the test will
>    // fail by timing out.

We seem to be failing now once we've reached a number of timeout runs that's greater than MAX_TIMEOUT_RUNS, so we should change the end of this comment (this also applies to the second occurrence in this file).

@@ +2856,5 @@
> +      // xpcshell tests won't display the entire contents so log the incorrect
> +      // line.
> +      for (let i = 0; i < aryLog.length; ++i) {
> +        if (aryLog[i] != aryCompare[i]) {
> +          logTestInfo("the first incorrect line in the callback log is: " +

Is it possible to have more than one incorrect line? If so, "first" might be misleading in this log statement (this also applies to the second occurrence in this file).

Comment 8

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

(In reply to Stephen Pohl [:spohl] from comment #7)
> Comment on attachment 8591299 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8591299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
> @@ +2578,5 @@
> >   *
> >   * @param   aCheckString
> >   *          The string to check if the update log contains.
> >   */
> >  function checkUpdateLogContains(aCheckString) {
> 
> I wonder if we should change the name of this function (and the description
> for it) to something like |ensureUpdateLogContains|. "check" (and the
> description for the function) seems to indicate that the function could
> return a true or false value, depending on whether or not the string was
> found. However, we clearly only use this to ensure that the string is in the
> log. What do you think?
I tend to use the prefix "is" for function names with a boolean return.

If it were to change I'd prefer going with the prefix "test" though in the context of test code I consider the "check" prefix to be a test check.

> @@ +2841,5 @@
> >    let logContents = readFile(appLaunchLog);
> >    // It is possible for the log file contents check to occur before the log file
> >    // contents are completely written so wait until the contents are the expected
> >    // value. If the contents are never the expected value then the test will
> >    // fail by timing out.
> 
> We seem to be failing now once we've reached a number of timeout runs that's
> greater than MAX_TIMEOUT_RUNS, so we should change the end of this comment
> (this also applies to the second occurrence in this file).
I can clarify this.

> 
> @@ +2856,5 @@
> > +      // xpcshell tests won't display the entire contents so log the incorrect
> > +      // line.
> > +      for (let i = 0; i < aryLog.length; ++i) {
> > +        if (aryLog[i] != aryCompare[i]) {
> > +          logTestInfo("the first incorrect line in the callback log is: " +
> 
> Is it possible to have more than one incorrect line? If so, "first" might be
> misleading in this log statement (this also applies to the second occurrence
> in this file).
It definitely is possible which is why the log says "the first incorrect line in the callback log is:". I think that implies it is only reporting the first incorrect line and that other incorrect lines won't be reported. Would l=you like me to add a disclaimer along the lines of other incorrect lines aren't reported?

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> (In reply to Stephen Pohl [:spohl] from comment #7)
> > @@ +2841,5 @@
> > >    let logContents = readFile(appLaunchLog);
> > >    // It is possible for the log file contents check to occur before the log file
> > >    // contents are completely written so wait until the contents are the expected
> > >    // value. If the contents are never the expected value then the test will
> > >    // fail by timing out.
> > 
> > We seem to be failing now once we've reached a number of timeout runs that's
> > greater than MAX_TIMEOUT_RUNS, so we should change the end of this comment
> > (this also applies to the second occurrence in this file).
> I can clarify this.

Talked on irc. Would be great if we could adjust the comment here, but everything else looks fine. Thanks!

Comment 10

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

Attachment: patch - updated to comments

Comment 11

[Pulsebot]

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

Comment 12

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/7601bd7dc9b3

Comment 13

[Pulsebot]

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

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7601bd7dc9b3