836620 - Cleanup app binary copies used by app update xpcshell tests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

A bunch are left behind and they should be cleaned up.

Comment 1

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

Attachment: patch rev1

Also includes some additional logging and cleanup. Sent to try though I didn't have any problems running these locally and I don't expect any problems with these changes though there might be timeouts if the app doesn't exit. If that does happen I'll add additional logging so it is apparent that the application isn't exiting as expected.
https://tbpl.mozilla.org/?tree=Try&rev=69b0e86c5c09

Comment 2

[Brian R. Bondy [:bbondy]]

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

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

I was wondering if it would be better to put the removeCallbackCopy inside a callback for do_register_cleanup.  I'm fine with this though as per your Comment 1.

::: toolkit/mozapps/update/test/unit/head_update.js.in
@@ +1411,5 @@
>                "and the expected command line arguments passed to it");
>    do_check_eq(logContents, expectedLogContents);
>  
>    // Use a timeout to give any files that were in use additional time to close.
> +  do_timeout(TEST_HELPER_TIMEOUT, removeCallbackCopy);

nit: I think you can just call this directly since it loops recursively inside that function already with a timeout.

Comment 3

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

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> Comment on attachment 708439 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 708439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was wondering if it would be better to put the removeCallbackCopy inside a
> callback for do_register_cleanup.  I'm fine with this though as per your
> Comment 1.
I considered that but I wanted this to happen during the testing portion since it has to use a timeout to call the function again in case the binary is in use.
> ::: toolkit/mozapps/update/test/unit/head_update.js.in
> @@ +1411,5 @@
> >                "and the expected command line arguments passed to it");
> >    do_check_eq(logContents, expectedLogContents);
> >  
> >    // Use a timeout to give any files that were in use additional time to close.
> > +  do_timeout(TEST_HELPER_TIMEOUT, removeCallbackCopy);
> 
> nit: I think you can just call this directly since it loops recursively
> inside that function already with a timeout.
good catch and thanks

Comment 4

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

Attachment: patch rev2

Also made a couple of changes to the dump statements to make it clear in the logs where it failed. I'll wait on the try builds before landing.

Carrying forward r+

Comment 5

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dc5d403329

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

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

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/092c67a86353

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/2b7b7dac2ee2

Pushed to mozilla-esr17
https://hg.mozilla.org/releases/mozilla-esr17/rev/e0cbd5e14d37