mozharness device_talosrunner has some minor issues

RESOLVED INVALID

Status

Release Engineering
Mozharness
RESOLVED INVALID
5 years ago
2 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozharness])

Attachments

(1 attachment)

(1) You need to specify a whole whack of configuration options (results server, etc.) even if you only want to run talos in local development mode for test purposes (as I do in bug 776728)
(2) The option to turn on development mode is named "start_python_webserver", which is confusing (that's not all it does)
(3) We use the configuration option "active_suites" to specify tests, but this requires you to specify suites twice with two different names. The name used by the underlying talos mozharness script is just "tests". Why not use that?
Created attachment 648483 [details] [diff] [review]
Patch to hopefully address issues pointed out in bug
Attachment #648483 - Flags: review?(aki)

Updated

5 years ago
Whiteboard: [mozharness]

Comment 2

5 years ago
Comment on attachment 648483 [details] [diff] [review]
Patch to hopefully address issues pointed out in bug

In principle, I don't disagree with this.

* Have you run this to verify it still works?
Might be harmless, but I tend to try to test when making changes.  Not sure if you've got this working locally yet?

* unit.sh points out "scripts/device_talosrunner.py:142: undefined name 'additional'", probably from

> +        else:
> +            additional.options.extend(['--webServer', c['talos_webserver']])

* You're renaming a script config setting, but not updating the config files (I only found configs/users/aki/tablet1.py start_python_webserver, which isn't obviously tied to this script, but I tend to recursive grep when making a change.)

r=me with the additional s,start_python_webserver,develop_mode, in configs/users/aki/tablet1.py and the additional_options fix above.  A test run would be good but isn't strictly blocking here.
Attachment #648483 - Flags: review?(aki) → review+
(In reply to Aki Sasaki [:aki] from comment #2)
> Comment on attachment 648483 [details] [diff] [review]
> Patch to hopefully address issues pointed out in bug
> 
> In principle, I don't disagree with this.
> 
> * Have you run this to verify it still works?
> Might be harmless, but I tend to try to test when making changes.  Not sure
> if you've got this working locally yet?

I have, yes. Of course it's somewhat hard to say whether it's work 100% perfectly, since it's Talos, but my phone did seem to be running everything ok.
(oh, and thanks for the other feedback -- all valid issues and I will address before checking in)
Comment on attachment 648483 [details] [diff] [review] [diff] [review]
Patch to hopefully address issues pointed out in bug

---------AUTOMATIC COMMENT---------
--  filter pep8-callek-june-2013 --
---------AUTOMATIC COMMENT---------

$pep8 <dir> --diff --max-line-length=159 --show-source < attachment.diff
/tmp/tmp.6sQOVBKBmy/scripts/device_talosrunner.py:168:19: E124 closing bracket does not match visual indentation
                  ] + additional_options
                  ^
Product: mozilla.org → Release Engineering

Comment 6

4 years ago
We aren't using device_talosrunner.py (:kmoir wrote http://hg.mozilla.org/build/mozharness/file/f374f4bc03ee/scripts/android_panda_talos.py instead), so I removed it a while ago in a general cleanup sweep.

-> WONTFIX/WFM ?

Updated

3 years ago
Component: Other → Mozharness
This almost certainly isn't valid anymore.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.