Intermittent test_marionette.py TestProtocol2Errors.test_known_error_status | OSError: [Errno 2] No such file or directory: '/tmp/tmpSvjGCe'

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: intermittent-bug-filer, Assigned: maja_zf)

Tracking

({intermittent-failure})

Version 3
mozilla55
Unspecified
Android
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

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.
Flags: needinfo?(ahalberstadt)
OS: Unspecified → Android
Blocks: 1297394
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.
Flags: needinfo?(ahalberstadt)
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
Duplicate of this bug: 1340635
(Assignee)

Comment 9

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → mjzffr

Comment 12

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-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+

Comment 16

2 years ago
Pushed by mjzffr@gmail.com:
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

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c480bc5e587
https://hg.mozilla.org/mozilla-central/rev/f3b577947e06
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1349786
You need to log in before you can comment on or make changes to this bug.