1429759 - Marionette tests should be able to safely switch the profile

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

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

By default the Marionette harness is automatically creating a profile for the tests. That was fine so far. But to get tests for bug 1423897 implemented, the test itself has to exactly specify the path and/or the folder name of the profile.

We should allow Marionette tests to switch the profile. Keep in mind that this should only be possible while Firefox is closed. Otherwise dataloss might appear for data read/write to the profile.

Also this is only necessary for Marionette tests which are run by a binary (instance) controlled by Marionette itself.

Comment 1

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

Attachment: Bug 1429759 - Allow Marionette tests to change the user profile.

If Marionette handles the instance of the application including the
user profile, the tests should be able to change the user profile.

This will enable tests to cover specific bugs in the application as
caused by the profile name and path.

To prevent dataloss changing the profile should only be allowed
when the application is not running.

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

Comment 2

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

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

Comment 3

[David Burns :automatedtester]

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review218856

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2dd58774f0c
Allow Marionette tests to change the user profile. r=automatedtester

Comment 5

[Cosmin Sabou [:CosminS]]

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

Comment 6

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

Due to the failures as seen since the patch got landed, can we please back it out from all the branches it landed on? Thanks.

Comment 7

[Cristina Coroiu [:ccoroiu]]

Backed out 1 changesets (bug 1429759) on request from whimboo a=backout

Backout: https://hg.mozilla.org/mozilla-central/rev/f095d500e45453ae318c26097dc2258bdf48d084

Comment 8

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

The Android failures are already fixed with the last update of the patch, but I will still have to dig into the awsy failures.

Comment 9

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

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

Comment 10

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

And here the explanation of the memory regression:

With my patch I added a new `profile` property to the Marionette Python class. It is responsible to clean-up an old profile, before setting or creating a new one. This code should run first, after all constructors are done because each sub class can add additional preferences and other profile options.

Sadly I used `self.profile = profile` in the Marionette class, and as such a new profile has been created immediately, which only contained the required preferences as set by the GeckoInstance class. But misses everything from the DesktopInstance class, which itself updates the required preferences after calling its super __init__ method.

As result we missed all the preferences from DesktopInstance:
https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/testing/marionette/client/marionette_driver/geckoinstance.py#419-490

I wonder why none of our existing unit tests noticed this problem! At least one new tests needs to be written.

Comment 11

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/3-4/

Comment 12

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/4-5/

Comment 13

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/5-6/

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review220348


Static analysis found 18 defects in this patch.
 - 18 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:189
(Diff revision 6)
> +
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:190
(Diff revision 6)
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:194
(Diff revision 6)
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")
> +
> +        if isinstance(profile, Profile):
> +            # Only replace the profile if it is not the current one
> +            if hasattr(self, "_profile") and profile is self._profile:

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:203
(Diff revision 6)
> +            profile_args = self.profile_args
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:204
(Diff revision 6)
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:205
(Diff revision 6)
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:208
(Diff revision 6)
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),
> +                    dir=self.workspace)
> +                # The target must not exist yet
> +                os.rmdir(profile_args["path_to"])

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:214
(Diff revision 6)
> +
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 6)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 6)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:233
(Diff revision 6)
> +
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 6)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 6)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:237
(Diff revision 6)
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port
> +
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 6)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 6)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:244
(Diff revision 6)
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level
> +
>          if "-jsdebugger" in self.app_args:
> -            profile_args["preferences"].update({
> +            args["preferences"].update({

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:253
(Diff revision 6)
>                  "devtools.debugger.prompt-connection": False,
>                  "marionette.debugging.clicktostart": True,
>              })
> +
>          if self.addons:
> -            profile_args["addons"] = self.addons
> +            args["addons"] = self.addons

Error: Remove bad quotes. [flake8: Q000]

Comment 15

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/6-7/

Comment 16

[Code Review Bot [:reviewbot]]

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review220368


Static analysis found 18 defects in this patch.
 - 18 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:189
(Diff revision 7)
> +
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:190
(Diff revision 7)
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:194
(Diff revision 7)
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")
> +
> +        if isinstance(profile, Profile):
> +            # Only replace the profile if it is not the current one
> +            if hasattr(self, "_profile") and profile is self._profile:

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:203
(Diff revision 7)
> +            profile_args = self.profile_args
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:204
(Diff revision 7)
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:205
(Diff revision 7)
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:208
(Diff revision 7)
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),
> +                    dir=self.workspace)
> +                # The target must not exist yet
> +                os.rmdir(profile_args["path_to"])

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:214
(Diff revision 7)
> +
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 7)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 7)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:233
(Diff revision 7)
> +
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 7)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 7)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:237
(Diff revision 7)
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port
> +
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 7)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 7)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:244
(Diff revision 7)
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level
> +
>          if "-jsdebugger" in self.app_args:
> -            profile_args["preferences"].update({
> +            args["preferences"].update({

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:253
(Diff revision 7)
>                  "devtools.debugger.prompt-connection": False,
>                  "marionette.debugging.clicktostart": True,
>              })
> +
>          if self.addons:
> -            profile_args["addons"] = self.addons
> +            args["addons"] = self.addons

Error: Remove bad quotes. [flake8: Q000]

Comment 17

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

Ionat, would you mind to check the memory consumption of the following try build? It would be great to know that with the updated patch we don't see a regression anymore.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d21df989ee6

Thanks!

Comment 18

[Ionuț Goldan [:igoldan]]

Seems like my local dev environment got broken somehow. I would first have to fix it and then do the checks. If you can, simply backout your changes our this push [1], then repush to Try, so we have something to compare against.

Comment 19

[Ionuț Goldan [:igoldan]]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d21df989ee6

Comment 20

[Ionuț Goldan [:igoldan]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #18)
> Seems like my local dev environment got broken somehow. I would first have
> to fix it and then do the checks. If you can, simply backout your changes
> our this push [1], then repush to Try, so we have something to compare
> against.

I managed to fix my environment and did two pushes on Try, to do the comparison. Will notify you of the results.

Comment 21

[Ionuț Goldan [:igoldan]]

Here is the comparison view for the Try pushes [1]. The new changes look good. No AWSY regressions, rather some improvements.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8bad81d6f7e1&newProject=try&newRevision=3efd201494a7&framework=4

Comment 22

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

Great, and thanks for checking this.

Before I ask for another review I have to check the Mn6 failures on Android. I explicitly marked the restart tests in test_prefs.py as to be skipped on mobile, but Fennec got restarted for the very first test `TestEnforcePreferences.test_change_preference`. I wonder if in some cases `setUp()` is run even for skipped tests?

https://treeherder.mozilla.org/logviewer.html#?job_id=157858837&repo=try&lineNumber=2190

[task 2018-01-23T01:38:14.640Z] 01:38:14     INFO -  TEST-START | testing/marionette/harness/marionette_harness/tests/unit/test_prefs.py TestEnforcePreferences.test_change_preference
[task 2018-01-23T01:38:22.105Z] 01:38:22     INFO -  mozdevice Detected adb 1.0.39
[task 2018-01-23T01:38:22.188Z] 01:38:22     INFO -  mozdevice Detected Android sdk 18
[task 2018-01-23T01:38:22.297Z] 01:38:22     INFO -  mozdevice will use zip to push directories
[task 2018-01-23T01:38:29.357Z] 01:38:29     INFO -  timed out waiting for profiles.ini
[task 2018-01-23T01:38:31.731Z] 01:38:31     INFO -  Application command: /builds/worker/workspace/build/android-sdk-linux/platform-tools/adb -s emulator-5554 shell am start -a android.activity.MAIN -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp --es args '-no-remote -profile /storage/sdcard/tests/profile -marionette' --es env0 MOZ_CRASHREPORTER=1 --es env1 R_LOG_VERBOSE=1 --es env2 MOZ_HIDE_RESULTS_TABLE=1 --es env3 MOZ_LOG=signaling:3,mtransport:4,DataChannel:4,jsep:4,MediaPipelineFactory:4 --es env4 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env5 R_LOG_DESTINATION=stderr --es env6 MOZ_CRASHREPORTER_NO_REPORT=1 --es env7 NO_EM_RESTART=1 --es env8 MOZ_PROCESS_LOG=/tmp/tmpXLaEUopidlog --es env9 R_LOG_LEVEL=6
[task 2018-01-23T01:38:32.906Z] 01:38:32     INFO -  Starting: Intent { act=android.activity.MAIN cmp=org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp (has extras) }
[task 2018-01-23T01:40:00.087Z] 01:40:00     INFO -  TEST-SKIP | testing/marionette/harness/marionette_harness/tests/unit/test_prefs.py TestEnforcePreferences.test_change_preference | took 105447ms

Comment 23

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

And that is indeed the problem. Our custom skip decorators are just failing. I will have to fix that in bug 1432456 first.

Comment 24

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

(In reply to Henrik Skupin (:whimboo) from comment #23)
> And that is indeed the problem. Our custom skip decorators are just failing.
> I will have to fix that in bug 1432456 first.

This can't actually be fixed that easily. As such I will move out the `enforceGeckoPrefs` tests into a separate test module.

Comment 25

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/7-8/

Comment 26

[Code Review Bot [:reviewbot]]

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review220542


Static analysis found 18 defects in this patch.
 - 18 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/geckoinstance.py:189
(Diff revision 8)
> +
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:190
(Diff revision 8)
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:194
(Diff revision 8)
> +            raise errors.MarionetteException("The used profile can only be updated "
> +                                             "when the instance is not running")
> +
> +        if isinstance(profile, Profile):
> +            # Only replace the profile if it is not the current one
> +            if hasattr(self, "_profile") and profile is self._profile:

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:203
(Diff revision 8)
> +            profile_args = self.profile_args
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:204
(Diff revision 8)
> +            profile_path = profile
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:205
(Diff revision 8)
> +
> +            # If a path to a profile is given then clone it
> +            if isinstance(profile_path, basestring):
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:208
(Diff revision 8)
> +                profile_args["path_from"] = profile_path
> +                profile_args["path_to"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or os.path.basename(profile_path)),
> +                    dir=self.workspace)
> +                # The target must not exist yet
> +                os.rmdir(profile_args["path_to"])

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:214
(Diff revision 8)
> +
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 8)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:215
(Diff revision 8)
> +                profile = Profile.clone(**profile_args)
> +
> +            # Otherwise create a new profile
> +            else:
> +                profile_args["profile"] = tempfile.mkdtemp(
> +                    suffix=".{}".format(profile_name or "mozrunner"),

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:233
(Diff revision 8)
> +
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 8)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:234
(Diff revision 8)
> +        self._update_profile(clone_from, profile_name=profile_name)
> +
> +    @property
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:237
(Diff revision 8)
> +    def profile_args(self):
> +        args = {"preferences": deepcopy(self.required_prefs)}
> +        args["preferences"]["marionette.defaultPrefs.port"] = self.marionette_port
> +
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 8)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:241
(Diff revision 8)
>          if self.prefs:
> -            profile_args["preferences"].update(self.prefs)
> +            args["preferences"].update(self.prefs)
> +
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:244
(Diff revision 8)
>          if self.verbose:
>              level = "TRACE" if self.verbose >= 2 else "DEBUG"
> -            profile_args["preferences"]["marionette.logging"] = level
> +            args["preferences"]["marionette.logging"] = level
> +
>          if "-jsdebugger" in self.app_args:
> -            profile_args["preferences"].update({
> +            args["preferences"].update({

Error: Remove bad quotes. [flake8: Q000]

::: testing/marionette/client/marionette_driver/geckoinstance.py:253
(Diff revision 8)
>                  "devtools.debugger.prompt-connection": False,
>                  "marionette.debugging.clicktostart": True,
>              })
> +
>          if self.addons:
> -            profile_args["addons"] = self.addons
> +            args["addons"] = self.addons

Error: Remove bad quotes. [flake8: Q000]

Comment 27

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review220706

Lots of questions. I found the new profile management API kind of hard to follow. Made suggestions re documentation.

::: testing/marionette/client/marionette_driver/geckoinstance.py:189
(Diff revision 8)
> +
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "

Nit: s/The used profile/The current profile/

::: testing/marionette/client/marionette_driver/geckoinstance.py:189
(Diff revision 8)
> +
> +        :param profile: A Profile instance to be used.
> +        :param name: Profile name to be used in the path.
> +        """
> +        if self.runner and self.runner.is_running():
> +            raise errors.MarionetteException("The used profile can only be updated "

I don't think I saw any tests that check that this error is raised if we attempt to change profiles while instance is running. Could you add?

::: testing/marionette/client/marionette_driver/geckoinstance.py:227
(Diff revision 8)
> +
> +        self._profile = profile
> +
> +    def switch_profile(self, profile_name=None, clone_from=None):
> +        if isinstance(clone_from, Profile):
> +            clone_from = clone_from.profile

Why not use the Profile object directly instead of always passing the profile directory? Whatever the answer, I think it should be explained in a docstring here, as it's somewhat surprising behaviour.

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:23
(Diff revision 8)
> +    def setUp(self):
> +        super(BaseProfileManagement, self).setUp()
> +
> +        self.orig_profile_path = self.profile_path
> +
> +        # Create external profile and mark it as not-removable

This comment isn't clear to me. I think it would help to replace this comment with one directly above line 28 (create_new) that says something like "prevent profile from being removed during cleanup"

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:24
(Diff revision 8)
> +        tmp_dir = tempfile.mkdtemp(suffix="external")
> +        shutil.rmtree(tmp_dir, ignore_errors=True)
> +
> +        self.external_profile = mozprofile.Profile(profile=tmp_dir)
> +        self.external_profile.create_new = False

These steps in setup/teardown are repeatedly creating/deleting files during 11 subtests that don't use an external profile at all. I think it would worthwhile to move the external profile to a subclass that is used to only test profile switching.

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:99
(Diff revision 8)
> +    def test_quit_keeps_same_profile(self):
> +        self.marionette.quit()
> +        self.marionette.start_session()
>  
> -    def test_change_preference(self):
> -        self.assertTrue(self.marionette.get_pref("marionette.test.bool"))
> +        self.assertEqual(self.profile_path, self.orig_profile_path)
> +        self.assertNotIn(self.workspace, self.profile_path)

I'd add a comment here to explain that the workspace was set after the original session started, which is why we don't expect it to be in use.

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:132
(Diff revision 8)
> +    def setUp(self):
> +        super(TestSwitchProfileWithoutWorkspace, self).setUp()
> +
> +        self.marionette.quit()
> +
> +    def tearDown(self):

This def can be removed since it's only calling the super's teardown anyway.

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:135
(Diff revision 8)
> +    def test_no_cleanup_of_profile_for_path_only(self):
> +        self.marionette.instance._profile = self.external_profile.profile
> +        self.marionette.instance.switch_profile()

How is this test checking lack of cleanup? 
Also, I don't see how setting `_profile` should have any effect on `switch_profile()` here - it creates a new profile either way. Clarification would be helpful.

::: testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py:177
(Diff revision 8)
> +
> +        self.assertNotEqual(self.profile_path, self.external_profile.profile)
> +        self.assertTrue(os.path.exists(self.external_profile.profile))
> +
> +
> +class TestSwitchProfileWithWorkspace(WorkspaceProfileManagement):

You might want to add tests that check that session start-up still works after a profile has been switched.

Comment 28

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review220706

> I don't think I saw any tests that check that this error is raised if we attempt to change profiles while instance is running. Could you add?

Ups, you are right. I completely missed it to add after the refactoring. This is done now.

> Why not use the Profile object directly instead of always passing the profile directory? Whatever the answer, I think it should be explained in a docstring here, as it's somewhat surprising behaviour.

I updated the docstring to make that clear. Just in short... given that we always have to create a new profile we need the path as passed to `_update_profile`. Maybe we can make the `profile_name` argument mandatory.

> This comment isn't clear to me. I think it would help to replace this comment with one directly above line 28 (create_new) that says something like "prevent profile from being removed during cleanup"

Good idea.

> These steps in setup/teardown are repeatedly creating/deleting files during 11 subtests that don't use an external profile at all. I think it would worthwhile to move the external profile to a subclass that is used to only test profile switching.

Not as a subclass because I don't want to duplicate the code for `TestSwitchProfileWithoutWorkspace` and `TestSwitchProfileWithWorkspace`, but a mixin would work here.

> I'd add a comment here to explain that the workspace was set after the original session started, which is why we don't expect it to be in use.

I added this to the `WorkspaceProfileManagement` class itself, because it would affect any test which is based on this class implementation.

> How is this test checking lack of cleanup? 
> Also, I don't see how setting `_profile` should have any effect on `switch_profile()` here - it creates a new profile either way. Clarification would be helpful.

I clarified it. Let me know if that makes more sense now.

> You might want to add tests that check that session start-up still works after a profile has been switched.

I just added this to every test for switching profiles. It should be basically fine this way, and not have to duplicate test cases.

Comment 29

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213006/diff/8-9/

Comment 30

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

Comment on attachment 8942754 [details]
Bug 1429759 - Allow Marionette tests to change the user profile.

https://reviewboard.mozilla.org/r/213006/#review221436

Great! Thanks, Henrik.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/949b1558f9b9
Allow Marionette tests to change the user profile. r=automatedtester,maja_zf

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/949b1558f9b9