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

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lizzard, Assigned: AndreeaMatei)

Tracking

Version 2

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 fixed)

Details

(Whiteboard: [mozmill-test-failure][sprint])

Attachments

(1 attachment, 3 obsolete attachments)

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
(Reporter)

Comment 1

4 years ago
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
(Reporter)

Updated

4 years ago
Whiteboard: [mozmill-test-failure]
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.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Summary: Test failure "An update has been found" in /testDirectUpdate/testUpdate.js → Update status should not be success=true in case of the check for the applied update is failing
(Assignee)

Comment 3

4 years ago
I will provide a patch tomorrow switching to assert calls where needed.
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
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.
(Assignee)

Updated

4 years ago
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][sprint]
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8516774 [details] [diff] [review]
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.
Attachment #8516774 - Flags: review?(andrei.eftimie)

Comment 7

4 years ago
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.
Attachment #8516774 - Flags: review?(andrei.eftimie) → review-
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8518158 [details] [diff] [review]
patch v2

Updated, failing report sample, I added a failing expect on purpose:
http://mozmill-crowd.blargon7.com/#/update/report/1e5e44bea8f3e17708194a929aa297b5
Attachment #8518158 - Flags: review?(andrei.eftimie)

Comment 10

4 years ago
Comment on attachment 8518158 [details] [diff] [review]
patch v2

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

lgtm
Attachment #8518158 - Flags: review?(hskupin)
Attachment #8518158 - Flags: review?(andrei.eftimie)
Attachment #8518158 - Flags: review+
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".
Attachment #8518158 - Flags: review?(hskupin) → review-

Comment 12

4 years ago
(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.
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.
(Assignee)

Comment 14

4 years ago
Created attachment 8519882 [details] [diff] [review]
patch v3

Updated to bitwise operators and the assert message.
Attachment #8516774 - Attachment is obsolete: true
Attachment #8518158 - Attachment is obsolete: true
Attachment #8519882 - Flags: review?(andrei.eftimie)

Comment 15

4 years ago
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.
Attachment #8519882 - Flags: review?(andrei.eftimie) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8519938 [details] [diff] [review]
patch v3.1

Updated, thanks Andrei!
Attachment #8519882 - Attachment is obsolete: true
Attachment #8519938 - Flags: review?(hskupin)
Attachment #8519938 - Flags: review?(hskupin) → review+
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox-esr31: --- → affected
(Assignee)

Comment 17

4 years ago
Landed on default, the next will follow tomorrow:
http://hg.mozilla.org/qa/mozmill-tests/rev/bcbd5c75b250 (default)
status-firefox36: affected → fixed
(Assignee)

Comment 18

4 years ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox33: affected → fixed
status-firefox34: affected → fixed
status-firefox35: affected → fixed
status-firefox-esr31: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.