1170281 - cleanup talos output.py file

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

talos has gone through a lot of changes over the years.

For this bug, I do not have clear direction.  Previously we had two upload classes:
GraphServerOutput
DatazillaOutput

We removed DatazillaOutput, and replaced it with PerfherderOutput!

In the near future (later this summer), we won't be uploading to graph server.  I believe we could cleanup output.py in preparation of that.  Some thoughts:
* we have a base class and methods defined to support graphserver + datazilla
* perfherderoutput has what it needs- ideally it could be simplified a bit
* we could reduce graphserver output code to not be so generic and add some hacks/scaffolding around it
* maybe move graphserver output and related code to a different file (post_file.py, graph_server.py?) and keep output.py simplified.
* reduce the needs of data structures to hold data

I am open to anything!

Comment 1

[Julien Pagès (:parkouss)]

The graph server url (results_urls) option should be removed then (eventually hardcoded if we start to adress this bug before we drop the support to graph server). See bug 1172918 comment 6.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

We have now removed graphserver support from Talos!  This bug can be more actionable now.

Comment 3

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v1.diff

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8758804 [details] [diff] [review]
Bug1170281_v1.diff

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

a few things I noticed- lets fix those and post a new patch.

::: testing/talos/talos/output.py
@@ -282,5 @@
>                                 'subtests': counter_subtests})
>          return test_results
>  
> -# available output formats
> -formats = {'output_urls': PerfherderOutput}

have you looked in results.py for where we use formats?

@@ +186,5 @@
> +            json.dump(results, file(results_path, 'w'), indent=2,
> +                      sort_keys=True)
> +
> +    def post(self, results, server, path, scheme, tbpl_output):
> +        raise NotImplementedError("Abstract base class")

we should have post defined, this isn't an abstract base class anymore

Comment 5

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v2.diff

Have incorporated the said changes :)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8758827 [details] [diff] [review]
Bug1170281_v2.diff

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

lets give this another try!

::: testing/talos/talos/results.py
@@ -44,5 @@
>              cls = output.formats[format]
>              cls.check(urls)
>  
> -    @classmethod
> -    def check_formats_exist(cls, formats):

who calls this?  we should ensure when we remove code that nobody is depending on it...I suspect there is a handful of other lines to remove from here.

Comment 7

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v3.diff

Thanks for that :)

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8758872 [details] [diff] [review]
Bug1170281_v3.diff

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

hey, this is looking better.  Please run on try |try -b o -p linux64,win32 -u none -t all| after verifying things work ok with a simple local test.

Comment 9

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v4.diff

A successful try push for the same can be found here,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14e04e84d4b7

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8759989 [details] [diff] [review]
Bug1170281_v4.diff

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

one thing I would like to verify before calling this done.  Please verify a local file is produced with the current patch, if not, then add the for loop back in.

::: testing/talos/talos/results.py
@@ +39,5 @@
>          try:
> +            url = output_formats.items()
> +            _output = output.Output(self)
> +            results = _output()
> +            _output.output(results, url, tbpl_output)

do we need the for loop?  I believe it is needed to output the perfherder json as well as a file output.  this can be tested with |mach talos-test -a svgx --output myfile.json|, then verify if myfile.json exists.

Comment 11

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v5.diff

With the current patch local.json is being created. Please have a look.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8763996 [details] [diff] [review]
Bug1170281_v5.diff

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

thanks for fixing this up!

Comment 13

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1170281 -> Bug1170281_v7.diff
applying Bug1170281_v7.diff
patching file testing/talos/talos/output.py
Hunk #2 FAILED at 39
1 out of 3 hunks FAILED -- saving rejects to file testing/talos/talos/output.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1170281_v7.diff

Comment 14

[Vishnu (:malayaleecoder)]

Attachment: Bug1170281_v6.diff

I have done the rebasing, please have a look :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b09506d96f
cleanup talos output.py file r=jmaher

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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