1286513 - Let mozharness use ReftestFormatter when adapting structured output parser

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Paul Yang [:pyang] (away)]

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/

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Paul Yang [:pyang] (away)]

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.

Comment 3

[Paul Yang [:pyang] (away)]

Attachment: structured-log-using-reftestformatter

below is try result

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

enable raw log emitting is also included.

Comment 4

[Paul Yang [:pyang] (away)]

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 5

[Andrew Halberstadt [:ahal]]

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.

Comment 6

[Paul Yang [:pyang] (away)]

(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.

Comment 7

[Paul Yang [:pyang] (away)]

(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.

Comment 8

[Andrew Halberstadt [:ahal]]

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?

Comment 9

[Paul Yang [:pyang] (away)]

Attachment: structured-log-using-reftestformatter

Remove enabling structured log section, including only importing reftestformatter.

Comment 10

[Paul Yang [:pyang] (away)]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a994094152bc&selectedJob=24606603

Comment 11

[Andrew Halberstadt [:ahal]]

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.

Comment 12

[Paul Yang [:pyang] (away)]

Attachment: structured-log-using-reftestformatter

Remove comment by Comment 11

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8776536 [details] [diff] [review]
structured-log-using-reftestformatter

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

Thanks, looks good!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a3485a9ddd
Let mozharness use ReftestFormatter when adapting structured output parser; r=ahal

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4a3485a9ddd