Closed Bug 1125098 Opened 9 years ago Closed 8 years ago

Remove XML report functionality from Marionette test runner

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox46 fixed, firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: davehunt, Assigned: ShrutiJ, Mentored)

References

Details

(Keywords: pi-marionette-runner, Whiteboard: [lang=py][good first bug])

Attachments

(1 file, 8 obsolete files)

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.
I'd like to play with this bug! Can it be assigned to me?
Flags: needinfo?(dave.hunt)
Assigned! Let me know if you need any help. :)
Assignee: nobody → s244sing
Flags: needinfo?(dave.hunt)
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)
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 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+
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+
Flags: needinfo?(s244sing)
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+
(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)
Minor fix, seems to have fixed the previous try commit failures.
Attachment #8554805 - Flags: review?(dave.hunt)
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+
collapsing patch to a single one.
Attachment #8554321 - Attachment is obsolete: true
Attachment #8554805 - Attachment is obsolete: true
Attachment #8555483 - Flags: review?(dave.hunt)
(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.
(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.
The patch fails to apply for me. Please update it and run a new try build.
Flags: needinfo?(s244sing)
(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)
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-
Flags: needinfo?(s244sing)
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)
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-
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)
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
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-
Simarpreet,

are you able to finish off this bug?
Flags: needinfo?(s244sing)
Hi David,

I'm afraid I wouldn't be able to come back to this for a while.
Flags: needinfo?(s244sing)
Unassigning for someone else to pick this up.
Assignee: s244sing → nobody
Were the changes made by Simarpreet implemented or does this need to be completely redone? I'd like to take a stab at this.
(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.
Assignee: nobody → glouisedell
I apologize but I can no longer work on this.
Assignee: glouisedell → nobody
Hi, I would like to work on this bug.
(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.
OS: Mac OS X → All
Hardware: x86 → All
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?
(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.
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.
Flags: needinfo?(dave.hunt)
Attached patch bug1125098.diff (obsolete) — Splinter Review
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 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+
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-
Attached patch bug1125098.diff (obsolete) — Splinter Review
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)
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)
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! :)
Attached patch bug1125098.diffSplinter Review
Attachment #8715170 - Attachment is obsolete: true
Attachment #8716579 - Flags: review?(mjzffr)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/28adba6d7e46
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(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)
(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)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.