742750 - mozharness.mozilla.talos should be smarter about checking for binary, tests

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P3)

Description

[Jeff Hammel]

Currently, binary and tests are checked for in __init__:

http://hg.mozilla.org/build/mozharness/file/776008125d68/mozharness/mozilla/talos.py#l85

This works for the all actions case, but if you only want to create a
virtualenv or --list-actions, then this check is superfluous and
annoying.  Instead, we should check for these only for actions that
need it.

Comment 1

[Jeff Hammel]

Attachment: check to see applicable actions for binary and tests

jetperf (bug 729205) will need this as well, as its actions are different

Comment 2

[Aki Sasaki (not active)]

Comment on attachment 612644 [details] [diff] [review]
check to see applicable actions for binary and tests

Hm.  Aiui:

* We actually don't need either tests or binary set for run-tests.  By that point that value has already passed from PerfConfigurator to the yaml file.
* We actually do need tests and binary set for generate-config, whether we run-tests in this specific run or not.  A perfectly valid workflow would be to do everything up to run-tests in one or more script runs, and then run-tests afterwards using the pre-generated yaml.

The right place for this feels like preflight_generate_config().  This is automatically called if generate-config is in the action list.

However, you were concerned that this wouldn't warn the user immediately that something's wrong.  We could certainly call preflight_generate_config() early in check() if generate-config is in the action list.

When we get the download-and-install portion slotted in, this logic will change again.  The default binary path will be the path we're going to install to.  The user can override this via commandline or config file.  We're going to fail out if we don't install anything or the path is otherwise invalid.  Since this is going to change in the near future, I wouldn't spend too much time trying to get this perfect now, but I've said very similar things to you before and you've preferred to spend the time, so do what you will :)

r- due to faulty checks as to when we require tests and binary to be set.

Comment 3

[Jeff Hammel]

Attachment: move to preflight_generate_config and call from check()

I kept check() as a separate function so that derived classes, e.g. jetperf, can override since they can/will have difference actions

Comment 4

[Aki Sasaki (not active)]

Comment on attachment 614601 [details] [diff] [review]
move to preflight_generate_config and call from check()

I think this works.  I have some changes to make in this area (to make mozharness.mozilla.testing.talos inherit TestingMixin, and for device_talosrunner.py to inherit Talos).

I'm going to r+ because this works as is, unbitrot my changes, and continue making changes on my talosrunner branch.  I'll upstream this sooner than the rest of my work since it's shared.

Comment 5

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/mozharness/rev/b8941a8b52e5

Should probably file a follow-up about integration of mozharness.mozilla.testing.talos and .basetest

Comment 6

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #5)
> pushed: http://hg.mozilla.org/build/mozharness/rev/b8941a8b52e5
> 
> Should probably file a follow-up about integration of
> mozharness.mozilla.testing.talos and .basetest

follow-up filed: https://bugzilla.mozilla.org/show_bug.cgi?id=745021