671825 - Unclear failure message when update is impossible due to Firefox being read-only/unwritable (no update permissions)

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[[:Aleksej]]

When Firefox directory is read-only,
/restartTests/testSoftwareUpdateAutoProxy/test2.js fails with:

  “Update permissions - got 'false' expected 'true'”

It is not very clear that “Update permissions” are (or depend on) Firefox files write permissions.

Comment 1

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

Attachment: Patch v1

This test was kinda broken in several areas. The following fixes have been incorporated:

* Early exit the test if the user doesn't have the permission to update Firefox
* Check for the error pane and not for no updates found. That causes a problem if an update is currently downloaded.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 546744 [details] [diff] [review]
Patch v1

The code's fine. I'm not nuts about the behavior though. 

I really don't like a test being marked as PASS when the test didn't run. That means I can't trust the result at all. It's meaningless for verifying the test ran correctly and to completion.

Do we have any way to skip a test from inside it, or mark it NOTRUN or something like that? If not, we should get that capability added to Mozmill. The idea is when something fails environmental requirements, you NOTRUN it.

The truth of the matter is, I'd almost rather have it mark a FAIL than do this.

Consider this r=me, but I'd like to understand whether there's any better way before we commit this.

Comment 3

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

Attachment: Patch v2

Good call. Sure, it's possible to mark a test as skipped based on flags we can retrieve inside the setupModule function. I have updated this patch which now skips the test function if there are no permissions to update Firefox.

Comment 4

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

Attachment: Patch v2.1

Comment 5

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/16048747

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 547039 [details] [diff] [review]
Patch v2.1

Looks fine. r+

The exception handling behavior will eat any real errors that happen in that clause (we should rethrow anything but timeouts) but I'm not sure we have a particularly good way around that yet.

It's not worse than it was before, though, and I think this problem exists elsewhere too so I don't want to hang on it. Let's get it in.

Comment 7

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

Attachment: Patch v3

No, you are right. We do not need the waitFor call anymore with my changes. Now that we check that the error page is not selected in the wizard, it doesn't matter if updates are available or not.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 547340 [details] [diff] [review]
Patch v3

In that case, excellent change. It's really simplified the test. r+

Comment 9

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/7b280136846b (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/37107323d519 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2220960f2220 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/ee8a9967add6 (release)

Comment 10

[[:Aleksej]]

Verified fix by running only restartTests/testSoftwareUpdateAutoProxy:
* Nightly: 20110721030828-eo-x86_64
* Aurora: 20110721042002-ru-x86_64 http://mozmill-crowd.brasstacks.mozilla.com/db/8d67634d3d2adcac4408713078af447b
* Beta: 6.0b2 candidate 1 en-US
* Release: 5.0 ru