testrun_update.py needs to report a Failure if the "restore_binary" method fails

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: andrei, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
On Windows the restore_binary method fails silently ont he following lines:
> shutil.rmtree(self._folder, True)
> shutil.move(self._backup_folder, self._folder)

"rmtree" fails to delete the whole folder, then
"move" moves the _backup_folder into _folder (instead of renaming itself to it).

We then silently fail to run the DirectUpdate test (we miss the firefox build in the correct location).
More investigation info can be found here: https://github.com/mozilla/mozmill-ci/issues/223

Such a failure should raise an exception and not yield to a successful report.
(Assignee)

Updated

6 years ago
Priority: -- → P2
(Assignee)

Updated

6 years ago
Depends on: 858686
(Assignee)

Comment 1

6 years ago
Created attachment 734563 [details] [diff] [review]
Patch v1

Patch originally attached to bug 858686 and which fits better here.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #734563 - Flags: review?(dave.hunt)
Comment on attachment 734563 [details] [diff] [review]
Patch v1

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

A couple of nits, but r=me with these addressed.

::: libs/testrun.py
@@ +883,5 @@
>  
>      def restore_binary(self):
>          """ Restores the backup of the application binary. """
> +        timeout_rmtree = 15
> +        done = time.time() + timeout_rmtree

Can we call this timeout or max_time rather than done, which sounds more like a success boolean.

@@ +888,2 @@
>  
> +        print "*** Remove binary at '%s'" % self._folder

nit: Removing

@@ +898,5 @@
> +                    raise
> +                else:
> +                    time.sleep(1)
> +
> +        print "*** Restore backup from '%s'" % self._backup_folder

nit: Restoring
Attachment #734563 - Flags: review?(dave.hunt) → review+
Successful report: http://mozmill-ci.blargon7.com/#/update/report/cb9e33213cf3d2f37d7b0f8dcc34f8e7

Jenkins console included the following, indicating three failed attempts to remove the temporary folder, before eventually succeeding:

*** Removing binary at 'c:\users\mozauto\appdata\local\temp\tmpkiggp7.binary\'
[Error 145] The directory is not empty: 'c:\\users\\mozauto\\appdata\\local\\temp\\tmpkiggp7.binary\\'

[Error 5] Access is denied: 'c:\\users\\mozauto\\appdata\\local\\temp\\tmpkiggp7.binary\\maintenanceservice_installer.exe'

[Error 5] Access is denied: 'c:\\users\\mozauto\\appdata\\local\\temp\\tmpkiggp7.binary\\maintenanceservice_installer.exe'
*** Restoring backup from 'c:\users\mozauto\appdata\local\temp\tmpwt8omp.binary_backup'
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

6 years ago
Thanks for the landing Dave. Good to see that we make a little bit of progress on that stuff and that we correctly report now what's going wrong. It's a kinda mess what's going on with maintenanceservice_installer.exe here and I don't have a solution for it yet - see latest updates on bug 858686. 

By any means that was not really a blocker for the release tests given that we were affected by that for a longer time. The failure was simply not visible. Anyway, good to see that it has been landed.

I will forward port the patch to our 2.0 automation scripts today.
Blocks: 858686
No longer depends on: 858686
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.