Closed Bug 1186844 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: parkouss, Assigned: parkouss)

References

Details

Attachments

(4 files, 1 obsolete file)

Also fixes pep8 issues (this blocks bug 1186834).
Attached patch 1186844.patch (obsolete) — Splinter Review
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.
Attachment #8637837 - Flags: review?(jmaher)
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).
Attachment #8637854 - Flags: review?(jmaher)
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?
Attachment #8637854 - Flags: review?(jmaher) → review+
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?
Attachment #8637837 - Flags: review?(jmaher) → review+
(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. :)
Attached patch 1186844_2.patchSplinter Review
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).
Attachment #8637837 - Attachment is obsolete: true
I assume we need to land the mozharness patch at the same time as this?
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...
Attachment #8638429 - Flags: review?(j.parkouss) → review+
Keywords: leave-open
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/j.parkouss@gmail.com-68a3ad4f92fe/try-linux64/try_ubuntu64_hw_test-chromez-bm104-tests1-linux-build241.txt.gz

We can safely remove it.
Attachment #8638907 - Flags: review?(jmaher)
Comment on attachment 8638907 [details] [diff] [review]
11868448_remove_option.patch

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

simple!
Attachment #8638907 - Flags: review?(jmaher) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.