Closed
Bug 956739
Opened 10 years ago
Closed 10 years ago
Move marionette tests to structured logging
Categories
(Remote Protocol :: Marionette, defect)
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
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → james
Reporter | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
I'm going to take a stab at getting this landed.
Assignee: james → ahalberstadt
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
I don't think autolog support is used or needed anymore.. Malini, would it be terrible if we just removed it?
Flags: needinfo?(mdas)
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: ahalberstadt → cmanchester
Assignee | ||
Updated•10 years ago
|
Attachment #8432535 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8433609 -
Flags: feedback?(james)
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Blocks: structured-logging
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
A try run that will actually run marionette: https://tbpl.mozilla.org/?tree=Try&rev=6d3bc998a80e
Assignee | ||
Comment 11•10 years ago
|
||
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?
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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?
Updated•10 years ago
|
Keywords: ateam-marionette-task
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8433609 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8440811 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
(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
Assignee | ||
Updated•10 years ago
|
Attachment #8440942 -
Flags: review?(mdas)
Assignee | ||
Comment 18•10 years ago
|
||
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.
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8440942 -
Attachment is obsolete: true
Reporter | ||
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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-
Assignee | ||
Comment 23•10 years ago
|
||
(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!
Assignee | ||
Comment 24•10 years ago
|
||
This addresses review comments. Recent try run here: https://tbpl.mozilla.org/?tree=Try&rev=109025885b1a
Attachment #8443457 -
Flags: review?(mdas)
Assignee | ||
Updated•10 years ago
|
Attachment #8442498 -
Attachment is obsolete: true
Comment 25•10 years ago
|
||
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-
Comment 26•10 years ago
|
||
(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" :)
Comment 27•10 years ago
|
||
This patch will have to be updated since setup.py will be changed after Bug 1028254 lands
Updated•10 years ago
|
Whiteboard: [affects=marionette]
Assignee | ||
Comment 28•10 years ago
|
||
Ok, this should be current and compliant with recent version bumps.
Attachment #8445169 -
Flags: review?(mdas)
Assignee | ||
Updated•10 years ago
|
Attachment #8443457 -
Attachment is obsolete: true
Comment 29•10 years ago
|
||
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+
Assignee | ||
Comment 30•10 years ago
|
||
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
Assignee | ||
Comment 31•10 years ago
|
||
Try links: https://tbpl.mozilla.org/?tree=Try&rev=702c82166ed7 https://tbpl.mozilla.org/?tree=Try&rev=7730e1a6da4f
Comment 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c57f58a0ad
Keywords: checkin-needed
Comment 33•10 years ago
|
||
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
Reporter | ||
Comment 34•10 years ago
|
||
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 35•10 years ago
|
||
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.
Reporter | ||
Comment 36•10 years ago
|
||
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)?
Assignee | ||
Comment 37•10 years ago
|
||
Filed bug 1030845. Let's agree there on the requirements for a formatter. This will also block bug 886570.
Assignee | ||
Comment 38•10 years ago
|
||
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.
Assignee | ||
Comment 39•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8445169 -
Attachment is obsolete: true
Assignee | ||
Comment 40•10 years ago
|
||
Ed, would you mind signing off on the failure output in the try run from comment 39?
Flags: needinfo?(emorley)
Comment 41•10 years ago
|
||
(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)
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8449436 -
Flags: review?(mdas)
Attachment #8449436 -
Flags: feedback?(james)
Assignee | ||
Updated•10 years ago
|
Attachment #8449091 -
Attachment is obsolete: true
Attachment #8449091 -
Flags: review?(mdas)
Attachment #8449091 -
Flags: feedback?(james)
Assignee | ||
Comment 43•10 years ago
|
||
(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
Comment 44•10 years ago
|
||
lgtm with that tbplformatter.py change - thank you :-) (and sorry this has been such a pain...!)
Comment 45•10 years ago
|
||
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+
Assignee | ||
Comment 46•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8449436 -
Attachment is obsolete: true
Attachment #8449436 -
Flags: feedback?(james)
Assignee | ||
Comment 47•10 years ago
|
||
This moves the one-off formatter into marionette itself. Carrying r+ forward.
Assignee | ||
Updated•10 years ago
|
Attachment #8451398 -
Attachment is obsolete: true
Attachment #8451398 -
Flags: feedback?(james)
Assignee | ||
Updated•10 years ago
|
Attachment #8451792 -
Flags: review+
Assignee | ||
Comment 48•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a30a12d70e
https://hg.mozilla.org/mozilla-central/rev/53a30a12d70e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•