1400895 - Better try support for test-verify

Status: RESOLVED FIXED

(Testing :: General, enhancement, P3)

Description

[Geoff Brown [:gbrown]]

Currently, if I modify a test and push to try with "-u test-verify-e10s", my test will be verified.

But there are other use cases to consider:
 - I've modified the browser and want to make sure certain tests have not been broken.
 - I've modified test support files or test utilities and want to make sure certain tests have not been broken.
 - The test is intermittently failing and I want to use test-verify on try to determine if it can be reproduced with TV.
 - The test is intermittently failing and I want to reproduce the failure efficiently on try, perhaps with more logging.

For these cases, I'd like to support something like 

  "-u test-verify-e10s <path>"

to trigger verification of <path> regardless of file modifications.

Comment 1

[Geoff Brown [:gbrown]]

The first problem here is that we can't get past the when-files-changed clause:

https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/taskcluster/ci/test/tests.yml#2034

unless we happen to have other .js/.html/.xul/.xhtml file changes we want to push to try.

Comment 2

[Geoff Brown [:gbrown]]

It might also be nice if TV ran even when not explicitly requested, like the lint jobs.

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #2)
> It might also be nice if TV ran even when not explicitly requested, like the
> lint jobs.

That came up again today: Sometimes people seem to expect test-verify to run on try when they modify a test, even if not explicitly requested. Would that be wasteful/annoying when not wanted?

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

what about an interface of |./mach try fuzzy -q "linux64 test-verify" <path>| where path is a single path including the test file name?

that wouldn't solve test-verify auto starting, but we could do it for *html, *js, *xul, *xhtml, *htm; and sometimes there will be TV jobs that don't run anything in those cases.

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #4)
> what about an interface of |./mach try fuzzy -q "linux64 test-verify"
> <path>| where path is a single path including the test file name?

Yes, that looks right. 'try fuzzy' already allows <paths>, but I think it only accepts directories -- not individual tests.

> that wouldn't solve test-verify auto starting, but we could do it for *html,
> *js, *xul, *xhtml, *htm; and sometimes there will be TV jobs that don't run
> anything in those cases.

I don't think you need to worry about that. 'mach try -u test-verify-e10s' starts TV even with no changes (task "schedules" are somehow avoided when a task is explicitly requested for try):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=676d0acefc55f103e217dfa7e0f79f9eaf9aec1c


For bonus points, it occurs to me that the 'lookup test path from test name' logic from 'mach test-info' would be really cool:

https://dxr.mozilla.org/mozilla-central/rev/0528a414c2a86dad0623779abde5301d37337934/testing/mach_commands.py#660-675

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

interesting thought- that uses direct hg and a source tree- I believe we have a source tree accessible from the decision task.  Would the intention of the logic from |mach test-info| be to take an edited or provided source file and confirm it is a test file?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

it appears that we can support fuzzypaths for a single file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92c3598e48fd89004a1f82b91af832f60f8f1c3e

Comment 8

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #6)
> interesting thought- that uses direct hg and a source tree- I believe we
> have a source tree accessible from the decision task.  Would the intention
> of the logic from |mach test-info| be to take an edited or provided source
> file and confirm it is a test file?

It confirms that the file exists, but not that it is a test. The bigger gain is filename -> full path completion:

mach test-info test_simple.js
===== test_simple.js =====
Found netwerk/test/unit/test_simple.js in source control.

Conceivably one could say

mach try fuzzy -q 'test-verify linux64' test_simple.js

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

the single file (or list of files is found in the parameters.yml and then set as an environment variable for mozharness via: MOZHARNESS_TEST_PATHS.

We would need to schedule test-verify and optimize it out if not specified or no MOZHARNESS_TEST_PATHS exist.  We can then take the test paths and use that as our input file names both in selecting what to schedule in tests.py as well as in mozharness where verify_tools.py can look for the env variable first (and then make sure we remove it from the test runners)

If we wanted to edit a job arbitrarily we would just need to edit the parameters.yml to schedule a test-verify job with the proper data.  If we were not running the decision task again, we would need to schedule the test-verify job with the proper environment variable set.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: allow test-verify to be forced and use paths from mach try fuzzy

here is a variety of try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=0376a356b5ddf1c3c4ccf9b570e5bdb09bd9f33e&group_state=expanded&filter-searchStr=test-verify&tochange=2a2efd3112183e43e0cb9fe2da72d7473f922439

this now will run test-verify when you change files or specify files on the ./mach try command line, likewise you can now specify -u test-verify and get tests to run whereas before it required an 'add new' job trick.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8976322 [details] [diff] [review]
allow test-verify to be forced and use paths from mach try fuzzy

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

I think this is a good approach. I like the idea of appending the "specified" paths in |mach try| to the list of touched files for the purposes of optimization. I think it'll be useful for things outside of test-verify as well.

I think the biggest issue for me here is the change to fuzzy.py. See if it'll work with my suggestion instead.

::: taskcluster/taskgraph/transforms/tests.py
@@ +842,5 @@
>                           'js/src/test/test/',
>                           'js/src/test/non262/',
>                           'js/src/test/test262/']
>  
> +    changed_files = None

It would be nice if we called this "specified_files" or something up here. Then join it with "changed_files" later on.

@@ +849,5 @@
> +        if 'templates' in task_config.keys() and \
> +           'env' in task_config['templates'].keys():
> +            changed_files = task_config['templates']['env'].get('MOZHARNESS_TEST_PATHS', None)
> +            if changed_files:
> +                changed_files = changed_files.split(":")

Try something like:

env = config.params.get('try_task_config', {}).get('templates', {}).get('env', {})
changed_files = env.get('MOZHARNESS_TEST_PATHS', '').split(':')

@@ +853,5 @@
> +                changed_files = changed_files.split(":")
> +
> +    if not changed_files:
> +        changed_files = files_changed.get_changed_files(config.params.get('head_repository'),
> +                                                        config.params.get('head_rev'))

I think you'd want the union of these two things right? Otherwise the normal stuff that would run based on modified files (like lint jobs) will no longer run when you use test-verify on try.

::: tools/tryselect/selectors/fuzzy.py
@@ +186,5 @@
>                      flavor, " and subsuite '{}'".format(subsuite) if subsuite else ""))
>              continue
>  
>          task_regexes.add(suite['task_regex'])
> +    task_regexes.add('test-verify(?:-e10s)?(?:-[0-9])?$')

I'm a little fuzzy (no pun intended) on how this works. Doesn't this mean we'll *always* have the test-verify tasks in try_task_config.json? If the goal is to make sure they're always considered, just add the `always_target` taskcluster parameter to the test-verify tasks:
https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/mozlint.yml#2

Comment 12

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #11)
> env = config.params.get('try_task_config', {}).get('templates',
> {}).get('env', {})
> changed_files = env.get('MOZHARNESS_TEST_PATHS', '').split(':')

Oops, this isn't quite right, ''.split(':') will yield ['']. So you'll probably need an if statement after all, but it should still be cleaner.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

I tried adding the always-target attribute to the task (needed to add it in as a valid option for test jobs first), but it still doesn't recognize verify jobs from mach try fuzzy.

test-verify tasks will get optimized out if there are no tests to run (there are some edge cases).

Another problem I noticed was that when I go to find out how many chunks to generate by looking at the changed_files, I do not have access to try_tasks_json while in the |mach try fuzzy| stage.  I would like to figure out how to solve that and I believe our usage of mach + test-verify will be stable.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: support mach try fuzzy <path> for test-verify

a lot of changes here:
* file discovery is done in a taskgraph/util/perfile.py file now
* transform always generates 1 task
* optimize will remove tasks with 0 files to test
* regex support in all test types, and multiple regex/type supported in fuzzy.py

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8979698 [details] [diff] [review]
support mach try fuzzy <path> for test-verify

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

Thanks, this looks much better! And hopefully will stop us from running into those mysterious disappearing tasks :p

All my comments are minor, though there's just enough of them it's probably worth re-reviewing.

::: taskcluster/taskgraph/optimize.py
@@ +387,5 @@
> +        # we would like to return 'False, None' while it's high_value_task
> +        # and we wouldn't optimize it. Otherwise, it will return 'True, None'
> +        env = params.get('try_task_config', {})
> +        if not env:
> +            env = {}

env = params.get('try_task_config') or {}

::: taskcluster/taskgraph/transforms/tests.py
@@ +30,5 @@
>  )
>  from taskgraph.util.taskcluster import get_artifact_path
>  from mozbuild.schedules import INCLUSIVE_COMPONENTS
>  
> +from ..util.perfile import perfile_number_of_chunks

nit: use import format consistent with above, e.g from taskgraph.util ..

::: taskcluster/taskgraph/util/perfile.py
@@ +9,5 @@
> +
> +from mozbuild.util import memoize
> +from mozpack.path import match as mozpackmatch
> +from mozversioncontrol import get_repository_object, InvalidRepoPath
> +from taskgraph import files_changed

Do you know if files_changed.get_files_changed() is any different from the one in mozversioncontrol? I never realized taskcluster had this function. Maybe using mozversioncontrol isn't needed in this case?

@@ +92,5 @@
> +
> +
> +# create a single instance of this class, and expose its `perfile_number_of_chunks`
> +# bound method as a module-level function
> +perfile_number_of_chunks = PerFile().perfile_number_of_chunks

Why bother making PerFile a class? A module level function with global variables works just as well and is easier to read. Plus it'll make this consistent with the rest of the util modules (they also use module functions and globals).

::: tools/tryselect/selectors/fuzzy.py
@@ +186,5 @@
>                      flavor, " and subsuite '{}'".format(subsuite) if subsuite else ""))
>              continue
>  
> +        for regex in suite['task_regex']:
> +            task_regexes.add(regex)

nit: instead of looping, just change `add` to `update`

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the review, files_changed.get_files_changed() uses hg.mozilla.org/json-automationrelevance:
https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/files_changed.py#20

this means it doesn't get data from the local repo, it gets it from the repo it was launched from (i.e. autoland, try, etc.)

In order to query both modes: fuzzy and decision task, we need to support local outgoing files as well as changed files.  Could taskcluster use vcs instead of the hg server?  I am not sure if it would know what set of patches make up the entire push.  Otherwise I think it could be explored to change.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: support mach try fuzzy <path> for test-verify

updated the patch to address all the comments except for the get_changed_files which I mentioned in comment 16.

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8979869 [details] [diff] [review]
support mach try fuzzy <path> for test-verify

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

Thanks, lgtm!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f56e6dd9164
Better try support for test-verify. r=ahal

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8f56e6dd9164

Comment 21

[Geoff Brown [:gbrown]]

This works great for me - thanks!

I have updated https://developer.mozilla.org/en-US/docs/Mozilla/QA/Test_Verification appropriately.


It may be worth noting that this only helps TV/TVw with 'mach try fuzzy'; I don't think it is possible to do the same thing with try syntax:

$ mach try -b d -p android-api-16 -u test-verify dom/base/test/test_intersectionobservers.html --artifact
Specified path "/home/gbrown/src/dom/base/test/test_intersectionobservers.html" is not a directory under the srcdir, unable to specify tests outside of the srcdir