59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
This is a problem with Marionette when running our tests for Fennec. For example look at: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=mn&bugfiler&fromchange=3b64878e4797a4442e0887b2f4e563dce7cfaa57&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=7778610 As what I think happens here is that we call cleanup() multiple times for the same device instance. By that the temporary folder will no longer exist because it is only getting created in __init__(): https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/mozbase/mozrunner/mozrunner/devices/emulator.py#86 Andrew, what do you think? Or is the emulator class targeted for single use only? We get into this situation when calling FennecRunner.cleanup() from Marionette, eg. for restarts of the application. https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/mozbase/mozrunner/mozrunner/devices/emulator.py#167 If we have to cleanup all the files under the created temporary folder, we should do so but not removing the temporary directory as is can be still used after the cleanup() call.
OS: Unspecified → Android
Just to add.. IMHO if there is a runner I would expect to be able to launch the application multiple times without killing the emulator environment after the first cleanup().
I think the emulator class was initially designed for a single use, but modifying it to be re-useable makes a lot of sense given the long startup times. Could we re-create the temporary directory if it doesn't exist (i.e make it an @property)? Otherwise, deleting just the files would work too. Also cc'ing Maja. She's probably more familiar with this code now than me as she (somewhat recently) wrote the emulator implementation for Android.
The problem is that the temp folder name is set in the __init__ method and that at least one subclass is using it to store its own content. So it might even be a problem to remove all the files. Maybe we should have a temp directory around the whole time, but specific code may want to create its own sub folders? That will be easier to manager for deletion inside of cleanup.
Could we do something like: @property def tempdir(self): if not self._tempdir or not os.path.isdir(self._tempdir): self._tempdir = tempfile.mkdtemp() return self._tempdir This way if the tempdir is ever deleted, a new one will be created instead. If subclasses need to, they can override the cleanup method to not delete it.
No, at least the Emulator class puts userdata into that temporary folder, which we might want to keep until we really reached the end of the test run: https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/mozbase/mozrunner/mozrunner/devices/emulator.py#205 I feel that we have to refactor those classes so that they do not rely on a fixed temporary folder as set in the __init__ method.
9 failures in 722 pushes (0.012 failures/push) were associated with this bug in the last 7 days. Repository breakdown: * mozilla-inbound: 6 * autoland: 3 Platform breakdown: * android-4-3-armv7-api15: 9 For more details, see: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1322993&startday=2017-01-09&endday=2017-01-15&tree=all
Two things: 1. Marionette.quit and Marionette.cleanup don't use DeviceRunner properly: they need to specify whether the emulator should be killed or not. 2. Regardless, DeviceRunner.cleanup shouldn't try to delete a directory that doesn't exist. Marionette.quit intends to only quit the application, so it should not ask to kill the emulator or do any cleanup related to killing the emulator. With regard to Comment 2, it's the Marionette client's responsibility to specify whether the emulator should be killed or not, but it's not doing that properly right now in the Fennec case. DeviceRunner.cleanup is meant to be used when the emulator is being shut down. As far as I can tell, so there's no reason to delete self.tmpdir if we're only killing the browser while keeping the emulator alive. (Is it important to delete the emulator's user data when closing the browser?) I don't think we ever want to kill the emulator except at the very end of a test run, so DeviceRunner's handling of tmpdir is fine for the Marionette usecase. Patch forthcoming.
Comment on attachment 8847804 [details] Bug 1322993 - Distinguish between desktop and emulator when closing instance; https://reviewboard.mozilla.org/r/120728/#review123014 r=me with the comments fixed. Thanks for getting this fixed! ::: testing/marionette/client/marionette_driver/geckoinstance.py:247 (Diff revision 1) > + Close the managed Gecko process. > + > + Depending on self.runner_class, setting `clean` to True may also kill > + the emulator process in which this instance is running. > + > + :param restart: If true, assume this is being called by restart method. nits: please use `True` instead of `true` in all the cases. ::: testing/marionette/client/marionette_driver/geckoinstance.py:385 (Diff revision 1) > > return runner_args > > - def close(self, restart=False): > - super(FennecInstance, self).close(restart) > - if self.runner and self.runner.device.connected: > + def close(self, restart=False, clean=False): > + # If `clean` is true and the Fennec instance is running in an > + # emulator managed by mozrunner, this will stop the emulator. Mind turning this into a docstring?
Attachment #8847804 - Flags: review?(hskupin) → review+
Comment on attachment 8847803 [details] Bug 1322993 - Guard against deleting nonexistent directory during emulator cleanup; https://reviewboard.mozilla.org/r/120726/#review124460
Attachment #8847803 - Flags: review?(hskupin) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/0c480bc5e587 Guard against deleting nonexistent directory during emulator cleanup; r=whimboo https://hg.mozilla.org/integration/autoland/rev/f3b577947e06 Distinguish between desktop and emulator when closing instance; r=whimboo
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.