779983 - mozharness device_talosrunner has some minor issues

Status: RESOLVED INVALID

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[William Lachance (:wlach)]

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

Comment 1

[William Lachance (:wlach)]

Attachment: Patch to hopefully address issues pointed out in bug

Comment 2

[Aki Sasaki (not active)]

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.

Comment 3

[William Lachance (:wlach)]

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

Comment 4

[William Lachance (:wlach)]

(oh, and thanks for the other feedback -- all valid issues and I will address before checking in)

Comment 5

[Justin Wood (:Callek)]

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
                  ^

Comment 6

[Aki Sasaki (not active)]

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 ?

Comment 7

[William Lachance (:wlach)]

This almost certainly isn't valid anymore.