Closed
Bug 1125098
Opened 9 years ago
Closed 8 years ago
Remove XML report functionality from Marionette test runner
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox46 fixed, firefox47 fixed)
RESOLVED
FIXED
mozilla47
People
(Reporter: davehunt, Assigned: ShrutiJ, Mentored)
References
Details
(Keywords: pi-marionette-runner, Whiteboard: [lang=py][good first bug])
Attachments
(1 file, 8 obsolete files)
18.16 KB,
patch
|
impossibus
:
review+
|
Details | Diff | Splinter Review |
The XUnit reports can now be generated by mozlog using the command line argument --log-xunit so we should remove this functionality from the Marionette test runner. This will involve: Removing the --xml-output command line option from BaseMarionetteOptions in https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/marionette/client/marionette/runner/base.py Removing references to the xml_output keyword argument from BaseMarionetteTestRunner in https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/marionette/client/marionette/runner/base.py#l472 Remove the code that generates the XML report from BaseMarionetteTestRunner in https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/marionette/client/marionette/runner/base.py#l990 Replace references to --xml-output with --log-xunit in https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/config/mozharness/marionette.py A separate patch will be needed for gaia to remove the reference to the XML report in the Treeherder mixin. This could be safely done ahead of the Marionette test runner changes: https://github.com/mozilla-b2g/gaia/blob/3a015d905fd45ddea735019865206a27844b3c0a/tests/python/gaia-ui-tests/gaiatest/mixins/treeherder.py#L226 We should also clean up any other references or related imports.
Comment 1•9 years ago
|
||
I'd like to play with this bug! Can it be assigned to me?
Flags: needinfo?(dave.hunt)
Reporter | ||
Comment 2•9 years ago
|
||
Assigned! Let me know if you need any help. :)
Assignee: nobody → s244sing
Flags: needinfo?(dave.hunt)
Updated•9 years ago
|
Keywords: ateam-marionette-runner
Comment 3•9 years ago
|
||
Adding the first patch that gets rid of --xml-output references from the test runner code.
Attachment #8554321 -
Flags: review?(dave.hunt)
Attachment #8554321 -
Flags: feedback?(dburns)
Comment 4•9 years ago
|
||
Adding the patch for gaia repo to remove xml_output references when generating reports for treeherder. This patch can also be found as a Pull Request on GitHub: https://github.com/mozilla-b2g/gaia/pull/27656
Attachment #8554324 -
Flags: review?(dave.hunt)
Comment 5•9 years ago
|
||
Try server commit: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2c72a464051a
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8554324 [details] [diff] [review] [PATCH]: Bug 1125098: Removing references to xml_output Review of attachment 8554324 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Could you submit it as a pull request for the Gaia repository so that we get a Gaia-Try run? Thanks.
Attachment #8554324 -
Flags: review?(dave.hunt) → review+
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8554321 [details] [diff] [review] [PATCH]: Bug 1125098: Remove --xml-output references. Review of attachment 8554321 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Your try run doesn't appear to have worked though - could you make sure you have the correct patch applied when pushing to try, and provide a new run.
Attachment #8554321 -
Flags: review?(dave.hunt) → review+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(s244sing)
Comment 8•9 years ago
|
||
Comment on attachment 8554321 [details] [diff] [review] [PATCH]: Bug 1125098: Remove --xml-output references. Review of attachment 8554321 [details] [diff] [review]: ----------------------------------------------------------------- Going with Dave Hunt's judgement here :)
Attachment #8554321 -
Flags: feedback?(dburns) → feedback+
Comment 9•9 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #7) > Comment on attachment 8554321 [details] [diff] [review] > [PATCH]: Bug 1125098: Remove --xml-output references. > > Review of attachment 8554321 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks great! Your try run doesn't appear to have worked though - could > you make sure you have the correct patch applied when pushing to try, and > provide a new run. Oops, I relied on the try-hook that the github gaia repo has, seems like it didn't pick up my changes. I've manually pushed to try, with all the needed tests here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b9d2f0a5bbf
Flags: needinfo?(s244sing)
Comment 10•9 years ago
|
||
Minor fix, seems to have fixed the previous try commit failures.
Attachment #8554805 -
Flags: review?(dave.hunt)
Comment 11•9 years ago
|
||
Try commit after last patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e73ea2f226
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8554805 [details] [diff] [review] [PATCH]: Bug 1125098: Adding results list back to fix test failure. Review of attachment 8554805 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Can you provide a combined patch for final review? Also, please submit your gaia patch as a pull request so that we have gaia-try results and can land that ahead of the test runner patch. ::: testing/marionette/client/marionette/runner/base.py @@ +914,4 @@ > if test_container: > self.launch_test_container() > > + self.results = [] I thought perhaps we might be able to remove this, but it looks like the HTML report is currently depending on it. We'll remove that in the future so this looks fine.
Attachment #8554805 -
Flags: review?(dave.hunt) → review+
Comment 13•9 years ago
|
||
collapsing patch to a single one.
Attachment #8554321 -
Attachment is obsolete: true
Attachment #8554805 -
Attachment is obsolete: true
Attachment #8555483 -
Flags: review?(dave.hunt)
Comment 14•9 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #12) > Comment on attachment 8554805 [details] [diff] [review] > [PATCH]: Bug 1125098: Adding results list back to fix test failure. > > Review of attachment 8554805 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Can you provide a combined patch for final review? Also, please > submit your gaia patch as a pull request so that we have gaia-try results > and can land that ahead of the test runner patch. > Here's a link: https://github.com/mozilla-b2g/gaia/pull/27656 > ::: testing/marionette/client/marionette/runner/base.py > @@ +914,4 @@ > > if test_container: > > self.launch_test_container() > > > > + self.results = [] > > I thought perhaps we might be able to remove this, but it looks like the > HTML report is currently depending on it. We'll remove that in the future so > this looks fine.
Reporter | ||
Comment 15•9 years ago
|
||
(In reply to Simarpreet Singh from comment #14) > Here's a link: https://github.com/mozilla-b2g/gaia/pull/27656 Thanks, I've landed this here: https://github.com/mozilla-b2g/gaia/commit/fe0d03f56dbca9edc6fa582399a898a0cc7f545c Let's get a try run on the combined m-c patch, then I can flag it to be checked in.
Reporter | ||
Comment 16•9 years ago
|
||
The patch fails to apply for me. Please update it and run a new try build.
Flags: needinfo?(s244sing)
Comment 17•9 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #16) > The patch fails to apply for me. Please update it and run a new try build. Hmm, that's odd. Either way, I updated the mc repo and ran another try-server commit here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a85e050c2bb5
Flags: needinfo?(s244sing)
Reporter | ||
Comment 18•9 years ago
|
||
Comment on attachment 8555483 [details] [diff] [review] [PATCH]: Bug 1125098: Remove --xml-output references. Review of attachment 8555483 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/client/marionette/runner/base.py @@ +914,4 @@ > if test_container: > self.launch_test_container() > > + self.results = [] Sorry for not noticing this earlier, but this will wipe out the results list on execution of each test. Please restore defining self.results in the __init__ method.
Attachment #8555483 -
Flags: review?(dave.hunt) → review-
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(s244sing)
Comment 19•9 years ago
|
||
Sorry got busy with school and midterms. This should take care of things. Try commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edea747f5281
Flags: needinfo?(s244sing)
Attachment #8562807 -
Flags: review?(dave.hunt)
Reporter | ||
Comment 20•9 years ago
|
||
Comment on attachment 8562807 [details] [diff] [review] [PATCH]: xml_ouptut: Fixing a misplaced list initializer. Review of attachment 8562807 [details] [diff] [review]: ----------------------------------------------------------------- This patch will only apply on top of the previous patch. Please create a single patch for mozilla-central. Also, please mark your existing patches as obsolete once you've attached your new one.
Attachment #8562807 -
Flags: review?(dave.hunt) → review-
Comment 21•9 years ago
|
||
Single collapsed patch. Try commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6d3a90c9c49
Attachment #8554324 -
Attachment is obsolete: true
Attachment #8555483 -
Attachment is obsolete: true
Attachment #8562807 -
Attachment is obsolete: true
Attachment #8562936 -
Flags: review?(dave.hunt)
Reporter | ||
Comment 22•9 years ago
|
||
Comment on attachment 8554324 [details] [diff] [review] [PATCH]: Bug 1125098: Removing references to xml_output This patch is not obsolete - it was checked in.
Attachment #8554324 -
Attachment is obsolete: false
Reporter | ||
Comment 23•9 years ago
|
||
Comment on attachment 8562936 [details] [diff] [review] [PATCH]: xml-output work combined patch. Review of attachment 8562936 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks. As you see there are try failures. Please address the comments and provide an updated patch. Be sure to attach a complete patch and only mark this one as obsolete. Also, can you set your commit message to "Bug 1125098 - Remove XML report functionality from Marionette test runner. r=dhunt". ::: testing/config/mozharness/marionette.py @@ +13,4 @@ > "--profile=%(profile)s", > "--symbols-path=%(symbols_path)s", > "--gecko-log=%(gecko_log)s", > + "--log-xunit=%(log_xunit)s", This change is causing failures. We'd need to change this in mozharness if we wanted to rename xml_output to log_xunit. For now I'd suggest retaining xml_output here. So the line would be: "--log-xunit=%(xml_output)s", @@ +32,4 @@ > "--testvars=%(testvars)s", > "--profile=%(profile)s", > "--symbols-path=%(symbols_path)s", > + "--log-xunit=%(log_xunit)s", Same issue as above. ::: testing/marionette/client/marionette/runner/base.py @@ +48,4 @@ > TestResultCollection.__init__(self, 'MarionetteTest') > self.passed = 0 > self.testsRun = 0 > + self.results = [] Why are we adding this to MarionetteTestResult? @@ +915,4 @@ > if test_container: > self.launch_test_container() > > + self.results = [] This is still resetting the list of results for every test. Please move this back into the __init__ method.
Attachment #8562936 -
Flags: review?(dave.hunt) → review-
Comment 25•9 years ago
|
||
Hi David, I'm afraid I wouldn't be able to come back to this for a while.
Flags: needinfo?(s244sing)
Reporter | ||
Comment 26•9 years ago
|
||
Unassigning for someone else to pick this up.
Assignee: s244sing → nobody
Comment 27•9 years ago
|
||
Were the changes made by Simarpreet implemented or does this need to be completely redone? I'd like to take a stab at this.
Reporter | ||
Comment 28•9 years ago
|
||
(In reply to glouisedell from comment #27) > Were the changes made by Simarpreet implemented or does this need to be > completely redone? I'd like to take a stab at this. Nothing landed for this bug so far. The patches needed a little more work, and now might be outdated. Feel free to start from scratch or to pick up the existing patches.
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → glouisedell
Comment 29•9 years ago
|
||
I apologize but I can no longer work on this.
Assignee: glouisedell → nobody
Assignee | ||
Comment 30•8 years ago
|
||
Hi, I would like to work on this bug.
Comment 31•8 years ago
|
||
(In reply to Shruti Jasoria [:ShrutiJ] from comment #30) > Hi, I would like to work on this bug. Let us know if you have any issues working on it.
Updated•8 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 32•8 years ago
|
||
I have a doubt regarding this bug
> Removing references to the xml_output keyword argument from
> BaseMarionetteTestRunner in
> https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/marionette/
> client/marionette/runner/base.py#l472
>
In this it has been asked to remove references to the xml_output argument. So do I need remove it entirely? It has been used in an if condtion, so I'll have to remove that condition block too, right?
Comment 33•8 years ago
|
||
(In reply to Shruti Jasoria [:ShrutiJ] from comment #32) > > Removing references to the xml_output keyword argument from > > BaseMarionetteTestRunner in > > https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/marionette/ > > client/marionette/runner/base.py#l472 > > In this it has been asked to remove references to the xml_output argument. > So do I need remove it entirely? It has been used in an if condtion, so I'll > have to remove that condition block too, right? Yes, remove all traces of it. I don’t think we need this at all.
Assignee | ||
Comment 34•8 years ago
|
||
i'm facing some problem. I can't find the file marionette.py https://hg.mozilla.org/mozilla-central/file/34e2d2bd7ec4/testing/config/mozharness/marionette.py in my copy of the repository. In fact, mozharness is not even a sub-directory of config. I have pulled the latest version, but it didn't help.
Updated•8 years ago
|
Flags: needinfo?(dave.hunt)
Reporter | ||
Comment 35•8 years ago
|
||
I found references to --xml-output in the following mozharness files: https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/marionette/automation_emulator_config.py https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/marionette/gaia_ui_test_prod_config.py https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/marionette/prod_config.py https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/marionette/test_config.py https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/configs/marionette/windows_config.py It's been a long time since I worked on mozharness, so I'm not sure if there's anywhere else to consider. A try run should highlight anything that's been missed though.
Flags: needinfo?(dave.hunt)
Assignee | ||
Comment 36•8 years ago
|
||
Here's the patch which get rids of xml_output functionality. I'll upload the one for gaia soon. Let me now if any other changes have to be made.
Attachment #8714785 -
Flags: feedback?(dburns)
Comment 37•8 years ago
|
||
Comment on attachment 8714785 [details] [diff] [review] bug1125098.diff Review of attachment 8714785 [details] [diff] [review]: ----------------------------------------------------------------- From my quick glance over this looks good. Please flag :maja_za for review.
Attachment #8714785 -
Flags: feedback?(dburns) → feedback+
Assignee | ||
Updated•8 years ago
|
Attachment #8714785 -
Flags: review?(mjzffr)
Assignee: nobody → shrutijasoria1996
Comment on attachment 8714785 [details] [diff] [review] bug1125098.diff Review of attachment 8714785 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/client/marionette/runner/base.py @@ +636,5 @@ > self.gecko_log = os.path.join(self.workspace_path or '', 'gecko.log') > else: > self.gecko_log = gecko_log > > + Nit: should have only one empty line above @property and make sure the empty line is actually empty (no tabs, spaces)
Attachment #8714785 -
Flags: review?(mjzffr) → review+
Thanks, Shruti. I have given r+, but please update your patch by addressing the above comment. I have pushed your current patch to the try server on your behalf. Take a look at the results and follow-up on any failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac6e56c6055
Comment on attachment 8714785 [details] [diff] [review] bug1125098.diff Review of attachment 8714785 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/client/marionette/runner/base.py @@ -641,5 @@ > self.gecko_log = gecko_log > > - # for XML output > - self.testvars['xml_output'] = self.xml_output > - self.results = [] As the try run points out, we should keep `self.results = []`
Attachment #8714785 -
Flags: review+ → review-
Assignee | ||
Comment 41•8 years ago
|
||
I've made the required changes. Also, how do I edit treeherder.py, the file which has to be edited for gaia? It's not there on the master branch anymore.
Attachment #8714785 -
Attachment is obsolete: true
Attachment #8715170 -
Flags: review?(mjzffr)
Assignee | ||
Comment 42•8 years ago
|
||
After searching a bit, I did find it in some of the branches. Like this one: https://github.com/mozilla-b2g/gaia/tree/bugfix/1171685-branch-2-2r-hash-device-id/tests/python/gaia-ui-tests/gaiatest/mixins So which on should I edit?
Flags: needinfo?(mjzffr)
Awesome, thanks for the quick update. You shouldn't need to edit anything in Github for this bug. Maybe I'm missing something -- could you explain how to arrived at the conclusion that you need to edit a file called treeherder.py? In the mean time, here's another try run for your latest patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d1a6d0ca0a4 -- Looks much greener so far :) I will review your patch later today or tomorrow.
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 44•8 years ago
|
||
Comment 0 says that i need to make changes to treeherder.py > > A separate patch will be needed for gaia to remove the reference to the XML > report in the Treeherder mixin. This could be safely done ahead of the > Marionette test runner changes: > https://github.com/mozilla-b2g/gaia/blob/ > 3a015d905fd45ddea735019865206a27844b3c0a/tests/python/gaia-ui-tests/gaiatest/ > mixins/treeherder.py#L226 > Let me know what else has to be mended for this bug.
Flags: needinfo?(mjzffr)
Right, I totally missed that, sorry. I will delegate your question to davehunt, since he originally mentioned treeherder.py a year ago -- maybe the gaia change is no longer relevant.
> Also, how do I edit treeherder.py, the file which has to be edited for gaia? It's not there on the master branch anymore.
Flags: needinfo?(mjzffr) → needinfo?(dave.hunt)
Comment on attachment 8715170 [details] [diff] [review] bug1125098.diff Review of attachment 8715170 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Just submit a correction to remove the extra whitespace, and then we'll check it in. ::: testing/marionette/client/marionette/runner/base.py @@ +1,1 @@ > + # This Source Code Form is subject to the terms of the Mozilla Public Nit: remove extra whitespace
Attachment #8715170 -
Flags: review?(mjzffr) → review+
Also please change your commit message to match the bug summary and include the name of the reviewer as follows: Bug 1125098 - Remove XML report functionality from Marionette test runner; r=maja_zf You can do so with 'hg histedit'. Thanks! :)
Assignee | ||
Comment 48•8 years ago
|
||
Attachment #8715170 -
Attachment is obsolete: true
Attachment #8716579 -
Flags: review?(mjzffr)
Reporter | ||
Comment 49•8 years ago
|
||
It looks like the Treeherder submission code was removed in bug 1230244, so there's nothing to do there any more.
Flags: needinfo?(dave.hunt)
Attachment #8562936 -
Attachment is obsolete: true
Attachment #8554324 -
Attachment is obsolete: true
Comment on attachment 8716579 [details] [diff] [review] bug1125098.diff Review of attachment 8716579 [details] [diff] [review]: ----------------------------------------------------------------- Yay, thanks Shruti! :) Next step: get your code checked in. You can do this by setting the 'checkin-needed' flag. I'll do it for you this time around. For the next bug you work on, consider using MozReview for reviews: * http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html * http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/firefoxworkflow.html
Attachment #8716579 -
Flags: review?(mjzffr) → review+
Keywords: checkin-needed
Comment 51•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/28adba6d7e46
Keywords: checkin-needed
Comment 52•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28adba6d7e46
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 54•8 years ago
|
||
(In reply to David Burns :automatedtester from comment #53) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92912e0981e I see that some tests have failed. Do I need to make any changes?
Flags: needinfo?(dburns)
Comment 55•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfea002b0ab4
status-firefox46:
--- → fixed
(In reply to Shruti Jasoria [:ShrutiJ] from comment #54) > (In reply to David Burns :automatedtester from comment #53) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92912e0981e > > I see that some tests have failed. Do I need to make any changes? No, the failures don't look related to your patch. Thanks for following-up.
Flags: needinfo?(dburns)
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
•