1149363 - For xpcshell tests instead of throwing fail the test so it is easily understood what is going on

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

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)]

Attachment: patch rev1

I will change the rest of the do_check_* calls to Assert.* calls in bug 1149276. They provide much better info in the logs so there is less ambiguity when determining failures on treeherder.

Comment 2

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

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6174f1472dc9

Comment 3

[Stephen A Pohl [:spohl]]

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

B2G seems to run into failures. Clearing r? request for now.

Comment 4

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

Try push
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7e6753c421

Comment 5

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

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

Fairly certain this try run with just this patch will be fine

Comment 6

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

Try is looking good

Comment 7

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

Try passed 100%

Comment 8

[Stephen A Pohl [:spohl]]

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

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

Looks good! r+ with questions answered/comments addressed.

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +1595,4 @@
>      if (IS_AUTHENTICODE_CHECK_ENABLED && isBinarySigned(updaterBinPath)) {
> +      logTestInfo("the " + updaterBinPath + " file is signed but the test " +
> +                  "registry key does not exist - failing the test since this " +
> +                  "is likely a build system without the test registry key");

Does the logTestInfo here add much value (because it's printed in a location that's more accessible maybe)? If not, could we remove the |&& isBinarySigned(updaterBinPath)| from the if condition and instead just do the |isBinarySigned(updaterBinPath)| check as part of the |Assert.ok|?

@@ +1628,5 @@
>  
>    if (IS_AUTHENTICODE_CHECK_ENABLED && !isBinarySigned(updaterBinPath)) {
>      logTestInfo("test registry key exists but this test can only run on " +
>                  "builds with signed binaries when " +
>                  "DISABLE_UPDATER_AUTHENTICODE_CHECK is not defined");

same question here as above

@@ +1960,2 @@
>        if (aFailTest) {
> +        Assert.ok(false, "maintenance service should stop! Process " +

Assert.ok(!aFailTest, ...

@@ +3474,5 @@
>      if (gProcess.isRunning) {
>        logTestInfo("attempt to kill process");
>        gProcess.kill();
>      }
> +    do_throw("Launch application timer expired!");

|Assert.ok(false, "Launch application timer shouldn't expire")| ?

Comment 9

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

(In reply to Stephen Pohl [:spohl] from comment #8)
> Comment on attachment 8585890 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8585890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! r+ with questions answered/comments addressed.
> 
> ::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
> @@ +1595,4 @@
> >      if (IS_AUTHENTICODE_CHECK_ENABLED && isBinarySigned(updaterBinPath)) {
> > +      logTestInfo("the " + updaterBinPath + " file is signed but the test " +
> > +                  "registry key does not exist - failing the test since this " +
> > +                  "is likely a build system without the test registry key");
> 
> Does the logTestInfo here add much value (because it's printed in a location
> that's more accessible maybe)? If not, could we remove the |&&
> isBinarySigned(updaterBinPath)| from the if condition and instead just do
> the |isBinarySigned(updaterBinPath)| check as part of the |Assert.ok|?
Likely not and changed.

> @@ +1628,5 @@
> >  
> >    if (IS_AUTHENTICODE_CHECK_ENABLED && !isBinarySigned(updaterBinPath)) {
> >      logTestInfo("test registry key exists but this test can only run on " +
> >                  "builds with signed binaries when " +
> >                  "DISABLE_UPDATER_AUTHENTICODE_CHECK is not defined");
> 
> same question here as above
ditto

> @@ +1960,2 @@
> >        if (aFailTest) {
> > +        Assert.ok(false, "maintenance service should stop! Process " +
> 
> Assert.ok(!aFailTest, ...
Since this ends up in the log I'd prefer only doing this when it has failed since the logs are confusing enough.

> @@ +3474,5 @@
> >      if (gProcess.isRunning) {
> >        logTestInfo("attempt to kill process");
> >        gProcess.kill();
> >      }
> > +    do_throw("Launch application timer expired!");
> 
> |Assert.ok(false, "Launch application timer shouldn't expire")| ?
changed

Comment 10

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

Attachment: patch - comments addressed

Comment 11

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

Pushed updated patch to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd19c6fd20f7

Comment 12

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

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

Comment 13

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

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