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

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Treeherder Bug Filer]

Filed by: hskupin [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=7778610&repo=autoland

https://queue.taskcluster.net/v1/task/DMFgz_EPTNSLg-k-CXklfw/runs/0/artifacts/public/logs/live_backing.log

Comment 1

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

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.

Comment 2

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

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().

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 4

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

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.

Comment 5

[Andrew Halberstadt [:ahal]]

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.

Comment 6

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

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.

Comment 7

[Intermittent Failures Robot]

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

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1322993 - Guard against deleting nonexistent directory during emulator cleanup;

Review commit: https://reviewboard.mozilla.org/r/120726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120726/

Comment 11

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1322993 - Distinguish between desktop and emulator when closing instance;

GeckoInstance should clearly choose between closing just the application under
test versus cleaning up the environment it's running in. For example, if
we're running Fennec in an emulator we need to choose when to only close
Fennec or also kill the emulator.

Review commit: https://reviewboard.mozilla.org/r/120728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120728/

Comment 12

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

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?

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8847803 [details]
Bug 1322993 - Guard against deleting nonexistent directory during emulator cleanup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120726/diff/1-2/

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8847804 [details]
Bug 1322993 - Distinguish between desktop and emulator when closing instance;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120728/diff/1-2/

Comment 15

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

Comment on attachment 8847803 [details]
Bug 1322993 - Guard against deleting nonexistent directory during emulator cleanup;

https://reviewboard.mozilla.org/r/120726/#review124460

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c480bc5e587
https://hg.mozilla.org/mozilla-central/rev/f3b577947e06

Comment 18

[BMO Automation]

Moving bug to Testing::Marionette Client and Harness component per bug 1815831.