1083575 - Update status should not be success=true in case of the check for the applied update is failing

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Liz Henry (:lizzard) (relman/hg->git project)]

Mozmill ondemand_update testrun for Firefox 33.0 zu on mm-win-81-64-3 (2014-10-15_15-27-33) completed with  failures. (It says "PASS" but two tests appear to have failed. )

View the build in Jenkins:
http://mm-ci-master.qa.scl3.mozilla.com:8080/job/ondemand_update/97092/

View the results in the Mozmill Dashboard:
http://mozmill-release.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf30215d0
http://mozmill-release.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf30269f5

Build (pre): 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (zu, 20140902214533) - Snippet: AUS
Build (post): 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (zu, 20140902214533) - Snippet: AUS

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Another failure like this but for fr.  
Build (pre): 	Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0 (fr, 20140527133511) - Snippet: AUS
Build (post): 	Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0 (fr, 20140527133511) - Snippet: AUS
	
http://mozmill-release.blargon7.com/#/update/report/ad8dad905f3281cb521a23ddf30ba7f1

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so the problem lays in the last test method:
http://hg.mozilla.org/qa/mozmill-tests/file/fa55b5797aea/firefox/tests/update/testDirectUpdate/testUpdate.js#l86

The assertUpdateApplied() call is not really asserting but each check inside this method is an expect call. That means that we continue until the end of this method, and set the success property to true.

This is indeed something which gives wrong assumptions and can make results invalid. Andreea, is that something one of you can help with?

I will transform this bug into the topic it has to stay in.

Liz, in case of those above problems, please simply retrigger the test first, and if it continuous to fail, you can file a bug. Please try so.

Comment 3

[Andreea Matei [:AndreeaMatei]]

I will provide a patch tomorrow switching to assert calls where needed.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets try to not use assert calls here. Then we wont be able to test multiple final conditions. Maybe a single assert could be used at the end, which checks that an update has really applied successfully. Not sure how to best do that.

Comment 5

[Andreea Matei [:AndreeaMatei]]

So I'm thinking to have a variable incremented in all these expect calls if one fails and check it at the end of the method. If all expects passed, the variable should be 0 for success.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

We're asserting in the end that the new variable still has value 0. If not, then one or more of the expect calls have failed (failure messages that we get in the report/console). 
Tested it by adding a failing expect call.

Comment 7

[Andrei Eftimie]

Comment on attachment 8516774 [details] [diff] [review]
patch v1

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

::: firefox/lib/software-update.js
@@ +383,5 @@
>      var info = aUpdateData.updates[aUpdateData.update.index];
>  
> +    // Set a variable for the outcome of the expect calls,
> +    // in order to assert it in the end
> +    var expectFailed = 0;

I see 2 possible scenarios here:

1) Keep a state valid vs failed, and only report that we have had (1 or more) failures
2) Try to keep track of every expect call and also report which one failed

The current approach is somewhere in the middle, but only reports as option 1) would.

===

If we go for 1) I would suggest to have a boolean status field, and assign directly the return value of each expect call into the variable with a fallback. Something along the following lines:

> var valid = true;
> valid = expect.ok() && valid;
> valid = expect.equal() && valid;
> ...
> assert.ok(valid, %message%);

For 2) if we'd want to keep track of each failure, let's also name each failure in the final failure message.
Start with an empty array and push each failure into the array, and somehow concatenate each message in the final assert call.

For what we want I think option 1) is enough as it will register as a failed build, and we will be able to check all except calls when investigating the failure.

Comment 8

[Andreea Matei [:AndreeaMatei]]

We have the error message of the expect calls in the report, along with the last assert I added, now. So we're fine there. I'll update it to make use of the valid var as you suggested.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated, failing report sample, I added a failing expect on purpose:
http://mozmill-crowd.blargon7.com/#/update/report/1e5e44bea8f3e17708194a929aa297b5

Comment 10

[Andrei Eftimie]

Comment on attachment 8518158 [details] [diff] [review]
patch v2

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

lgtm

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8518158 [details] [diff] [review]
patch v2

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

::: firefox/lib/software-update.js
@@ +389,5 @@
>      // The upgraded version should be identical with the version given by
>      // the update and we shouldn't have run a downgrade
>      var check = Services.vc.compare(info.build_post.version, info.build_pre.version);
> +    valid = expect.ok(check >= 0,
> +                      "The version of the upgraded build is higher or equal") && valid;

Nice proposal from Andrei. I like that method. But please put "&& valid" in-front of expect.ok(), and the latter in it's own line so we don't have an additional 2 char indentation.

@@ +413,5 @@
>      // Check that no application-wide add-ons have been disabled
> +    valid = expect.equal(info.build_post.disabled_addons, info.build_pre.disabled_addons,
> +                         "No application-wide add-ons have been disabled by the update") && valid;
> +
> +    assert.ok(valid, "One or more of the checks for a successfully applied update has failed");

This message should positive. Also I would reduce it to something like: "Build has been upgraded successfully".

Comment 12

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Nice proposal from Andrei. I like that method. But please put "&& valid"
> in-front of expect.ok(), and the latter in it's own line so we don't have an
> additional 2 char indentation.

That would look nicer indeed, but in case of a failure, we won't execute the rest of the expect calls at all. valid needs to come after the expect call if we want to run all of them, regardless of previous failures.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ouch! You are right. Yeah, in such a case it would have to be at the end. I wonder if we could use bitwise operators then. 'valid &= expect.ok()' is resulting in a number but we can check for 1 or 0 in the final assert.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Updated to bitwise operators and the assert message.

Comment 15

[Andrei Eftimie]

Comment on attachment 8519882 [details] [diff] [review]
patch v3

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

r+ with having the same starting value as the final testing at the end

Nice idea with the bitwise assignment operators.

::: firefox/lib/software-update.js
@@ +383,5 @@
>      var info = aUpdateData.updates[aUpdateData.update.index];
>  
> +    // Set a variable for the outcome of the expect calls,
> +    // in order to assert it in the end
> +    var valid = true;

You should make this 1 from the start since you now expect 1 at the end.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.1

Updated, thanks Andrei!

Comment 17

[Andreea Matei [:AndreeaMatei]]

Landed on default, the next will follow tomorrow:
http://hg.mozilla.org/qa/mozmill-tests/rev/bcbd5c75b250 (default)

Comment 18

[Andreea Matei [:AndreeaMatei]]

Backported and did testruns, all green:
http://hg.mozilla.org/qa/mozmill-tests/rev/99c7702ab682 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/6b2478f9007d (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/edd8db583e9b (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/55f4349fbfb4 (esr31)