Closed Bug 779983 Opened 12 years ago Closed 9 years ago

mozharness device_talosrunner has some minor issues

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: wlach, Assigned: wlach)

Details

(Whiteboard: [mozharness])

Attachments

(1 file)

(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?
Whiteboard: [mozharness]
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
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 ?
Component: Other → Mozharness
This almost certainly isn't valid anymore.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: