1275020 - Allow session tests to control life cycle of Firefox

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect, P4)

Description

[André Reinald]

Follow up of bug 1191324.

Comment 1

[André Reinald]

Attachment: Bug 1275020 - Allow session tests to control life cycle of Firefox;

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

Comment 2

[André Reinald]

Comment on attachment 8779247 [details]
Bug 1275020 - Allow session tests to control life cycle of Firefox;

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

Comment 3

[André Reinald]

Comment on attachment 8779247 [details]
Bug 1275020 - Allow session tests to control life cycle of Firefox;

https://reviewboard.mozilla.org/r/70284/#review67564

::: testing/marionette/harness/session/session_test.py
(Diff revision 1)
>          # mozlog via the test id, so this is overriden to maintain
>          # consistency.
>          return self.test_name
>  
>      def setUp(self):
> -        # Convert the marionette weakref to an object, just for the

Removed unfitted comment here.

::: testing/marionette/harness/session/session_test.py:246
(Diff revision 1)
>              self.marionette.timeouts(self.marionette.TIMEOUT_SEARCH, self.marionette.timeout)
>              self.marionette.timeouts(self.marionette.TIMEOUT_SCRIPT, self.marionette.timeout)
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)
>          else:
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, 30000)
> +        self.marionette.quit_in_app()

The marionette driver constructor launches Firefox. But session tests should start with no instance of Firefox launched.

I see 3 solutions to achieve this:

1. don't instantiate a marionette driver here -> causes lots of crashes in code outside of my reach.

2. add a parameter to the marionette driver constructor so it doesn't launch an instance -> I thought I could try that later.

3. stop the instance of Firefox just after having instantiated the marionette driver -> this is what I tried here.

::: testing/marionette/harness/session/session_test.py:248
(Diff revision 1)
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)
>          else:
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, 30000)
> +        self.marionette.quit_in_app()
> +
> +    def start_session(self):

This code is duplicated from the marionette driver "restart" method. I though it would make sense to factorize that into a "launch_instance" method. I'm open to discussions on that.

::: testing/marionette/harness/session/session_test.py:258
(Diff revision 1)
> +            # In some cases Firefox restarts itself by spawning into a new process group.
> +            # As long as mozprocess cannot track that behavior (bug 1284864) we assist by
> +            # informing about the new process id.
> +            self.marionette.instance.runner.process_handler.check_for_detached(self.session['processId'])
> +
> +    def stop_session(self):

Using the new method "quit_in_app" implemented in bug 1279243.

::: testing/marionette/harness/session/tests/test_session.py:13
(Diff revision 1)
>  from session.session_test import SessionTestCase as TC
>  
>  
>  class TestHelloWorld(TC):
>      def setUp(self):
>          TC.setUp(self)

setUp and tearDown should not start and stop Firefox any more.

::: testing/marionette/harness/session/tests/test_session.py:19
(Diff revision 1)
>  
>      def tearDown(self):
>          TC.tearDown(self)
>  
>      def test_malformed_packet(self):
> +        TC.startSesstion()

Each test is responsible for controlling the lifecycle of Firefox through calls to start_session() and stop_session(). Misspelled here… correcting right away.

Comment 4

[André Reinald]

Comment on attachment 8779247 [details]
Bug 1275020 - Allow session tests to control life cycle of Firefox;

https://reviewboard.mozilla.org/r/70282/#review67576

::: testing/marionette/harness/session/tests/test_session.py:19
(Diff revisions 1 - 2)
>  
>      def tearDown(self):
>          TC.tearDown(self)
>  
>      def test_malformed_packet(self):
> -        TC.startSesstion()
> +        TC.start_session()

Corrected misspellings.

Comment 5

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

Comment on attachment 8779247 [details]
Bug 1275020 - Allow session tests to control life cycle of Firefox;

https://reviewboard.mozilla.org/r/70284/#review68924

I haven't tried running this but here's my understanding of the behaviour. For every test method that is run you see the following: 
* start a Firefox instance then restart it (setUp)
* reconnect to the Firefox instance that was just restarted (start_session), [do test steps], restart Firefox (stop_session)
* stop the Firefox instance (tearDown)

So for every test method, Firefox is started 3 times.

::: testing/marionette/harness/session/session_test.py:246
(Diff revision 1)
>              self.marionette.timeouts(self.marionette.TIMEOUT_SEARCH, self.marionette.timeout)
>              self.marionette.timeouts(self.marionette.TIMEOUT_SCRIPT, self.marionette.timeout)
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)
>          else:
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, 30000)
> +        self.marionette.quit_in_app()

In order to start a Marionette session, you need to be connect to a Marionette server (i.e. a Fx binary needs to be running). So if you don't start an instance during setUp, you can't start a session either, which means your individual test methods will have to start/stop Marionette sessions -- this might be fine for your usecase. However, if you prefer to start one common session during setUp, then Option 3 seems the way to go.

Re Option 2, maybe you don't even need a special parameter: if you init `Marionette` with `bin=None`, no instance will be started, then you have the option to set `marionette.bin` and start an instance whenever you're ready. As explained above, in that case each test method would have to start/stop Marionette sessions as needed.

::: testing/marionette/harness/session/session_test.py:248
(Diff revision 1)
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, self.marionette.timeout)
>          else:
>              self.marionette.timeouts(self.marionette.TIMEOUT_PAGE, 30000)
> +        self.marionette.quit_in_app()
> +
> +    def start_session(self):

Yes, refactoring this as a method in Marionette class could make sense. Note that it does not launch a Firefox instance: instead, it assumes that an instance is already running (launched via in-app restart) and it is reconnecting to it with the same session id. A good name might be "restart_session".

::: testing/marionette/harness/session/session_test.py:258
(Diff revision 1)
> +            # In some cases Firefox restarts itself by spawning into a new process group.
> +            # As long as mozprocess cannot track that behavior (bug 1284864) we assist by
> +            # informing about the new process id.
> +            self.marionette.instance.runner.process_handler.check_for_detached(self.session['processId'])
> +
> +    def stop_session(self):

To avoid confusion, you might want to call this something like "restart_binary", since it does more than just stop a Marionette session. (There's a delete_session method in the Marionette class that only stops a session.)

::: testing/marionette/harness/session/tests/test_session.py:12
(Diff revision 1)
>      def setUp(self):
>          TC.setUp(self)
>  
>      def tearDown(self):
>          TC.tearDown(self)

Since setUp just calls TC.setUp, which is inherited anyway, you should be able to omit this definition altogether. Same with tearDown.

::: testing/marionette/harness/session/tests/test_session.py:13
(Diff revision 1)
>  from session.session_test import SessionTestCase as TC
>  
>  
>  class TestHelloWorld(TC):
>      def setUp(self):
>          TC.setUp(self)

In this patch, SessionTestCase.setUp starts a Firefox instance (call to Marionette(bin=...)) then restarts it (quit_in_app()). SessionTestCase.tearDown closes the Firefox instance.

Also, if you expect to call start_session and stop_session at the beginning and end of every test method, then these calls really belong in setUp and tearDown.

Comment 6

[Georg Fritzsche [:gfritzsche]]

After the new approach from bug 1301781 et al, i think this doesn't apply anymore.