Closed Bug 956739 Opened 10 years ago Closed 10 years ago

Move marionette tests to structured logging

Categories

(Remote Protocol :: Marionette, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: jgraham, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-task, Whiteboard: [affects=marionette])

Attachments

(1 file, 11 obsolete files)

43.99 KB, patch
chmanchester
: review+
Details | Diff | Splinter Review
Moving tests over to structured logging has a number of advantages, notably that it becomes possible to separate running the tests from formatting the output, which greatly reduces the work that each test harness has to do.

Marionette is a nice place to start for this kind of refactoring because it is relatively simple and self-contained. I have a prototype at https://bitbucket.org/jgraham/mozilla-central-patches/src/tip/structured_demo
Assignee: nobody → james
Attached patch Work in progress (obsolete) — Splinter Review
Attached a work in progress, based on the patch in bug 956738. Does a few things:

* Removes all the result collection and reporting code from the marionette test runner

* Adds structured logging, based on the generic unittest adapter

* Rewrites the command line handling and the mach command to be in terms of argparse so that it can use the structured logging stuff

* Breaks some stuff including, but not limited to, autolog support.
I'm going to take a stab at getting this landed.
Assignee: james → ahalberstadt
Status: NEW → ASSIGNED
I don't think autolog support is used or needed anymore.. Malini, would it be terrible if we just removed it?
Flags: needinfo?(mdas)
No, feel free to remove autolog support.
Flags: needinfo?(mdas)
Attached patch structured_log (obsolete) — Splinter Review
I took this a bit further, but not much. This is probably slightly less bit-rotted than the old patch.
Attachment #8361099 - Attachment is obsolete: true
Unbitrot of original patch, port cli to argparse.

I expect this still needs a lot of work, but I'm running marionette's
own tests locally with this patch applied. In the meantime, James, do
I have the right idea with this use of argparse?
Assignee: ahalberstadt → cmanchester
Attachment #8432535 - Attachment is obsolete: true
Attachment #8433609 - Flags: feedback?(james)
Comment on attachment 8433609 [details] [diff] [review]
Move marionette tests to structured logging.;

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

Yep, the use of argparse looks right to me (at least the structured log integration, I didn't read it all in detail).
Attachment #8433609 - Flags: feedback?(james) → feedback+
Try run to see what tbpl does in response to a marionette failure so we can make sure to do something compatible:

https://tbpl.mozilla.org/?tree=Try&rev=92f899a0e481
As a part of this bug, is there any objection to modifying marionette's output to use the tbpl format in any cases a human wants to read the log (and to talk to tbpl's parsers in the short term), rather than the output format it currently uses, which is based on the unittest library?
By output, you mean test failures?

I don't have any objection myself.  We'll need to preserve stack traces, of course.  We should also get the output reviewed by WebQA, since they're a big consumer of Marionette and might have an opinion.
Comment on attachment 8433609 [details] [diff] [review]
Move marionette tests to structured logging.;

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

::: testing/marionette/client/marionette/runner/base.py
@@ -247,5 @@
> -            # this tells unittest.TestSuite not to continue running tests
> -            self.shouldStop = True
> -
> -
> -class MarionetteTextTestRunner(unittest.TextTestRunner):

A search for "MarionetteTextTestRunner" gets 117 hits on GitHub. Is removing this a viable option, or am I missing something here?

What's a rule of thumb I can use for figuring out what's changeable and what's not?
The rationale here is that the mozlog.structured module will subsume the reporting / output formatting provided by some of the mixins in marionette/runner/mixins, but the parts concerned with getting debug info into the log have to be preserved. It looks like we still need to bump the version with this patch until consumers are using the structured module for output. Does that make sense?

This patch includes some hacks to the tbplformatter to appease mozharness and tbpl; these wont be strictly necessary if bug 1024055 can land ahead of this.

Runs fine with mach, although I think we'll want to change the machformatter to be less noisy for tests with a single thread and no subtests.
Attachment #8440811 - Flags: feedback?(ahalberstadt)
Attachment #8433609 - Attachment is obsolete: true
Comment on attachment 8440811 [details] [diff] [review]
Move marionette tests to structured logging.

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

Looks reasonable to me! Though I can't comment on what this might break that's out of tree.

::: testing/marionette/client/marionette/runner/base.py
@@ +32,5 @@
> +    def _log_b2g_debug(self, output):
> +        if "Broken pipe" in output or "Connection timed out" in output:
> +            b2g_debug_output = B2GTestResultMixin.diagnose_socket(self)
> +            if b2g_debug_output:
> +                self.logger.error(b2g_debug_output)

Shouldn't this be defined in B2GTestResultMixin? (I don't actually know.. just looks like it might).

@@ +39,5 @@
> +        output = self._exc_info_to_string(err, test)
> +        if self.b2g_pid:
> +            self._log_b2g_debug(output)
> +        self.errors.append((test, output))
> +        super(MarionetteTestResult, self).addError(test, err)

Same as above, this contains b2g specific things.

@@ +46,5 @@
> +        output = self._exc_info_to_string(err, test)
> +        if self.b2g_pid:
> +            self._log_b2g_debug(output)
> +        self.failures.append((test, output))
> +        super(MarionetteTestResult, self).addFailure(test, err)

Same as above.
Attachment #8440811 - Flags: feedback?(ahalberstadt) → feedback+
This has the version bump and some modifications to the machformatter. Do gaia unit tests always get the in-tree marionette, or can we pin the version? This didn't do the trick: https://tbpl.mozilla.org/?tree=Try&rev=fc3ae6fb44aa

mdas, you looked like the right person to review the runner code, but let me know if that's not the case. Also, could you comment on ahal's question above? It looks to me like the B2GResultMixin is always added in by the constructor based on its argument, so this seemed like a reasonable equivalent way to get debug output without relying on the TestResultCollection functionality.
Attachment #8440942 - Flags: review?(mdas)
Attachment #8440811 - Attachment is obsolete: true
(In reply to Chris Manchester [:chmanchester] from comment #16)
> Created attachment 8440942 [details] [diff] [review]
> Move marionette tests to structured logging.
> 
> This has the version bump and some modifications to the machformatter. Do
> gaia unit tests always get the in-tree marionette, or can we pin the
> version? This didn't do the trick:
> https://tbpl.mozilla.org/?tree=Try&rev=fc3ae6fb44aa
> 
This does use in-tree marionette.

It looks like we need to fix HTMLReportingTestRunnerMixin at http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/mixins/reporting.py?force=1#43
The structured module provides things like html generation, so this will become a bit redundant, but it looks like we need to maintain compatibility during the transition. 

Unfortunately I think it will make removing the old runner and sharing code with the moztest adapter impossible, but I can work on a compatible version.
(In reply to Chris Manchester [:chmanchester] from comment #16)
> Created attachment 8440942 [details] [diff] [review]
> Move marionette tests to structured logging.
> 
> This has the version bump and some modifications to the machformatter. Do
> gaia unit tests always get the in-tree marionette, or can we pin the
> version? This didn't do the trick:
> https://tbpl.mozilla.org/?tree=Try&rev=fc3ae6fb44aa
> 
> mdas, you looked like the right person to review the runner code, but let me
> know if that's not the case. Also, could you comment on ahal's question
> above? It looks to me like the B2GResultMixin is always added in by the
> constructor based on its argument, so this seemed like a reasonable
> equivalent way to get debug output without relying on the
> TestResultCollection functionality.

As strange as it seems, it's by design. It's from https://bugzilla.mozilla.org/show_bug.cgi?id=975169, so we dynamically mix in the mixin depending on the environment.
This is a more conservative approach to integrating structured logging, that appears to succeed in maintaining compatibility.

Recent try: https://tbpl.mozilla.org/?tree=Try&rev=498199425528
Attachment #8442498 - Flags: review?(mdas)
Attachment #8442498 - Flags: feedback?(james)
Attachment #8440942 - Attachment is obsolete: true
Comment on attachment 8442498 [details] [diff] [review]
Move marionette tests to structured logging.

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

So it is slightly disappointing that we didn't manage to remove more code here. For example, all the HTML report stuff seems to still be in place, and we still seem to have an almost complete MarionetteTestResult class on top of the StructuredTestResult class. I am assuming that there are good reasons for this, but we should file bugs for more cleanup in the future.

::: testing/marionette/client/setup.py
@@ +1,5 @@
>  import os
>  from setuptools import setup, find_packages
>  import sys
>  
> +version = '0.7.10'

I think this should perhaps be version 0.8.

::: testing/mozbase/mozlog/mozlog/structured/formatters/tbplformatter.py
@@ +20,5 @@
>          if data.get('component'):
>              return "%s %s\n" % (data["component"], data["message"])
>  
> +        if data.get("level") and data["level"] == "ERROR":
> +            return "TEST-UNEXPECTED-FAIL | %s \n" % data["message"]

This seems very odd. Why is an error-level log message output as a test fail? An ERROR is enough to fail the build on tbpl anyway (and certainly is with the mozharness changes).

@@ +50,5 @@
>          start_time = self.test_start_times.pop(self.test_id(data["test"]))
>          time = data["time"] - start_time
>  
>          if "expected" in data:
> +            return "TEST-UNEXPECTED-%s TEST-END | %s | expected %s | %s | took %ims\n" % (

I think I have a slight variation of this change in my tree. But this is fine to start with.
Attachment #8442498 - Flags: feedback?(james) → feedback+
Comment on attachment 8442498 [details] [diff] [review]
Move marionette tests to structured logging.

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

This is a good start, and thanks for removing the autolog bits!

The problem is that I can't run runtests.py with this patch applied. As an example:

$ python runtests.py  --address=localhost:2828 --binary=/Applications/FirefoxNightly.app/Contents/MacOS/firefox tests/unit/test_log.py

Gives me the following traceback:

Traceback (most recent call last):
  File "runtests.py", line 38, in <module>
    cli()
  File "runtests.py", line 25, in cli
    structured.commandline.add_logging_group(parser)
  File "/Users/mdas/Code/m-c/testing/marionette/client/venv/lib/python2.7/site-packages/mozlog-1.6-py2.7.egg/mozlog/structured/commandline.py", line 40, in add_logging_group
    group = parser.add_argument_group("Output Logging",
AttributeError: BaseMarionetteOptions instance has no attribute 'add_argument_group'

Also, for the next patch, can you run the Mn and Gu tests against it? Gu (gaia-ui-tests) consume the test runners that are being changed, so I'd like to make sure they run without issue.

::: testing/marionette/client/marionette/runner/base.py
@@ +479,5 @@
>                   testvars=None, tree=None, type=None, device_serial=None,
> +                 symbols_path=None, timeout=None, shuffle=False,
> +                 shuffle_seed=random.randint(0, sys.maxint),
> +                 sdcard=None, this_chunk=1, total_chunks=1, sources=None,
> +                 server_root=None, gecko_log=None, include_debug=True,

I think this should be False, we don't want to save screenshots by default

@@ +522,5 @@
>          self.gecko_log = gecko_log
>          self.mixin_run_tests = []
>          self.manifest_skipped_tests = []
>          self.tests = []
> +        self.suite_name = "MarionetteUnitTests"

where is suite_name used? If it does get used somewhere, this string should be "Marionette Tests" so it's more general, since other harnesses use the vanilla runner

@@ +1001,5 @@
>  
>          doc.appendChild(testsuite)
>          return doc.toprettyxml(encoding='utf-8')
>  
> +    def debug_callback(self):

Why do we have this function? will it be replacing something else currently in use, like for HTML reports?

::: testing/marionette/client/setup.py
@@ +1,5 @@
>  import os
>  from setuptools import setup, find_packages
>  import sys
>  
> +version = '0.7.10'

yup. 0.8 would be best
Attachment #8442498 - Flags: review?(mdas) → review-
(In reply to Malini Das [:mdas] from comment #22)
> Comment on attachment 8442498 [details] [diff] [review]
> Move marionette tests to structured logging.
> 
> Review of attachment 8442498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a good start, and thanks for removing the autolog bits!
> 
> The problem is that I can't run runtests.py with this patch applied. As an
> example:
> 
> $ python runtests.py  --address=localhost:2828
> --binary=/Applications/FirefoxNightly.app/Contents/MacOS/firefox
> tests/unit/test_log.py
> 
> Gives me the following traceback:
> 
> Traceback (most recent call last):
>   File "runtests.py", line 38, in <module>
>     cli()
>   File "runtests.py", line 25, in cli
>     structured.commandline.add_logging_group(parser)
>   File
> "/Users/mdas/Code/m-c/testing/marionette/client/venv/lib/python2.7/site-
> packages/mozlog-1.6-py2.7.egg/mozlog/structured/commandline.py", line 40, in
> add_logging_group
>     group = parser.add_argument_group("Output Logging",
> AttributeError: BaseMarionetteOptions instance has no attribute
> 'add_argument_group'

This works ok for me, are you running a version of mozlog with bug 1021931 fixed?

> 
> Also, for the next patch, can you run the Mn and Gu tests against it? Gu
> (gaia-ui-tests) consume the test runners that are being changed, so I'd like
> to make sure they run without issue.

Will do. I'll post a new try run this evening.

> 
> ::: testing/marionette/client/marionette/runner/base.py
> @@ +479,5 @@
> >                   testvars=None, tree=None, type=None, device_serial=None,
> > +                 symbols_path=None, timeout=None, shuffle=False,
> > +                 shuffle_seed=random.randint(0, sys.maxint),
> > +                 sdcard=None, this_chunk=1, total_chunks=1, sources=None,
> > +                 server_root=None, gecko_log=None, include_debug=True,
> 
> I think this should be False, we don't want to save screenshots by default
> 
> @@ +522,5 @@
> >          self.gecko_log = gecko_log
> >          self.mixin_run_tests = []
> >          self.manifest_skipped_tests = []
> >          self.tests = []
> > +        self.suite_name = "MarionetteUnitTests"
> 
> where is suite_name used? If it does get used somewhere, this string should
> be "Marionette Tests" so it's more general, since other harnesses use the
> vanilla runner
> 

Removed.

> @@ +1001,5 @@
> >  
> >          doc.appendChild(testsuite)
> >          return doc.toprettyxml(encoding='utf-8')
> >  
> > +    def debug_callback(self):
> 
> Why do we have this function? will it be replacing something else currently
> in use, like for HTML reports?

The structured module will subsume HTML reporting, but for now I looked at this again and it wont work right as I have it here, I'll take this out.

> 
> ::: testing/marionette/client/setup.py
> @@ +1,5 @@
> >  import os
> >  from setuptools import setup, find_packages
> >  import sys
> >  
> > +version = '0.7.10'
> 
> yup. 0.8 would be best

Done. Thanks mdas!
This addresses review comments. Recent try run here: https://tbpl.mozilla.org/?tree=Try&rev=109025885b1a
Attachment #8443457 - Flags: review?(mdas)
Attachment #8442498 - Attachment is obsolete: true
Comment on attachment 8443457 [details] [diff] [review]
Move marionette tests to structured logging.

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

Ah, so I was unable to run this locally because mozlog was not updated in pypi. I've filed bug 1028239 about it. Btw, the tbpl tests passed because they use intree packages.

Sorry, one thing I forgot from last patch review, when bug 1028239 lands, can you add 'mozlog >=0.2' to the requirements.txt file in testing/marionette/client/requirements.txt? Other than that, this looks perfectly and works when I use in-tree mozbase.
Attachment #8443457 - Flags: review?(mdas) → review-
(In reply to Malini Das [:mdas] from comment #25)
> Comment on attachment 8443457 [details] [diff] [review]
> Other than that, this looks
> perfectly and works when I use in-tree mozbase.

Haha. I mean: "This looks good and works perfectly when I use in-tree mozbase" :)
This patch will have to be updated since setup.py will be changed after Bug 1028254 lands
Whiteboard: [affects=marionette]
Ok, this should be current and compliant with recent version bumps.
Attachment #8445169 - Flags: review?(mdas)
Attachment #8443457 - Attachment is obsolete: true
Comment on attachment 8445169 [details] [diff] [review]
Move marionette tests to structured logging.

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

Great, thanks!

This can land in-tree, but I can't release a new client until Bug 1028254 lands. We need all mozbase dependencies to be available via pypi.
Attachment #8445169 - Flags: review?(mdas) → review+
There was an osx crash in Gu on a recent try push, but I tried again this morning and it looks like it was unrelated.
Keywords: checkin-needed
This made very TBPL-unfriendly changes to the failure output from the Marionette harness. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e5168bf8af
So AFAICT we just need to make sure that the exception appears on the first line, before the full traceback so that something that's logged with this patch as:

08:39:42    ERROR -  TEST-UNEXPECTED-ERROR TEST-END | test_single_finger_desktop.testSingleFingerMouse.test_chain | expected PASS | Traceback (most recent call last):
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\marionette\marionette_test.py", line 170, in run
08:39:42     INFO -      testMethod()
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\tests\testing\marionette\client\marionette\tests\unit\test_single_finger_desktop.py", line 100, in test_chain
08:39:42     INFO -      chain(self.marionette, self.wait_for_condition, "button1-mousemove-mousedown", "delayed-mousemove-mouseup")
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\tests\testing\marionette\client\marionette\tests\unit\single_finger_functions.py", line 46, in chain
08:39:42     INFO -      wait_for_condition(lambda m: expected2 in m.execute_script("return document.getElementById('delayed').innerHTML;"))
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\marionette\marionette_test.py", line 375, in wait_for_condition
08:39:42     INFO -      raise TimeoutException("wait_for_condition timed out")
08:39:42     INFO -  TimeoutException: TimeoutException: wait_for_condition timed out
08:39:42     INFO -   | took 35314ms

is instead logged as:

08:39:42    ERROR -  TEST-UNEXPECTED-ERROR TEST-END | test_single_finger_desktop.testSingleFingerMouse.test_chain | expected PASS | TimeoutException: TimeoutException: wait_for_condition timed out
08:39:42     INFO -    Traceback (most recent call last):
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\marionette\marionette_test.py", line 170, in run
08:39:42     INFO -      testMethod()
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\tests\testing\marionette\client\marionette\tests\unit\test_single_finger_desktop.py", line 100, in test_chain
08:39:42     INFO -      chain(self.marionette, self.wait_for_condition, "button1-mousemove-mousedown", "delayed-mousemove-mouseup")
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\tests\testing\marionette\client\marionette\tests\unit\single_finger_functions.py", line 46, in chain
08:39:42     INFO -      wait_for_condition(lambda m: expected2 in m.execute_script("return document.getElementById('delayed').innerHTML;"))
08:39:42     INFO -    File "C:\slave\test\build\tests\marionette\marionette\marionette_test.py", line 375, in wait_for_condition
08:39:42     INFO -      raise TimeoutException("wait_for_condition timed out")
08:39:42     INFO -  TimeoutException: TimeoutException: wait_for_condition timed out
08:39:42     INFO -   | took 35314ms

(I also note that giving a test that raises a TimeoutException a status of ERROR rather than TIMEOUT seems pretty broken)
Comment on attachment 8445169 [details] [diff] [review]
Move marionette tests to structured logging.

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

::: testing/mozbase/mozlog/mozlog/structured/formatters/tbplformatter.py
@@ +47,5 @@
>          start_time = self.test_start_times.pop(self.test_id(data["test"]))
>          time = data["time"] - start_time
>  
>          if "expected" in data:
> +            return "TEST-UNEXPECTED-%s TEST-END | %s | expected %s | %s | took %ims\n" % (

There needs to be just three parts delimited by the pipe symbol.
Comment on attachment 8445169 [details] [diff] [review]
Move marionette tests to structured logging.

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

::: testing/mozbase/mozlog/mozlog/structured/formatters/tbplformatter.py
@@ +47,5 @@
>          start_time = self.test_start_times.pop(self.test_id(data["test"]))
>          time = data["time"] - start_time
>  
>          if "expected" in data:
> +            return "TEST-UNEXPECTED-%s TEST-END | %s | expected %s | %s | took %ims\n" % (

What, exactly, should those three parts be? TEST-UNEXPECTED-FOO | test_id | failure string? Where "failure string" has testsuite-specific requirements? Presumably it will be OK to output a second line with the extra info (the actual expected and the duration)?
Filed bug 1030845. Let's agree there on the requirements for a formatter. This will also block bug 886570.
To give an update: while working on satisfying the requirement for starring on tbpl that test name string in logs match the old strings, I noticed that logs for js vs. python files are a bit different (python puts "<test file name> <test identifier>" in the middle field of the line, while javascript puts "<test file name>" in this position). A closer look at this made me realize that this patch doesn't address the unstructured logging marionette's javascript uses to log failures and errors. Because this impacts the way failures are logged, I don't think there is a coherent iteration of this that doesn't convert the logging in javascript as well.

So, this is further from finished than I thought, although I can't say exactly how far.
The additional changes ended up being minimal. This logs javascript test failures as test_status messages as they occur, and puts additional string manipulation to make error lines compatible with tbpl in a formatter specifically for that purpose. I put this in the mozlog package, but it might be better placed in the harness itself.

Here is a hung/failing run on try with this patch applied:
https://tbpl.mozilla.org/?tree=Try&rev=512837129843

And without this patch applied:
https://tbpl.mozilla.org/?tree=Try&rev=7f0ed15849fd
Attachment #8449091 - Flags: review?(mdas)
Attachment #8449091 - Flags: feedback?(james)
Attachment #8445169 - Attachment is obsolete: true
Ed, would you mind signing off on the failure output in the try run from comment 39?
Flags: needinfo?(emorley)
(In reply to Chris Manchester [:chmanchester] from comment #40)
> Ed, would you mind signing off on the failure output in the try run from
> comment 39?

Sure :-)

The main failure line looks fine (more concise than the original, which is great), however we seem to have regressed bug 1006511, in that with the patch, we get the "AssertionError: False is not true" on it's own line, that hits the log parser regex for Python exceptions, and will cause false positives on TBPL.
Flags: needinfo?(emorley)
Attachment #8449436 - Flags: review?(mdas)
Attachment #8449436 - Flags: feedback?(james)
Attachment #8449091 - Attachment is obsolete: true
Attachment #8449091 - Flags: review?(mdas)
Attachment #8449091 - Flags: feedback?(james)
(In reply to Ed Morley [:edmorley UTC+0] from comment #41)
> (In reply to Chris Manchester [:chmanchester] from comment #40)
> > Ed, would you mind signing off on the failure output in the try run from
> > comment 39?
> 
> Sure :-)
> 
> The main failure line looks fine (more concise than the original, which is
> great), however we seem to have regressed bug 1006511, in that with the
> patch, we get the "AssertionError: False is not true" on it's own line, that
> hits the log parser regex for Python exceptions, and will cause false
> positives on TBPL.

Ok, that makes sense. This version will prevent the exception from getting printed twice. A failed test looks like this:

TEST-UNEXPECTED-FAIL | test_capabilities.py TestCapabilities.test_mandates_capabilities | AssertionError: False is not true
Traceback (most recent call last):
  File "/home/cmanchester/m-c/testing/marionette/client/marionette/marionette_test.py", line 171, in run
    testMethod()
  File "/home/cmanchester/m-c/testing/marionette/client/marionette/tests/unit/test_capabilities.py", line 16, in test_mandates_capabilities
    self.assertTrue(False)
TEST-INFO expected PASS | took 43ms
lgtm with that tbplformatter.py change - thank you :-)

(and sorry this has been such a pain...!)
Comment on attachment 8449436 [details] [diff] [review]
Move marionette tests to structured logging.

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

r+ if you update cli() and if it applies cleanly on master. Thanks!

::: testing/marionette/client/marionette/runtests.py
@@ +27,5 @@
>      parser.verify_usage(options, tests)
>  
> +    logger = structured.commandline.setup_logging("Marionette Unit Tests",
> +                                                  options,
> +                                                  {})

Can you add an optional parameter to cli called "log_name" or similar, and have it set to "Marionette-based Tests" by default, and use that as the log name here? This is because downstream consumers, like gaiatest here https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/runtests.py#L60, invoke cli() directly, and they should be able to set the name of the logger to something applicable to them.

::: testing/marionette/client/setup.py
@@ -1,5 @@
>  import os
>  from setuptools import setup, find_packages
>  import sys
>  
> -version = '0.7.10'

This has been updated to 0.7.11 so you'll need to update your patch.
Attachment #8449436 - Flags: review?(mdas) → review+
Thanks mdas! This is unbitrot and adds a commandline option for setting the logger name.

jgraham, I just wanted your feedback on how to approach ad-hoc formatters for mozlog.structured. I didn't want to put my one-off in the short name dict used by the structured commandline (it's not really suitable for general consumption), but this ends up a bit of a hack.
Attachment #8451398 - Flags: feedback?(james)
Attachment #8449436 - Attachment is obsolete: true
Attachment #8449436 - Flags: feedback?(james)
This moves the one-off formatter into marionette itself. Carrying r+ forward.
Attachment #8451398 - Attachment is obsolete: true
Attachment #8451398 - Flags: feedback?(james)
https://hg.mozilla.org/mozilla-central/rev/53a30a12d70e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1050178
Depends on: 1050170
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.