1186844 - fix KeyError: 'results_urls' when no results_url is given and make the option deprecated

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

Also fixes pep8 issues (this blocks bug 1186834).

Comment 1

[Julien Pagès (:parkouss)]

Attachment: 1186844.patch

Well I took the opportunity to slightly refactor the output:

- moving the output decision logic to one place
- use absolute path in develop mode (this is great because at the end it prints the absolute path to output files - good when running from mach for example)
- remove the unneeded output_options dict.

This run fine locally.

Comment 2

[Julien Pagès (:parkouss)]

Attachment: 1186844_harness_fix.patch

So once patch 1 will be landed, Talos won't use the deprecated option - so we will be able to remove it from harness also. This is what this patch does (note that we will need to update the talos.json to point to the new rev, we can do that at the time by updating this patch).

Comment 3

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

Comment on attachment 8637854 [details] [diff] [review]
1186844_harness_fix.patch

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

this is great!  can we ensure that ./mach talos-test chromez still works?

Comment 4

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

Comment on attachment 8637837 [details] [diff] [review]
1186844.patch

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

looks good!

::: talos/PerfConfigurator.py
@@ +54,5 @@
>              'default': 5,
>              'flags': ['-w', '--browserWait']
>          }),
>          ('results_urls', {
> +            'help': 'DEPRECATED, will be removed soon.',

can we just remove this instead of deprecating it?

Comment 5

[Julien Pagès (:parkouss)]

(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 8637837 [details] [diff] [review]
> 1186844.patch
> 
> Review of attachment 8637837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good!
> 
> ::: talos/PerfConfigurator.py
> @@ +54,5 @@
> >              'default': 5,
> >              'flags': ['-w', '--browserWait']
> >          }),
> >          ('results_urls', {
> > +            'help': 'DEPRECATED, will be removed soon.',
> 
> can we just remove this instead of deprecating it?

Unfortunately, no. :( We can only remove it after the harness no more use that, but we can't remove that from the harness without hardcoding it... :)
So, will be a follow up. :)

Comment 6

[Julien Pagès (:parkouss)]

I pushed to try the talos patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=918508d59a5c

Comment 7

[Julien Pagès (:parkouss)]

Attachment: 1186844_2.patch

So try was pretty good, except that we found a regression (see bug 1187082).

Yesterday we added a patch to this try, and it fixed the regression - so this patch is exactly that (combination of the two). Regression fix is in run_tests.py, there is a comment.

jmaher, feel free to read/askfeedback/land this patch (for me it can be landed since this is what we tried with success).

Comment 8

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

landed the talos patch:
https://hg.mozilla.org/build/talos/rev/e47c566d1c11

Comment 9

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

Attachment: update talos.json in tree (1.0)

Comment 10

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

I assume we need to land the mozharness patch at the same time as this?

Comment 11

[Julien Pagès (:parkouss)]

no, the mozharness patch has not been test yet - it is about removing the option that is now obsolete for talos (harness will define it - just talos do not take it in account since it is now hardcoded).

Once Attachment #8638429 [details] [diff] is in m-c, I'll try the harness patch. Finally, once this harness patch will be in m-c, we will be able to remove the DEPRECATED option from talos !

Heh, all this will become a lot easier once Talos will be in tree. :)

---

Oh, I think I know (now!) what you mean, we could have removed totally the option and land the mozharness patch at the same time of updating the revision in talos.json. Heh, it would have been two patches instead of 4. (I feel stupid :))

Well if you have the time we still can do this - we should test the mozharness patch first. Else, just land your patch (I review it just after), and let's stick to my bad plan...

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2b525a29cf

Comment 13

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

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

Comment 14

[Julien Pagès (:parkouss)]

Pushed to try the mozharness patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a3ad4f92fe

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1165227cd73

Comment 16

[Julien Pagès (:parkouss)]

Attachment: 11868448_remove_option.patch

Well, now that we can see that mozharness no longer uses the --results-url option:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68a3ad4f92fe/try-linux64/try_ubuntu64_hw_test-chromez-bm104-tests1-linux-build241.txt.gz

We can safely remove it.

Comment 17

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

Comment on attachment 8638907 [details] [diff] [review]
11868448_remove_option.patch

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

simple!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Julien Pagès (:parkouss)]

Merged attachment reviewed in comment 17 in my no-android talos branch (since we will put that in-tree), http://www.bytebucket.org/parkouss/talos/commits/a420b8c69d7e10f51a3e25655ce7700ac58478bd?at=no-android.

Comment 20

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.