1191324 - Extend Marionette to allow automation of telemetry tests

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

As a first step for the Telemetry marionette test we should get it ready for landing and make it runnable from the harness.
That will enable us to:
* share the test code easier
* have the team add test-cases
* have the team run the test locally easily

(1) make it possible to run via: mach marionette-test path/to/test
    ... i.e. get rid of any hard-coded paths
(2) if we need special prefs for running the test we should add them in a separate patch here
(3) move the JavaScript to a separate file for readability & maintainability

Comment 1

[André Reinald]

Attachement #8643009 in bug #1168643 is quite explicit about what's needed to test telemetry, but to sum it up, here is a typical telemetry test:
- do things before firefox is launched (like setting up things in the profile),
- launch a local http server,
- launch firefox,
- do things while it's launched (inside ff and at the os level)
- quit it,
- do things after it's quit,
- possibly start another cycle inside the same testcase,
- stop the local http server,
- do things.

Does MarionetteTestCase allow this kind of testing? If not, what's needed?

Comment 2

[Andreas Tolfsen ❲:ato❳]

I don't have in-depth experience with the Python test runners.

My impression is that it doesn't allow tweaking of the way Firefox is launched or the profile.  You should however be able to launch a local HTTP server in a setup function in your unittest suite.

Comment 3

[André Reinald]

(In reply to Andreas Tolfsen (:ato) from comment #2)
> I don't have in-depth experience with the Python test runners.

Could you redirect the needinfo to anyone who could better help here?

Comment 4

[David Burns :automatedtester]

Hey, I have a few questions before I can really answer

(In reply to André Reinald from comment #1)
> Attachement #8643009 in bug #1168643 is quite explicit about what's needed
> to test telemetry, but to sum it up, here is a typical telemetry test:
> - do things before firefox is launched (like setting up things in the
> profile),

You can pass in a Profile created by mozprofile

> - launch a local http server,
Runner does that now
> - launch firefox,
Runner does that now
> - do things while it's launched (inside ff and at the os level)
marionette can manipulate chrome and content so that should be fine.

> - quit it,
If it is Firefox then you can or the runner will quit it

> - do things after it's quit,
If you manually quit then sure
> - possibly start another cycle inside the same testcase,

Do you mean a sub-test here?

> - stop the local http server,

Runner should do that for you

> - do things.

I guess...

> 
> Does MarionetteTestCase allow this kind of testing? If not, what's needed?

It should for the most part but we won't really know until you try it and it fails. Talking at a high level I can say yes but there might be things that will need updating

Comment 5

[Georg Fritzsche [:gfritzsche]]

Here are notes from a meeting with dburns on the tree integration for these tests:
https://etherpad.mozilla.org/duVZ600wrt

Comment 6

[André Reinald]

Attachment: telemetry_test_command.patch

WIP: added "test-telemetry" command to mach.

Intent:
- either derive as much as possible from current marionette test mechanism, 
- or add tests inside current code to skip firefox launch.

Need to reverse engineer how marionette tests currently work.

Using PuDB as a Python debugger (it has to be installed globally, having it in the _virtualenv is not enough).

Comment 7

[David Burns :automatedtester]

Comment on attachment 8665396 [details] [diff] [review]
telemetry_test_command.patch

redirecting to someone better than me at reviewing this.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8665396 [details] [diff] [review]
telemetry_test_command.patch

Review of attachment 8665396 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/mach_commands.py
@@ +121,5 @@
>          kwargs['binary'] = self.get_binary_path('app')
>          return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
> +
> +@CommandProvider
> +class TelemetryCommands(MachCommands):

Do we need to implement it in this file?
If this mostly kicks off our custom tests now, maybe this (and the other changes) should all live in toolkit/components/telemetry/tests/.
ahal might have a more informed opinion there though.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8665396 [details] [diff] [review]
telemetry_test_command.patch

Review of attachment 8665396 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I'm definitely a fan of splitting up the mach_commands.py files so they don't get too big. That being said, this re-uses a lot of stuff in this file, so you'd have to refactor things and it's probably more work than it's worth. So I'm ok with leaving it here.

Comment 10

[André Reinald]

Attachment: bugzilla-1191324-2.patch

WIP: not launching marionette from 'BaseMarionetteTestRunner' if testType is 'testTelemetry'. This is a possible approach instead of rewriting another TestRunner class.

Comment 11

[André Reinald]

Comment on attachment 8670167 [details] [diff] [review]
bugzilla-1191324-2.patch

Wrong bug number in commit message... will fix.

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8670167 [details] [diff] [review]
bugzilla-1191324-2.patch

Review of attachment 8670167 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, I'd get review from AutomatedTester though.

::: testing/marionette/client/marionette/runner/base.py
@@ +714,5 @@
>  
>          if not result:
>              raise Exception("Could not launch test container app")
>  
> +    def start_firefox(self):

Not sure about the naming here, it isn't necessarily firefox being started.

@@ +735,5 @@
> +        self.start_time = time.time()
> +
> +        need_external_ip = True
> +        if self.test_kwargs.get('testType') != 'testTelemetry':
> +            self.start_firefox()

need_external_ip = self.start_firefox()

Comment 13

[André Reinald]

I've written a breakout of what I have in mind about this bug. Comments welcome.

https://docs.google.com/a/mozilla.com/document/d/18ctfrBs5ofTzbq8JFzY67R8UL-WklSRbhzP4ILeq87U/edit?usp=sharing

Comment 14

[Andrew Halberstadt [:ahal]]

That sounds good with me, but for marionette related stuff it's a good idea to get AutomatedTester's opinion. I can review the mach_commands changes though.

Comment 15

[David Burns :automatedtester]

I am happy to expose the necessary things. Nothing in the Google Doc looks like it will cause problems.

Comment 16

[André Reinald]

Attachment: bugzilla-1191324-2.patch

New version of the patch.
Changed according to comments.
Added the stop_binary(self) method.

Comment 17

[André Reinald]

Attachment: bugzilla-1191324-3.patch

Doing it step by step.
Here I'm proxying the start_binary and stop_binary methods from BaseMarionetteTestRunner so MarionetteTestCase can make use of them.

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8673067 [details] [diff] [review]
bugzilla-1191324-3.patch

Review of attachment 8673067 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think you need to create new 'start_binary' and 'stop_binary' methods. You can directly access the underlying mozrunner instance (see http://mozbase.readthedocs.org/en/latest/mozrunner.html) in the MarionetteTestCase via the 'self.marionette.runner' attribute. To start/stop firefox you should be able to do 'self.marionette.runner.start()' and 'self.marionette.runner.stop()'.

Marionette also has a convenience 'restart' function, that will restart the binary, i.e 'self.marionette.restart()'. I'm not sure if this is what you want/need though.

Comment 19

[André Reinald]

(In reply to Andrew Halberstadt [:ahal] from comment #18)
> Comment on attachment 8673067 [details] [diff] [review]
> bugzilla-1191324-3.patch
> 
> Review of attachment 8673067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think you need to create new 'start_binary' and 'stop_binary'
> methods. You can directly access the underlying mozrunner instance (see
> http://mozbase.readthedocs.org/en/latest/mozrunner.html) in the
> MarionetteTestCase via the 'self.marionette.runner' attribute. To start/stop
> firefox you should be able to do 'self.marionette.runner.start()' and
> 'self.marionette.runner.stop()'.

I was not sure of the implications of the other things happening in BaseMarionetteTestRunner around the calls to marionette.runner.start()/stop(), and I thought it was useful to do them too, so I grouped them inside start/stop_binary() methods.

I also thought it was important to call BaseMarionetteTestRunner methods so they update that class instance variables and allow that class to be aware when/if a client was actually running at a given moment.

In other words, I picked that approach for safety reasons. Though a simpler and less cautious approach may work too.

Would it make sense to try your suggestion at a later moment, and roll back if it breaks something?

Comment 20

[André Reinald]

Attachment: bugzilla-1191324-4.patch

WIP: add a local telemetry ping server to the framework.

There is an argument parsing error on both:
./mach telemetry-test --pingPort=3000
./mach telemetry-test --pingPort 3000

Though in both cases, the kwargs gets filled with the correct key/value pair.

What's missing?

Other comments welcome about where and how to add that PingServer class.

For now it's in:
testing/marionette/client/marionette/runner/pingserver.py

which I want to include from:
testing/marionette/client/marionette/runner/base.py

Comment 21

[André Reinald]

Attachment: bugzilla-1191324-5.patch

WIP: start and stop the ping server from marionette CommonTestCase.setUp() and tearDown()

Comment 22

[Georg Fritzsche [:gfritzsche]]

(In reply to André Reinald from comment #20)
> There is an argument parsing error on both:
> ./mach telemetry-test --pingPort=3000
> ./mach telemetry-test --pingPort 3000

A different question - do we need to pass a port as an argument?
I think the ping server implementation should find a random free port to work from, we should have python code for that in existing marionette code or other test harnesses (although i don't know offhand where).

Comment 23

[André Reinald]

(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> (In reply to André Reinald from comment #20)
> > There is an argument parsing error on both:
> > ./mach telemetry-test --pingPort=3000
> > ./mach telemetry-test --pingPort 3000
> 
> A different question - do we need to pass a port as an argument?
> I think the ping server implementation should find a random free port to
> work from, we should have python code for that in existing marionette code
> or other test harnesses (although i don't know offhand where).

Right, using a random free port instead of a user specified one would be better.
So if someone can point to the right information, it'd be great!

Comment 24

[André Reinald]

But the "parsing error" question still remains.

Comment 25

[Andrew Halberstadt [:ahal]]

Could you paste a stack trace? I'm not really sure what parsing error you're talking about..

> Though in both cases, the kwargs gets filled with the correct key/value pair

Your comment makes it sound like it's working?

As an aside though, please don't use camelCase, instead use --ping-server, and access it as kwargs['ping_server'].

Comment 26

[Andrew Halberstadt [:ahal]]

Comment on attachment 8676224 [details] [diff] [review]
bugzilla-1191324-5.patch

Review of attachment 8676224 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really sure what I'm looking at here, please roll all your changes into a single patch. I don't have a lot of context on what you're trying to accomplish.

Comment 27

[Georg Fritzsche [:gfritzsche]]

(In reply to André Reinald from comment #23)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> > (In reply to André Reinald from comment #20)
> > > There is an argument parsing error on both:
> > > ./mach telemetry-test --pingPort=3000
> > > ./mach telemetry-test --pingPort 3000
> > 
> > A different question - do we need to pass a port as an argument?
> > I think the ping server implementation should find a random free port to
> > work from, we should have python code for that in existing marionette code
> > or other test harnesses (although i don't know offhand where).
> 
> Right, using a random free port instead of a user specified one would be
> better.
> So if someone can point to the right information, it'd be great!

Depending on the python class used you should probably be fine with just passing port 0.
Try e.g. |python -m SimpleHTTPServer 0|.

Comment 28

[André Reinald]

Attachment: bugzilla-1191324-a.patch

WIP - Folded patches into a single one for practicality as new changes will impact old patches, and to allow a better overview of what's being done.

Comment 29

[André Reinald]

After a fresh pull from m-c, with no patches applied, running marionette-test breaks:

> ImportError: cannot import name startTestRunner
> 
>   File "/Users/andre/Programmes/mozilla-central/testing/marionette/mach_commands.py", line 120, in run_marionette_test
>     return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
>   File "/Users/andre/Programmes/mozilla-central/testing/marionette/mach_commands.py", line 39, in run_marionette
>     from marionette.runtests import (

Any idea why?

Comment 30

[Andrew Halberstadt [:ahal]]

Looks like a regression from bug 1212608. I'll comment over there.

Comment 31

[Andrew Halberstadt [:ahal]]

Actually just noticed that bug 1222388 is already filed for this. It has a patch too, just waiting on review.

Comment 32

[André Reinald]

Attachment: bugzilla-1191324-b.patch

WIP:
- Removed command parameter: ping server port is arbitrary.
- Write port in profile before launching Firefox.
- Fixed pingserver.py import,

Need testing, blocking on bug 1222388.

Comment 33

[André Reinald]

Attachment: bugzilla-1191324-c.patch

WIP:
- rebased against current m-c with bug 1222388 fixed
- in 'testTelemetry' case, marionette instance should not be launched in run_tests()
- had to move lots more initialization stuff to start_binary()
- created and now using testing/marionette/client/telemetry/tests/unit-tests.ini file

Comment 34

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

Comment on attachment 8685405 [details] [diff] [review]
bugzilla-1191324-c.patch

Review of attachment 8685405 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the requested reviewer but I thought that I also have a look at this patch to give some feedback. Not sure if some of those items have already been discussed but I think it's worth talking about.

::: testing/marionette/client/marionette/marionette_test.py
@@ -27,5 @@
>  from marionette_driver.wait import Wait
>  from marionette_driver.expected import element_present, element_not_present
>  from mozlog import get_default_logger
>  
> -

nit: you want to leave this blank line for PEP 8.

@@ +453,5 @@
>          # duration of the test; this is deleted in tearDown() to prevent
>          # a persistent circular reference which in turn would prevent
>          # proper garbage collection.
> +        if self.test_runner.testType == 'testTelemetry':
> +            self.test_runner.start_pingServer()

What about making your own class for telemetry tests? The code would be cleanly separated and you wont need those if conditions.

::: testing/marionette/client/marionette/runner/base.py
@@ +25,5 @@
>  from mozlog import get_default_logger
>  from moztest.adapters.unit import StructuredTestRunner, StructuredTestResult
>  from moztest.results import TestResultCollection, TestResult, relevant_line
> +
> +import pingserver

This is a local package which should be in its own block after the httpd import.

@@ +840,5 @@
> +
> +            self.marionette.cleanup()
> +            self.marionette = None
> +
> +    def start_pingServer():

I wonder if we need that in the base Marionette package or if that is better handled in your own subclass for telemetry tests.

@@ +842,5 @@
> +            self.marionette = None
> +
> +    def start_pingServer():
> +        if not self.ping_server:
> +            self.ping_server = PingServer(self.profile)

You do not import PingServer but the module only. So that should fail.

@@ +857,5 @@
> +
> +        need_external_ip = True
> +
> +        if self.testType != 'testTelemetry':
> +            need_external_ip = self.start_binary()

Similar to the other comment. Why not a separate subclass of the TestRunner?

::: testing/marionette/mach_commands.py
@@ +51,5 @@
> +            tests = [os.path.join(topsrcdir,
> +                     'testing/marionette/client/telemetry/tests/unit-tests.ini')]
> +        else
> +            tests = [os.path.join(topsrcdir,
> +                     'testing/marionette/client/marionette/tests/unit-tests.ini')]

Just curious... if no tests have been set shouldn't we run all the tests?

@@ +131,5 @@
> +        conditions=[conditions.is_firefox],
> +        parser=setup_argument_parser,
> +    )
> +    def run_telemetry_test(self, tests, **kwargs):
> +        from pudb import set_trace; set_trace() # TODO: remove this line

I assume it will be removed. :)

Comment 35

[André Reinald]

(In reply to Henrik Skupin (:whimboo) from comment #34)
> Comment on attachment 8685405 [details] [diff] [review]
> bugzilla-1191324-c.patch
> 
> Review of attachment 8685405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not the requested reviewer but I thought that I also have a look at this
> patch to give some feedback. Not sure if some of those items have already
> been discussed but I think it's worth talking about.

That's perfectly OK, thanks for giving early feedback on the patch.

Creating a separate class was discussed in a meeting.
Doing things properly would involve some refactoring in order to pull out most common code between marionette and telemetry classes.
The current approach is to just avoid launching the marionette client (and the Firefox instance) from run_tests(), and let that up to each test in their setUp()/tearDown() methods.

I'll make the other suggested changes.

Comment 36

[Andrew Halberstadt [:ahal]]

Comment on attachment 8685405 [details] [diff] [review]
bugzilla-1191324-c.patch

Review of attachment 8685405 [details] [diff] [review]:
-----------------------------------------------------------------

I understand a bit better what you're trying to do, and I agree with whimboo that it seems like this should probably live outside of testing/marionette as a subclass. Here's an example of another suite of tests overwriting MarionetteTestCase:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L863

But again, I'm not a module owner of marionette so you should really be asking :AutomatedTester or :ato :). Earlier when AutomatedTester redirected to me, he was specifically talking about the mach commands change.

::: testing/marionette/client/marionette/marionette_test.py
@@ +452,5 @@
>          # Convert the marionette weakref to an object, just for the
>          # duration of the test; this is deleted in tearDown() to prevent
>          # a persistent circular reference which in turn would prevent
>          # proper garbage collection.
> +        if self.test_runner.testType == 'testTelemetry':

If self.test_runner is None, this is going to be an AttributeError

@@ +473,5 @@
>              and self.marionette.session_capabilities['b2g'] == True:
>                  self.close_test_container()
>  
>      def tearDown(self):
> +        if self.test_runner.testType == 'testTelemetry':

Ditto.

::: testing/marionette/client/marionette/runner/pingserver.py
@@ +26,5 @@
> +        s.wfile.write('  <head><title>Success</title></head>')
> +        s.wfile.write('  <body>')
> +        s.wfile.write('    <p>The server is working correctly on %s:%d. Firefox should send a POST request</p>' % self.server.pingAddress)
> +        s.wfile.write('  </body>')
> +        s.wfile.write('</html>')

nit: I think a  multiline string would be a bit nicer:

s.swfile.write("""
  <html>
    <head>...</head>
    etc.
  </html>
""")

@@ +29,5 @@
> +        s.wfile.write('  </body>')
> +        s.wfile.write('</html>')
> +
> +    def do_POST(s):
> +        global ping_path

There is no global ping_path variable?

::: testing/marionette/mach_commands.py
@@ +48,5 @@
>  
>      if not tests:
> +        if kwargs.get('testType') == 'testTelemetry':
> +            tests = [os.path.join(topsrcdir,
> +                     'testing/marionette/client/telemetry/tests/unit-tests.ini')]

Tests should always live next to the code they're testing, so I assume these tests should be under toolkit/components/telemetry.

Comment 37

[André Reinald]

It looks like the approach taken since comment 10 doesn't make everyone happy.

Basically, "launch firefox from the test runner" vs "launch firefox from each test" is the main difference between "marionette" and "telemetry" tests. The case is, even if we added a new "telemetry-test" command, to consider this difference as just a variant, not a new kind of tests.

Considering it as a new kind of tests as suggested by hskupin and ahalberstadt implies to:
1. factor out a common "base marionette test runner" class,
2. from there, derive the current "marionette test runner" and the "telemetry test runner" sub-classes,
3. factor out a common "base marionette test case" class,
4. from there, derive the current "marionette test case" and the "telemetry test case" sub-classes.

While that approach makes a more elegant design, it also means:
1. the time spent on this bug up to now is lost,
2. we shoud re-engineer existing code,
3. ETA will be pushed back.

The question is where should we go now?

Comment 38

[André Reinald]

Attachment: bugzilla-1191324-d.patch

WIP: changes according to comment 34 and comment 36.

Comment 39

[Andrew Halberstadt [:ahal]]

I want to clarify that my f- was because I noticed several errors in the patch, and because I don't think the tests should live in testing/marionette. With those issues fixed, I'd be ok landing it as is if Andreas/David are.

(p.s I don't think Henrik's suggestion would require factoring out a base class or anything. I think you could simply subclass the existing base class, overriding what you need to.)

Comment 40

[André Reinald]

(In reply to Andrew Halberstadt [:ahal] from comment #39)
> I want to clarify that my f- was because I noticed several errors in the
> patch, and because I don't think the tests should live in
> testing/marionette. With those issues fixed, I'd be ok landing it as is if
> Andreas/David are.

Those issues have been fixed. Unfortunately there are a few places where the runner class expects a functional instance of marionette. And I'm currently tracking those down.

> (p.s I don't think Henrik's suggestion would require factoring out a base
> class or anything. I think you could simply subclass the existing base
> class, overriding what you need to.)

Without refactoring, we'd have to override all BaseMarionetteTestRunner methods which use the "marionette" instance variable. That's a lot of code which would mostly be duplicated.

Comment 41

[David Burns :automatedtester]

we have had a meeting to discuss this. André is going to go down his current path of clearing up what his tests needs and will raise a future bug that we can make sure we refactor what we need into subclass/<insert the best thing at the time on implementing>

Comment 42

[André Reinald]

Attachment: bugzilla-1191324-e.patch

WIP:
- All pieces should be there.
- The "test runner" class assumes that "self.marionette" always exists. This is not the case for telemetry tests which launch and stop marionette. Currently struggling to remove this assumption (testing "self.marionette" before using it). Is it the right approach?
- When calling "./mach test-telemetry", testing/marionette/client/marionette/runner/base.py:564 throws "TypeError: 'NoneType' object is not callable" (current test file is testing/marionette/client/telemetry/tests/unit/test_telemetry.py). But the "test" argument looks fine.

Comment 43

[André Reinald]

Third - above looks like a bug in the framework, but I need confirmation about this. That code is executed only in case an error is raised, which apparently doesn't happen with current marionette tests.

Anyway, solved the error that triggered that call, and moving forward with this very bug.

Comment 44

[David Burns :automatedtester]

Comment on attachment 8690954 [details] [diff] [review]
bugzilla-1191324-e.patch

Review of attachment 8690954 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Please can you run a a try with 

try: -b do -p linux,macosx64,macosx64_gecko,win32,linux_gecko,linux64_gecko,linux64-mulet -u marionette,marionette-webapi,gaia-ui-test,gaia-integration,web-platform-tests,crashtest-1,crashtest-2,crashtest-3,reftest-1,reftest-2,reftest-3,reftest-4,reftest-5,reftest-6,reftest-7,reftest-8,reftest-9,reftest-10,reftest-11,reftest-12,reftest-13,reftest-14,reftest-15,reftest-16,reftest-17,reftest-18,reftest-19,reftest-20 -t none

::: testing/marionette/mach_commands.py
@@ +41,5 @@
>          BaseMarionetteArguments,
>          MarionetteHarness
>      )
>  
> +    from pudb import set_trace; set_trace() # TODO: remove this line

don't forget ;)

Comment 45

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8690954 [details] [diff] [review]
bugzilla-1191324-e.patch

Review of attachment 8690954 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/runner/pingserver.py
@@ +41,5 @@
> +            plainData = postData
> +
> +        jsonData = json.loads(plainData)
> +        fileName = jsonData['id'] + '.json'
> +        plainFile = open(os.path.join(self.server.pingPath, fileName), 'w')

I don't think we need to persist pings to disk now if that complicates things.

::: testing/marionette/client/telemetry/tests/unit-tests.ini
@@ +5,5 @@
> +; true if the test is compatible with the browser, otherwise false
> +browser = true
> +
> +; true if the test is compatible with b2g, otherwise false
> +b2g = true

We don't need to support B2G here.

Comment 46

[André Reinald]

(In reply to Georg Fritzsche [:gfritzsche] from comment #45)
> I don't think we need to persist pings to disk now if that complicates things.

We have to set the telemetry preferences (including the local ping server) before we launch Firefox.
That's another reason we need a profile before instantiating marionette.
So even if we skip ping persistence on disk, the trick we talked earlier would not work.
Back to the previous search path: create and then provide a profile to marionette.

Comment 47

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

(In reply to André Reinald from comment #46)
> We have to set the telemetry preferences (including the local ping server)
> before we launch Firefox.

If you would create your own runner class you can set preferences which will always be set for any of your test cases. That's something what we do with the update tests. See here:

https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_ui_harness/runners/update.py#L31

Comment 48

[André Reinald]

Current path is coming to a dead end: changing MarionetteTestRunner to match my needs results in awful code.

New plan is here:

https://docs.google.com/a/mozilla.com/document/d/1Y0Mh7lYYieAp3NIQNZvNSO2yhMTYZ-vzL__rlfd0Dxg/edit?usp=sharing

Feedback welcome before I start coding.

Comment 49

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

It would be great if you could give us at least comment permission for the doc. Or not sure where you expect the feedback.

Comment 50

[André Reinald]

(In reply to Henrik Skupin (:whimboo) from comment #49)
> It would be great if you could give us at least comment permission for the doc.

Sure, sorry: comment permission granted.

Comment 51

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

Thank you. Maja and myself added some comments.

Comment 52

[André Reinald]

Attachment: bugzilla-1191324-rel-a.patch

WIP: duplicated runner class, removed lot of code, cleaning errors.

Comment 53

[André Reinald]

Attachment: bugzilla-1191324-rel-a.patch

Sorry for the noise, forgot to attach base2.py in previous patch.

Comment 54

[André Reinald]

Attachment: bugzilla-1191324-rel-b.patch

WIP: classes declared inside base2.py seem "invisible" from the outside, I'm probably missing something here.

Comment 55

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

We solved the mentioned import issues on IRC.

Comment 56

[André Reinald]

Attachment: bugzilla-1191324-rel-c.patch

WIP: solved import errors, duplicated "upper" sources in the tree.

Comment 57

[André Reinald]

Attachment: bugzilla-1191324-rel-d.patch

WIP. TODO: write "helloworld" test (and see what's happening).

Comment 58

[André Reinald]

After about 2 weeks, I just tried to rebase my current work on m-c, only to see the directory testing/marionette/client/marionette disappeared.

As that was the place all my work occurred: base2.py, marionette2_test.py, runtests2.py, and the "sample test" test_marionette2.py, I'm kind of lost…

I guess lots of things have changed. Hints welcome.

Comment 59

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

The directories got renamed recently. See Bug 1246407.

Comment 60

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

(In reply to Maja Frydrychowicz (:maja_zf) from comment #59)
> The directories got renamed recently. See Bug 1246407.

Also see https://groups.google.com/forum/#!topic/mozilla.tools.marionette/2rCNn8jIQFM

Comment 61

[André Reinald]

Attachment: bugzilla-1191324-rel-f.patch

WIP: my "hello world" test is working, launching and stopping the instance.
TODO: make the "hello world" test closer to what telemetry needs, remove the hard coded debugger call.

Comment 62

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

Comment on attachment 8727814 [details] [diff] [review]
bugzilla-1191324-rel-f.patch

I'll take a look myself tomorrow, but I'm adding f? for whimboo since he's been involved in previous discussion.

Comment 63

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

Comment on attachment 8727814 [details] [diff] [review]
bugzilla-1191324-rel-f.patch

This is a huge, hard-to-digest patch, but I will highlight a few things that come to mind. 

[If you would like more detailed feedback about Python style or anything else, please push to MozReview and tell is which areas in particular to look at. (MozReview doesn't support the f? workflow, so just ask for review and note in your commit message that you only want feedback. It would also make it easier to give feedback if you add comments that provide a high-level overview of your changes; otherwise it's hard to keep track of how your changes fit together. Another helpful thing might be to split your patch into several logical commits.]


The following is based on a quick look at diffs between marionette_test.py and marionette2_test.py, and the others.  
* It would be worthwhile to replace Marionette2Harness, Marionette2Arguments, etc., by extending the original classes to get the customisations you need. 
** Example: https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/harness/firefox_ui_harness
** Example: https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/external_media_harness/runtests.py
* Where do you incorporate checking for crashes?
** re `self.shouldStop = True # not sure here` - this should be removed since you removed the crash check.
** It seems you removed http-related stuff. I'm surprised your tests don't need it. I guess you don't need to serve an local resources to your tests?

That's all for now. Hope it helps...

Comment 64

[André Reinald]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #63)
> Comment on attachment 8727814 [details] [diff] [review]
> bugzilla-1191324-rel-f.patch
> 
> This is a huge, hard-to-digest patch, but I will highlight a few things that
> come to mind.

Yes, sorry about that.

> [If you would like more detailed feedback about Python style or anything
> else, please push to MozReview and tell is which areas in particular to look
> at. (MozReview doesn't support the f? workflow, so just ask for review and
> note in your commit message that you only want feedback. It would also make
> it easier to give feedback if you add comments that provide a high-level
> overview of your changes; otherwise it's hard to keep track of how your
> changes fit together. Another helpful thing might be to split your patch
> into several logical commits.]

The highest level overview I can provide is: this test runner lets individual tests handle the life cycle of Firefox instead of providing them with an already running instance (and expecting that instance to be still running when the test finishes). I'll try to add some comments in the code, but I'm not sure exactly where, as this is basically derived from existing code (which I still don't fully understand).

> The following is based on a quick look at diffs between marionette_test.py
> and marionette2_test.py, and the others.  
> * It would be worthwhile to replace Marionette2Harness,
> Marionette2Arguments, etc., by extending the original classes to get the
> customisations you need.

There is indeed some code duplication, because I wanted to avoid adding complexity (basically parameters in constructors of existing classes) in a code that I don't feel comfortable with. I also understood from our conversations that at some point Marionette2 could become a drop-in replacement for Marionette runner, which was another reason to avoid depending on those classes.

> * Where do you incorporate checking for crashes?
> ** re `self.shouldStop = True # not sure here` - this should be removed
> since you removed the crash check.

Yes, that's a part I haven't dealt with right now. The crashes should end up being handled at the test level and not at the runner level. So there is more thinking to put in that.

> ** It seems you removed http-related stuff. I'm surprised your tests don't
> need it. I guess you don't need to serve an local resources to your tests?

I indeed didn't need http-related stuff right now. It was part of the complexity I wanted to get rid of in the first version. I should probably paste that back once that already huge patch lands.

Comment 65

[André Reinald]

I still have things to add (eg. the local ping server). I'll either add them to this patch or put them in a separate patch (but only once this one is stabilized).

Comment 66

[André Reinald]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #63)
> Comment on attachment 8727814 [details] [diff] [review]
> bugzilla-1191324-rel-f.patch
> 
> [Another helpful thing might be to split your patch into several logical commits.]

I didn't find a way to split it in smaller commits while preserving something that actually "works".
The patch is huge basically because it's a fork of existing code, but the number of changes to that code is limited.

Comment 67

[André Reinald]

Attachment: bugzilla-1191324-rel-g.patch

WIP: added back local http server, replicated changes made in base.py (removing xml output).

Comment 68

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 69

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 70

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

r?maja_zf

This is the "change patch" over the duplicated files.

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

Comment 71

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 72

[André Reinald]

Comment on attachment 8734740 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

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

Comment 73

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

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

https://reviewboard.mozilla.org/r/41459/#review39965

I suggest you put all your "duplicated" files under something like `testing/marionette/harness/marionette/telemetry_runner` for now, rather than calling them "thing2".

Comment 74

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

Comment on attachment 8734740 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

https://reviewboard.mozilla.org/r/42475/#review39967

I think overall these changes make sense, but I'd like things to be renamed and organized more neatly even though it's a work in progress. Also:
* You shouldn't have to reimplement MarionetteHarness -- details inline.
* You seem to have removed all crash checks, which doesn't seem like a good idea.

::: testing/marionette/harness/marionette/__init__.py:17
(Diff revision 2)
>      skip,
>      skip_if_b2g,
>      SkipTest,
>      skip_unless_protocol,
>  )
> +from .marionette2_test import (

Since these imports are mostly for convenience, I would prefer it if you reverted your changes to this file. (You will still be able to import whatever you need in your WIP modules - e.g. `from marionette.marionette2_test import Marionette2TestCase` instead of `from marionette import Marionette2TestCase`)

::: testing/marionette/harness/marionette/marionette2_test.py
(Diff revision 2)
>                                    testvars=testvars,
>                                    **kwargs))
>  
>      def setUp(self):
> -        CommonTestCase.setUp(self)
> +        Common2TestCase.setUp(self)
> -        self.marionette.test_name = self.test_name

Why not keep this?

::: testing/marionette/harness/marionette/runner/__init__.py:18
(Diff revision 2)
>      TestManifest,
>      TestResult,
>      TestResultCollection,
>  )
>  
> +from .base2 import (

Please revert the changes to this file. Since these imports are only for convenience, I don't want to include your WIP stuff in here for now.

::: testing/marionette/harness/marionette/runner/base2.py:192
(Diff revision 2)
>                      self.logger.info(' '.join(line).encode('ascii', 'replace'))
>                  self.logger.info('END LOG:')
>  
>      def stopTest(self, *args, **kwargs):
>          unittest._TextTestResult.stopTest(self, *args, **kwargs)
> -        if self.marionette.check_for_crash():
> +        self.shouldStop = True # not sure here

I already wrote some feedback about this in the first round of feedback.

::: testing/marionette/harness/marionette/runtests2.py:28
(Diff revision 2)
>      def __init__(self, **kwargs):
> -        BaseMarionetteArguments.__init__(self, **kwargs)
> +        BaseMarionette2Arguments.__init__(self, **kwargs)
> -        self.register_argument_container(BrowserMobProxyArguments())
>  
>  
> -class MarionetteHarness(object):
> +class Marionette2Harness(object):

As I mentioned in my very first round of feedback, there is no need to duplicate MarionetteHarness, etc. For example, see how the harness is customized in external-media-tests: https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/external_media_harness/runtests.py

::: testing/marionette/harness/marionette/tests2/test_marionette2.py
(Diff revision 2)
>  
>      def tearDown(self):
> -        self.marionette.protocol = self.op
> -        TC.tearDown(self)
> -
> -    def test_malformed_packet(self):

Is something missing from your patch? A bunch of new test methods that you added previously are being removed.

::: testing/marionette/mach_commands.py:127
(Diff revision 2)
>              del kwargs['test_objects']
>  
>          kwargs['binary'] = self.get_binary_path('app')
>          return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
> +
> +def setup_argument_parser2():

Change this to `setup_telemetry_marionette_argument_parser`

::: testing/marionette/mach_commands.py:131
(Diff revision 2)
> +
> +def setup_argument_parser2():
> +    from marionette.runner.base2 import BaseMarionette2Arguments
> +    return BaseMarionette2Arguments()
> +
> +def run_marionette2(tests, testtype=None,

Changes this to `run_marionette_telemetry`.

Add a comment to note that this is a WIP.

::: testing/marionette/mach_commands.py:170
(Diff revision 2)
> +        return 0
> +
> +@CommandProvider
> +class MachCommands2(MachCommandBase):
> +    @Command('marionette2-test', category='testing',
> +        description='Run a Marionette2 test (Check UI or the internal JavaScript using marionette).',

Change description to indicate that this is for calling the WIP telemetry-marionette runner.

::: testing/marionette/mach_commands.py:174
(Diff revision 2)
> +    @Command('marionette2-test', category='testing',
> +        description='Run a Marionette2 test (Check UI or the internal JavaScript using marionette).',
> +        conditions=[conditions.is_firefox],
> +        parser=setup_argument_parser2,
> +    )
> +    def run_marionette2_test(self, tests, **kwargs):

Change this name.

This method can go in the MachCommands class; no need to keep a separate MachCommands2.

::: testing/marionette/mach_commands.py:181
(Diff revision 2)
> +            tests = []
> +            for obj in kwargs['test_objects']:
> +                tests.append(obj['file_relpath'])
> +            del kwargs['test_objects']
> +
> +        from pudb import set_trace; set_trace() #AR TODO: remove this line

Dont' forget to remove this.

Comment 75

[André Reinald]

https://reviewboard.mozilla.org/r/42475/#review40005

A few global comments:
- I intent to address other use-cases than just telemetry, hence the "2" suffix, but I'm willing to consider a better name, just don't want it to be telemetry specific.
- This patch it's meant to be the 1st of a series of follow-ups (probably in separate bugs), but it's already working and could be used. WIP may not be appropriate naming any more.
- The ideal architecture would be to derive existing "marionette" and future "marionette2" classes from a common set of base classes. The difference between the two is wether Firefox is launched by the runner or not. But that's a refactoring far beyond what I can do in a reasonable time.

::: testing/marionette/harness/marionette/marionette2_test.py
(Diff revision 2)
>                                    testvars=testvars,
>                                    **kwargs))
>  
>      def setUp(self):
> -        CommonTestCase.setUp(self)
> +        Common2TestCase.setUp(self)
> -        self.marionette.test_name = self.test_name

Because self.marionette will not exist here in the next version.

For now it's created in CommonTestCase.setUp(), but that will be removed because we want to enable the testcase to do stuff before it launches Firefox.

::: testing/marionette/harness/marionette/runner/base2.py:192
(Diff revision 2)
>                      self.logger.info(' '.join(line).encode('ascii', 'replace'))
>                  self.logger.info('END LOG:')
>  
>      def stopTest(self, *args, **kwargs):
>          unittest._TextTestResult.stopTest(self, *args, **kwargs)
> -        if self.marionette.check_for_crash():
> +        self.shouldStop = True # not sure here

I remember your feedback. But how should I check for crash here, as there is no marionette instance?

::: testing/marionette/harness/marionette/runtests2.py:28
(Diff revision 2)
>      def __init__(self, **kwargs):
> -        BaseMarionetteArguments.__init__(self, **kwargs)
> +        BaseMarionette2Arguments.__init__(self, **kwargs)
> -        self.register_argument_container(BrowserMobProxyArguments())
>  
>  
> -class MarionetteHarness(object):
> +class Marionette2Harness(object):

Yes, I remember that part of feedback and looked into it.

Reusing the MarionetteHarness class, implies adding a testcase_class parameter to __init__() (and self._testcase_class) (needed on line 60).

It would also mean changing code that creates MarionetteHarness instances to account for this change. cli function (on line 76) being one example but maybe not the only one.

Separating the 2 branches of code looked like the safest way.

::: testing/marionette/harness/marionette/tests2/test_marionette2.py
(Diff revision 2)
>  
>      def tearDown(self):
> -        self.marionette.protocol = self.op
> -        TC.tearDown(self)
> -
> -    def test_malformed_packet(self):

TestHelloWorld is meant to be an example for implementing real test cases. I included a fes methods randomly picked from TestProtocol2Errors. The whole class could arguably be removed from the patch as it's not meant to be used in production.

::: testing/marionette/mach_commands.py:127
(Diff revision 2)
>              del kwargs['test_objects']
>  
>          kwargs['binary'] = self.get_binary_path('app')
>          return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
> +
> +def setup_argument_parser2():

I intended to not make it telemetry specific, hence my choice of naming everithing "2" instead of "telemetry". I guess there are other use-cases when tests want to handle the lifecycle of Firefox. Considering this, if you have a better idea than "2" I'll gladly take it.

::: testing/marionette/mach_commands.py:170
(Diff revision 2)
> +        return 0
> +
> +@CommandProvider
> +class MachCommands2(MachCommandBase):
> +    @Command('marionette2-test', category='testing',
> +        description='Run a Marionette2 test (Check UI or the internal JavaScript using marionette).',

Well, that's meant to be the first step of a series of follow-ups, but it's intended to be usable for testing telemetry. Is WIP the appropriate naming?

::: testing/marionette/mach_commands.py:174
(Diff revision 2)
> +    @Command('marionette2-test', category='testing',
> +        description='Run a Marionette2 test (Check UI or the internal JavaScript using marionette).',
> +        conditions=[conditions.is_firefox],
> +        parser=setup_argument_parser2,
> +    )
> +    def run_marionette2_test(self, tests, **kwargs):

Can the same subclass of MachCommandBase handle 2 commands?

Comment 76

[André Reinald]

https://reviewboard.mozilla.org/r/42475/#review39967

A few global comments:
- I intent to address other use-cases than just telemetry, hence the "2" suffix, but I'm willing to consider a better name, just don't want it to be telemetry specific.
- This patch it's meant to be the 1st of a series of follow-ups (probably in separate bugs), but it's already working and could be used. WIP may not be appropriate naming any more.
- The ideal architecture would be to derive existing "marionette" and future "marionette2" classes from a common set of base classes. The difference between the two is wether Firefox is launched by the runner or not. But that's a refactoring far beyond what I can do in a reasonable time.

> Since these imports are mostly for convenience, I would prefer it if you reverted your changes to this file. (You will still be able to import whatever you need in your WIP modules - e.g. `from marionette.marionette2_test import Marionette2TestCase` instead of `from marionette import Marionette2TestCase`)

Not really a WIP any more. Forcing users to prefix their includes is not very convenient.

> Why not keep this?

Because self.marionette will not exist here in the next version.

For now it's created in CommonTestCase.setUp(), but that will be removed because we want to enable the testcase to do stuff before it launches Firefox.

> Please revert the changes to this file. Since these imports are only for convenience, I don't want to include your WIP stuff in here for now.

Not really a WIP any more. Forcing users to prefix their includes is not very convenient.

> I already wrote some feedback about this in the first round of feedback.

I remember your feedback. But how should I check for crash here, as there is no marionette instance?

> As I mentioned in my very first round of feedback, there is no need to duplicate MarionetteHarness, etc. For example, see how the harness is customized in external-media-tests: https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/external_media_harness/runtests.py

Yes, I remember that part of feedback and looked into it.

Reusing the MarionetteHarness class, implies adding a testcase_class parameter to __init__() (and self._testcase_class) (needed on line 60).

It would also mean changing code that creates MarionetteHarness instances to account for this change. cli function (on line 76) being one example but maybe not the only one.

Separating the 2 branches of code looked like the safest way.

> Is something missing from your patch? A bunch of new test methods that you added previously are being removed.

TestHelloWorld is meant to be an example for implementing real test cases. I included a set of test methods randomly picked from TestProtocol2Errors. The whole class could arguably be removed from the patch as it's not meant to be used in production.

> Change this to `setup_telemetry_marionette_argument_parser`

It's not meant at being telemetry specific. Let's find a better name!

> Changes this to `run_marionette_telemetry`.
> 
> Add a comment to note that this is a WIP.

It's not meant at being telemetry specific. Let's find a better name!
And not a WIP neither, it should be usable at this point.

> Change description to indicate that this is for calling the WIP telemetry-marionette runner.

It's not meant at being telemetry specific. Let's find a better name!
And not a WIP neither, it should be usable at this point.

> Change this name.
> 
> This method can go in the MachCommands class; no need to keep a separate MachCommands2.

Can the same subclass of MachCommandBase handle 2 commands?

Comment 77

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

https://reviewboard.mozilla.org/r/42475/#review39967

Regarding the WIP status: would you say it's a prototype? Yes, it works, but there are many details to work out and it doesn't fit with the rest of Marionette runner right now.

> I remember your feedback. But how should I check for crash here, as there is no marionette instance?

It's not something you absolutely need to address right away, but definitely important to keep in mind for the near future. Maybe you can check for crashes somewhere in the TestCase? In tearDown? Maybe it would make sense to use a class or method decorators in certain tests - something along the lines of https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/decorators.py#19. I would ask :AutomatedTester about this.

> Yes, I remember that part of feedback and looked into it.
> 
> Reusing the MarionetteHarness class, implies adding a testcase_class parameter to __init__() (and self._testcase_class) (needed on line 60).
> 
> It would also mean changing code that creates MarionetteHarness instances to account for this change. cli function (on line 76) being one example but maybe not the only one.
> 
> Separating the 2 branches of code looked like the safest way.

Can't you pass in a TelemetryTestRunner [1] (that uses TelemetryTestCase in self.test_handlers) as the harness_class. If you subclass MarionetteHarness instead of duplicating it, you can also override process_args (line 60) to handle the pydebugger check as you need, or skip it altogether.

[1] I'm just using telemetry in the names for the sake of discussion.

> It's not meant at being telemetry specific. Let's find a better name!

Yeah... you're better equipped to find a good name. The main difference is that the marionette session is controlled in the testcases, right, so maybe something like "MarionetteSessionTestCase", etc?

> Can the same subclass of MachCommandBase handle 2 commands?

Yes. Example: https://dxr.mozilla.org/mozilla-central/source/python/mach_commands.py#54

Comment 78

[André Reinald]

https://reviewboard.mozilla.org/r/42475/#review39967

The "it doesn't fit with the rest of Marionette runner" was something I feared since I started this work. I guess it means I'll have to start it all over to "fit" and I'll need much closer guidance to "do things right". Meanwhile, it's quite discouraging to polish up something knowing it's temporary. Any hints as what's the best option to move forward?

Comment 79

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

https://reviewboard.mozilla.org/r/42475/#review39967

I don't think you need to start over at all. My main point is that we should clearly distinguish your pieces from Marionette runner via names and directory structure so that you can land it and continue to work on it. That is, put most of this work under a different directory under testing/marionette/harness and avoid "Thing2" naming wherever possible. 

As this project develops, we might want to integrate it with Marionette runner. As you say, integrating this with Marionette runner itself is a larger undertaking and might require more support from us. However, I think that's a separate issue to be discussed with David later.

Comment 80

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

Changed directory structure, not yet files.

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

Comment 81

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

Comment on attachment 8739972 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

https://reviewboard.mozilla.org/r/45467/#review42113

The `session` directory is a good choice. The mach command changes are in the right direction as well.

Comment 82

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

WIP - changed names of classes / methods / strings from "2" to "session"

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

Comment 83

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

Comment on attachment 8742777 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

https://reviewboard.mozilla.org/r/46465/#review44305

The renaming looks good.

::: testing/marionette/harness/session/runtests.py:28
(Diff revision 1)
> -class Marionette2Arguments(BaseMarionette2Arguments):
> +class SessionArguments(BaseSessionArguments):
>      def __init__(self, **kwargs):
> -        BaseMarionette2Arguments.__init__(self, **kwargs)
> +        BaseSessionArguments.__init__(self, **kwargs)
>  
>  
> -class Marionette2Harness(object):
> +class SessionHarness(object):

To avoid repetition, I suggest you try the following: SessionHarness should subclass MarionetteHarness and just override the process_args method to use SessionTestCase.

If you do that, you don't have to reimpliment cli() here; instead in the `if __name__=='__main__'` block, import cli from marionette harness runtests.py and call it with your "session" harness_class and runner_class and parser_class.

You may modify cli in marionette runtests.py to accept a `name` arg, so you can customize the logger with "Session test runner"

Comment 84

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 85

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

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

https://reviewboard.mozilla.org/r/41459/#review46181

I can't tell what's different in base.py since your previous patches, so I focused my review on the other files. Good integration of MarionetteHarness -- just a few small changes.

::: testing/marionette/harness/session/__init__.py:5
(Diff revision 4)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +__version__ = '2.3.0'

Please remove the version.

::: testing/marionette/harness/session/runtests.py:7
(Diff revision 4)
> +from marionette import __version__
> +from marionette_driver import __version__ as driver_version
> +from session.session_test import SessionTestCase, SessionJSTestCase
> +from session.runner import (
> +    BaseSessionTestRunner,
> +    BaseSessionArguments,
> +)
> +from marionette.runtests import MarionetteHarness, cli
> +import mozlog

There are some unused imports here.

::: testing/marionette/harness/session/runtests.py:28
(Diff revision 4)
> +
> +class SessionArguments(BaseSessionArguments):
> +    def __init__(self, **kwargs):
> +        BaseSessionArguments.__init__(self, **kwargs)
> +
> +class SessionHarness(MarionetteHarness):

We don't need to define SessionHarness; just call cli with the appropriate args.

::: testing/marionette/harness/session/runtests.py:38
(Diff revision 4)
> +                 args=None):
> +        MarionetteHarness.__init__(self, runner_class, parser_class, testcase_class, args)
> +
> +
> +if __name__ == "__main__":
> +    cli(runner_class=SessionTestRunner, parser_class=SessionArguments,

This can just be `cli(runner_class=SessionTestRunner, parser_class=SessionArguments,         harness_class=MarionetteHarness, testcase_class=SessionTestCase)`

Comment 86

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

WIP - Corrections after last feedback.

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

Comment 87

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

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

https://reviewboard.mozilla.org/r/49525/#review46355

These changes look good. Some minor fixes needed in the mach command. 

After this, the patch will be mostly polished. For final approval by me and AutomatedTester, I'd like to see one review request that contains two commits: the first where files are copied and appropriately named for your new directory structure, the second where the copies are changed to meet the requirements of your tests and your tests are added (along with any other changes to mach commands, MarionetteHarness, etc.).

::: testing/marionette/mach_commands.py:72
(Diff revision 1)
>      parser.verify_usage(args)
>  
>      args.logger = commandline.setup_logging("Marionette Unit Tests",
>                                              args,
>                                              {"mach": sys.stdout})
> -    failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run()
> +    failed = MarionetteHarness(runner_class=MarionetteTestRunner, parser_class=MarionetteArguments,

The parameters supplied to MarionetteHarness all match the default, so you should be able to omit them, as well as the imports above.

::: testing/marionette/mach_commands.py:100
(Diff revision 1)
> +        SessionTestCase,
> +    )
> +
> +    parser = BaseSessionArguments()
> +    commandline.add_logging_group(parser)
> +    args = parser.parse_args()

I think you need to update this code -- there have been some changes in run_marionette over the past few months; this is a copy of older code.

For example, parser.parse_args() will reparse all the args on the command-line (already parsed by mach), which led to some bugs previously. This is why in run_marionette() we only "reparse" the tests.

::: testing/marionette/mach_commands.py:108
(Diff revision 1)
> +        tests = [os.path.join(topsrcdir,
> +                 'testing/marionette/harness/session/tests/unit-tests.ini')]
> +    args.tests = tests
> +
> +    args.binary = binary
> +    path, exe = os.path.split(args.binary)

This isn't used.

Comment 88

[André Reinald]

https://reviewboard.mozilla.org/r/49525/#review46355

> The parameters supplied to MarionetteHarness all match the default, so you should be able to omit them, as well as the imports above.

I wrote it on purpose to reflect what's a few lines below in run_session().
If we want all default arguments removed, let's also remove the MarionetteTestRunner argument.

> I think you need to update this code -- there have been some changes in run_marionette over the past few months; this is a copy of older code.
> 
> For example, parser.parse_args() will reparse all the args on the command-line (already parsed by mach), which led to some bugs previously. This is why in run_marionette() we only "reparse" the tests.

I just copied the latest run_marionette() from dxr, then reported the changes in run_session().

> This isn't used.

It isn't used in run_marionette() neither, so I removed both.

Comment 89

[André Reinald]

Attachment: MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

WIP - Corrections after last feedback.

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

Comment 90

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

Comment on attachment 8747680 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

https://reviewboard.mozilla.org/r/49991/#review46727

Ok, looks good.
Please histedit down to two commits as I described before (a copy commit and a changes commit), and flag both me and AutomatedTester for review. Thanks!

Comment 91

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 92

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 93

[André Reinald]

Comment on attachment 8747680 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

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

Comment 94

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

Comment on attachment 8747680 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests; r?maja_zf

https://reviewboard.mozilla.org/r/49991/#review47831

The changes in Part 2 depend on Part 3, so Part 3 should come first in the commit series. (Or put Part 2 and 3 into one commit.)

Comment 95

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

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

https://reviewboard.mozilla.org/r/41459/#review47833

Comment 96

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

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

https://reviewboard.mozilla.org/r/49525/#review47857

::: testing/marionette/harness/session/runner/base.py:192
(Diff revision 2)
>                      self.logger.info(' '.join(line).encode('ascii', 'replace'))
>                  self.logger.info('END LOG:')
>  
>      def stopTest(self, *args, **kwargs):
>          unittest._TextTestResult.stopTest(self, *args, **kwargs)
> -        if self.marionette.check_for_crash():
> +        self.shouldStop = True # not sure here

stopTest is called at the end of each test method, I believe, and when self.shouldStop is set to True, no more tests in the current module will be run, so you should remove this line. (See: https://github.com/python/cpython/blob/2410a06c294d9b091f39f09f638c1d83d0ff440e/Lib/unittest/case.py#L584 and https://github.com/python/cpython/blob/2d264235f6e066611b412f7c2e1603866e0f7f1b/Lib/unittest/suite.py#L108)

::: testing/marionette/harness/session/session_test.py:226
(Diff revision 2)
> -        # Convert the marionette weakref to an object, just for the
> -        # duration of the test; this is deleted in tearDown() to prevent
> -        # a persistent circular reference which in turn would prevent
> -        # proper garbage collection.
>          self.start_time = time.time()
> -        self.marionette = self._marionette_weakref()
> +        self.marionette = Marionette(bin=self.binary, profile=self.profile); #AR

FYI, if you pass in profile=None, Marionette will take care of profile setup (also using mozprofile.Profile) -- perhaps you prefer to take advantage of that.

Comment 97

[André Reinald]

https://reviewboard.mozilla.org/r/49525/#review47857

> stopTest is called at the end of each test method, I believe, and when self.shouldStop is set to True, no more tests in the current module will be run, so you should remove this line. (See: https://github.com/python/cpython/blob/2410a06c294d9b091f39f09f638c1d83d0ff440e/Lib/unittest/case.py#L584 and https://github.com/python/cpython/blob/2d264235f6e066611b412f7c2e1603866e0f7f1b/Lib/unittest/suite.py#L108)

Hum… I wasn't sure here indeed. Ok, corrected.

> FYI, if you pass in profile=None, Marionette will take care of profile setup (also using mozprofile.Profile) -- perhaps you prefer to take advantage of that.

I really need to tweak the profile before I start a telemetry test, but I'll do that in a future patch, or even in a future bug. So leaving it like it is now.

Comment 98

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 99

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

https://reviewboard.mozilla.org/r/49525/#review47857

> Hum… I wasn't sure here indeed. Ok, corrected.

Sorry, I meant that you should just omit the assignment to shouldStop altogether.

Comment 100

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 101

[André Reinald]

https://reviewboard.mozilla.org/r/49525/#review47857

> Sorry, I meant that you should just omit the assignment to shouldStop altogether.

Ok, changed!

Comment 102

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

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

https://reviewboard.mozilla.org/r/49525/#review48845

Ok, I think that's everything. :) Please flag AutomatedTester for review as well -- I've read this patch so many times that I may have missed something. A second pair of eye would be nice.

Comment 103

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

Following Maja's suggestion, asking for additional review.

Comment 104

[David Burns :automatedtester]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

https://reviewboard.mozilla.org/r/49525/#review50126

::: testing/marionette/harness/session/session_test.py:226
(Diff revision 4)
> -        # Convert the marionette weakref to an object, just for the
> -        # duration of the test; this is deleted in tearDown() to prevent
> -        # a persistent circular reference which in turn would prevent
> -        # proper garbage collection.
>          self.start_time = time.time()
> -        self.marionette = self._marionette_weakref()
> +        self.marionette = Marionette(bin=self.binary, profile=self.profile); #AR

`#AR` ?

::: testing/marionette/mach_commands.py:80
(Diff revision 4)
> +def setup_session_argument_parser():
> +    from session.runner.base import BaseSessionArguments
> +    return BaseSessionArguments()
> +
> +def run_session(tests, testtype=None,
> +    binary=None, topsrcdir=None, **kwargs):

Could you move this to the same line as a method definition. It makes it hard to read the next line.

Comment 105

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 106

[André Reinald]

https://reviewboard.mozilla.org/r/49525/#review50126

> `#AR` ?

fixed

> Could you move this to the same line as a method definition. It makes it hard to read the next line.

fixed

Comment 107

[Carsten Book [:Tomcat]]

failed to apply:

applying 258d9c3425ad
patching file testing/marionette/harness/session/__init__.py
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file testing/marionette/harness/session/__init__.py.rej
patching file testing/marionette/harness/session/runner/base.py
Hunk #4 FAILED at 236
Hunk #9 FAILED at 457
Hunk #11 FAILED at 951
Hunk #12 succeeded at 973 with fuzz 2 (offset -24 lines).
3 out of 14 hunks FAILED -- saving rejects to file testing/marionette/harness/session/runner/base.py.rej
patching file testing/marionette/harness/session/session_test.py
Hunk #1 FAILED at 8
Hunk #6 FAILED at 641
2 out of 6 hunks FAILED -- saving rejects to file testing/marionette/harness/session/session_test.py.rej
patching file testing/marionette/harness/session/tests/unit-tests.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file testing/marionette/harness/session/tests/unit-tests.ini.rej
patching file testing/marionette/mach_commands.py
Hunk #1 FAILED at 22
1 out of 3 hunks FAILED -- saving rejects to file testing/marionette/mach_commands.py.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 108

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 109

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 110

[André Reinald]

(In reply to Carsten Book [:Tomcat] from comment #107)
> failed to apply:

I'm not sure how those failures could happen, I could not reproduce them.

But I realized the underlying code has gone through important changes, and I decided to start over from those current sources, then "copy + change" them. That takes this patch back to the wip status.

Comment 111

[André Reinald]

Comment on attachment 8732933 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - copy; r?maja_zf

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

Comment 112

[André Reinald]

Comment on attachment 8746664 [details]
MozReview Request: Bug 1191324 - Extend Marionette to allow automation of telemetry tests - changes; r?[e-mail]

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

Comment 113

[André Reinald]

(In reply to Carsten Book [:Tomcat] from comment #107)
> failed to apply:

Hope the patches will apply this time.

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9aca0e22adb5
Extend Marionette to allow automation of telemetry tests - copy; r=maja_zf
https://hg.mozilla.org/integration/fx-team/rev/e876e86ee17d
Extend Marionette to allow automation of telemetry tests - changes; r=automatedtester

Comment 115

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9aca0e22adb5
https://hg.mozilla.org/mozilla-central/rev/e876e86ee17d

Comment 116

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9aca0e22adb5
https://hg.mozilla.org/mozilla-central/rev/e876e86ee17d