Let mozharness use ReftestFormatter when adapting structured output parser

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pyang, Assigned: pyang)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/output.py#13

ReftestFormatter should be referred by mozharness scripts and output reftest formatted result.

As log formatter it should be placed at https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/formatters/
(Assignee)

Updated

2 years ago
Blocks: 1261199
I just want to comment that this is a bit sticky. The theory is that mozlog doesn't have any code specific to any particular test harness in it. So currently, the ReftestFormatter is in the proper place.

However, because mozharness now needs to import this class, putting this in mozlog makes this easier to do. So we can either stick to the principle of mozlog being test harness agnostic, or we can make life easier on ourselves and just stick it there anyway. Personally I'm OK with putting it in mozlog, though CCing jgraham in case he has a different opinion.
Summary: Move ReftestFormatter bask into mozlog package → Move ReftestFormatter back into mozlog package
(Assignee)

Comment 2

2 years ago
Thanks, that make me think we can step back to original problem instead of diving into solution.
I try to have an explicit path to import reftest modules, but don't see similar usage in whole mozharness.

First thing I got is to move reftestformatter back into mozlog.  It is straight forward and but makes mozlog have suite specific code.
Otherwise we can get suite base dir such like https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/android_emulator_unittest.py#114
retrieve abs path and import on demand.  Need to add query base dir function. not my favor to dynamic import.

Either way we should import reftest directory and replace self.formatter manually.
Summary: Move ReftestFormatter back into mozlog package → Let mozharness use ReftestFormatter when adapting structured output parser
(Assignee)

Comment 3

2 years ago
Created attachment 8772702 [details] [diff] [review]
structured-log-using-reftestformatter

below is try result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63691c03d492fc4c7ede06da407ef121f900fadd

enable raw log emitting is also included.
Attachment #8772702 - Flags: feedback?(ahalberstadt)
(Assignee)

Updated

2 years ago
Assignee: nobody → pyang
(Assignee)

Comment 4

2 years ago
This patch was packaged with turning on structuredlog config, or we can tell whether it works.
We can still run tests for non-enabling case.
Comment on attachment 8772702 [details] [diff] [review]
structured-log-using-reftestformatter

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

Great, I like this approach a lot better than moving it into mozlog!

::: testing/mozharness/scripts/desktop_unittest.py
@@ +397,5 @@
>                      option = option % str_format_values
>                      if not option.endswith('None'):
>                          base_cmd.append(option)
> +                if self.structured_output(suite_category):
> +                    base_cmd.append("--log-raw=-")

This change is part of the other patch right?

@@ +640,5 @@
>                                                       log_obj=self.log_obj)
>  
> +                # modifying parser's parameters, such like output formatter
> +                if suite_category == "reftest":
> +                    reftest_output = (

No need for this variable, might as well just call the function directly.
Attachment #8772702 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 6

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Comment on attachment 8772702 [details] [diff] [review]
> structured-log-using-reftestformatter
> 
> Review of attachment 8772702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, I like this approach a lot better than moving it into mozlog!
> 
> ::: testing/mozharness/scripts/desktop_unittest.py
> @@ +397,5 @@
> >                      option = option % str_format_values
> >                      if not option.endswith('None'):
> >                          base_cmd.append(option)
> > +                if self.structured_output(suite_category):
> > +                    base_cmd.append("--log-raw=-")
> 
> This change is part of the other patch right?

yes, it's common section for all structured log patch.  in mochitest patch we modified function signature and make it not minimal workable section(flavor related not necessary for reftest IMO).   mochitest patch still need time to work, in the meantime probably we can land this with reftest. 
Do you think we should move it to another bug and block all those bugs?


> 
> @@ +640,5 @@
> >                                                       log_obj=self.log_obj)
> >  
> > +                # modifying parser's parameters, such like output formatter
> > +                if suite_category == "reftest":
> > +                    reftest_output = (
> 
> No need for this variable, might as well just call the function directly.

Ok, will remove it.
(Assignee)

Comment 7

2 years ago
(In reply to Paul Yang [:pyang] from comment #6)
> (In reply to Andrew Halberstadt [:ahal] from comment #5)
> > Comment on attachment 8772702 [details] [diff] [review]
> > structured-log-using-reftestformatter
> > 
> > Review of attachment 8772702 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Great, I like this approach a lot better than moving it into mozlog!
> > 
> > ::: testing/mozharness/scripts/desktop_unittest.py
> > @@ +397,5 @@
> > >                      option = option % str_format_values
> > >                      if not option.endswith('None'):
> > >                          base_cmd.append(option)
> > > +                if self.structured_output(suite_category):
> > > +                    base_cmd.append("--log-raw=-")
> > 
> > This change is part of the other patch right?
> 
> yes, it's common section for all structured log patch.  in mochitest patch
> we modified function signature and make it not minimal workable
> section(flavor related not necessary for reftest IMO).   mochitest patch
> still need time to work, in the meantime probably we can land this with
> reftest. 
> Do you think we should move it to another bug and block all those bugs?

in Bug 1261194
attachment
https://bug1261194.bmoattachments.org/attachment.cgi?id=8772681

will be more sufficient to land this before reftest ones.  ni? ahal for more information.
Flags: needinfo?(ahalberstadt)
Yeah, I think you should land just this change first. But please only include the stuff necessary to import the ReftestFormatter in the patch. Feel free to upload the modified patch and flag me for r?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 9

2 years ago
Created attachment 8775467 [details] [diff] [review]
structured-log-using-reftestformatter

Remove enabling structured log section, including only importing reftestformatter.
Attachment #8772702 - Attachment is obsolete: true
Attachment #8775467 - Flags: review?(ahalberstadt)
Comment on attachment 8775467 [details] [diff] [review]
structured-log-using-reftestformatter

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

Thanks, this looks good! Please remove the comment and then feel free to land it (assuming you have a try run already, I believe I saw one..)!

::: testing/mozharness/scripts/desktop_unittest.py
@@ +638,5 @@
>                                                       log_obj=self.log_obj)
>  
> +                # Modifying parser's parameters, such like output formatter
> +                # It works only for reftest while Bug 1289702 addressed problem
> +                # and need more generic solution

I don't think this comment is necessary, it's fairly obvious what the code is doing.
Attachment #8775467 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8776536 [details] [diff] [review]
structured-log-using-reftestformatter

Remove comment by Comment 11
Attachment #8775467 - Attachment is obsolete: true
Comment on attachment 8776536 [details] [diff] [review]
structured-log-using-reftestformatter

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

Thanks, looks good!
Attachment #8776536 - Flags: review+

Comment 14

2 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a3485a9ddd
Let mozharness use ReftestFormatter when adapting structured output parser; r=ahal

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4a3485a9ddd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.