Closed Bug 1199588 Opened 9 years ago Closed 9 years ago

Profile location is ignored by BaseMarionetteTestRunner (always created under TEMP)

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: whimboo, Assigned: impossibus)

References

Details

(Keywords: pi-marionette-runner)

Attachments

(1 file)

Today I started to work on a workspace patch (bug 1199574) for our firefox-ui-tests which also sticks the profile under a given path. So I assigned the profile path to the profile property of BaseMarionetteTestRunner but there was no change of the location of the profile. Same happens when you specify the --profile CLI option.
It's blocking our work on firefox-ui-tests, so I will work on it.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
So the appropriate code which prevents us specifying a location for the profile is that:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py#91

Means the path as specified via the command line is used as source profile which gets cloned to /tmp. There is no way to let Marionette store the profile data at the given location.

David, is that a feature you could imagine to have for Marionette?
Flags: needinfo?(dburns)
The way that it is doing it now is the way that it should be doing it. Allowing tests to use a profile over and over from a central place and not a clone of it leads to the tests no longer being deterministic.
Flags: needinfo?(dburns)
See Also: → 1200862
Well, there can be different situations:

1. The referenced location via the --profile option is an empty or not existing directory. Then it does not make sense that we totally ignore the option and nevertheless create the profile in the TEMP folder.

2. There are situations when you want to keep the profile and that it should not always duplicated. With our Mozmill tests we were using exactly that and found a couple of bugs in Firefox that way.

In my opinion it's not always bad to allow the user to explicitly force the profile to be used without duplication. Maybe even through a different CLI option.
Oh, and I think that it should not be the task from Marionette to ensure a profile is clean. If a profile location is specified the calling code has to decide if it has to be a clean profile or to re-use the existing one.

Due to that behavior it is not possible right now to pinch Marionette into a workspace where all temporary data is located.
I think we’re making the wrong assumptions about the responsibilities of the different pieces here.  It seems better if the profile argument passed in would be an adapter that is responsible for creating/duplicating/deleting the profile, depending on the desired behaviour.

Something akin to tempfile.NamedTemporaryFile’s API would be desirable:

    def inner():
        # a temporary profile is created
        # and exists only until scoped_profile goes out of scope
        scoped_profile = firefox_profile.TemporaryProfile()
        session = Marionette(profile=scoped_profile)

    inner()
    # scoped_profile is now out of scope, and the profile is deleted

Similarly for a named, scoped profile:

    firefox_profile.NamedTemporaryProfile("/tmp/some-name")

The two temporary profiles above each implement the construct/deconstruct steps needed for setting up and tearing down a fresh, new profile in the desired location.  To do this they internally call down to the persistent profile API:

    persistent_profile = firefox_profile.Profile("~/.mozilla/profiles/my-profile")
    session = Marionette(profile=persistent_profile)
    # persistent_profile is never removed
    # because it is a plain old profile

This prevents overloading terminology in the API (such as “path”), makes a clear distinction between what components do (single responsibility), yet because they obey the same traits they are interchangeable.

A default profile adapter can be chosen for the Marionette constructor, alleviating the need for a magic definition of what None vs. undefined means.

I agree with Henrik that profile management shouldn’t be the responsibility of Marionette, and it would be nice if that responsibility would be delegated elsewhere.

The current behaviour is very surprising because you’re not actually _asking_ Marionette to duplicate the profile (nor should it), you’re asking it to use it.
Thanks Andreas for your comment. I haven't seen it before speaking to David in our 1-1 about this topic. I would have to read through your proposal more deeply because some of your terminology is new for me.

So we came up with other two scenarios - maybe only workarounds for now:

1. As described above only duplicate the profile if the folder as given via the --profile argument contains files. If the folder is empty or non-existent do not clone.

2. Our code could change the environment variable for TMP/TEMP to our workspace dir, so that tempfile will pick this up. This would not stop us from cloning a profile.

I'm totally behind the last 2 sentences of your comment Andreas an letting the tool or user who are invoking Marionette handle the profile situation. IMO the harness is trying to be too smart currently.
I asked Henrik some questions about this today. As I understand it, the main motivations for controlling where the profile is created are that (1) we might want to examine the profile to debug test failures, (2) we'd like to take advantage of Jenkins and/or mozharness for workspace cleanup and management. Ideally, all artifacts produced by a test job should be stored in one, easy-to-find place and managed in the same way.

In my opinion, the current --profile option in Marionette runner should be left as is and understood as the "source" of the profile we want to use: either we clone an existing profile or, if no existing profile is available, we generate a fresh one. However, that cloned/fresh profile should not *have* to go into TEMP.

Inspired by firefox-ui-tests, I propose we add a --workspace option to Marionette runner where the default value is the cwd. This should be understood as a directory path where Marionette stores any files it might generate: screenshots, logs, Firefox profile used during test run ("the clone"), etc. Marionette should not be responsible for the clean-up of these files.
Flags: needinfo?(hskupin)
Flags: needinfo?(dburns)
I would leave this up to David. It sounds good at least to me and which was my initial motivation for the workspace path in our firefox-ui-tests. Also I think that the profile might be handled differently here. It's the only piece as I can see so far which we might want to clean-up. So not sure if the workspace part could be easily applied to this request.
Flags: needinfo?(hskupin)
The reason for using TEMP is that if the there is a failure to remove the directories for some or other reason the OS should then take care of it, at least on a reboot. 

If we are going to add --workspace I would want the default directory to be TEMP so that it is an explicit choice by the user to have a directory there
Flags: needinfo?(dburns)
(In reply to David Burns :automatedtester from comment #11)
> If we are going to add --workspace I would want the default directory to be
> TEMP so that it is an explicit choice by the user to have a directory there

Totally agree. That is also what we have implemented for our firefox-ui-tests harness.
Bug 1199588 - Add --workspace option to Marionette runner; r?automatedtester

The default behaviour is unchanged: gecko.log and logcat are saved to the
cwd, a temporary Firefox profile is cloned or created in TMP.

When a path is specified via --workspace, gecko.log, logcat and the Firefox
profile directory are all saved there instead. A profile saved in the
workspace does not get deleted at the end of a session unless it's a clone.
Attachment #8683345 - Flags: review?(dburns)
Attachment #8683345 - Flags: review?(dburns) → review+
Comment on attachment 8683345 [details]
MozReview Request: Bug 1199588 - Add --workspace option to Marionette runner; r?automatedtester

https://reviewboard.mozilla.org/r/24287/#review21865
Assignee: hskupin → mjzffr
Comment on attachment 8683345 [details]
MozReview Request: Bug 1199588 - Add --workspace option to Marionette runner; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24287/diff/1-2/
Attachment #8683345 - Flags: review+ → review?(dburns)
This is just a rebase, but the interdiff looks odd. The rebase is correct; I think mozreview is confused.
Comment on attachment 8683345 [details]
MozReview Request: Bug 1199588 - Add --workspace option to Marionette runner; r?automatedtester

https://reviewboard.mozilla.org/r/24287/#review22367
Attachment #8683345 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/38257da4c79e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: