1489100 - Specify test paths separately for each suite in try_task_config.json MOZHARNESS_TEST_PATHS

Status: RESOLVED FIXED

(Developer Infrastructure :: Try, enhancement)

Description

[Gabriel Vîjială [:gabriel-v]]

Right now, all specified test paths for per-test try pushes go to the same environment variable: MOZHARNESS_TEST_PATHS.

This creates a number of problems when submitting test paths this way when running multiple test suites: bug 1488850, bug 1429463 

I suggest we completely remove MOZHARNESS_TEST_PATHS and add a new top-level dict to try_task_configs.json, so it looks like this:

{
  "test_paths": {"suite1": ["path1"], "suite2": ["path2"]},
  "tasks":["task1", "task2"]
}

instead of this, the current format:

{
  "templates":{
    "env":{
      "MOZHARNESS_TEST_PATHS":"path1:path2"
    },
  },
  "tasks":["task1", "task2"]
}

Comment 1

[Andrew Halberstadt [:ahal]]

To clarify, the data in "try_task_config.json" isn't available to the worker when it executes. It is only available to the taskgraph (decision task), and it is quite limited in how it can be applied to modify the taskgraph. There is a bit more information on this process here:
https://firefox-source-docs.mozilla.org/taskcluster/taskcluster/try.html#modifying-tasks-in-a-try-push

So a couple points:
1) Using 'templates' is mandatory
2) Templates need to correspond to a JSON-e file in taskcluster/taskgraph/templates
3) Templates can only modify the task definition

Because of 3) the only real way to pass data down to the worker is either directly in the 'command' or via the 'env'. So I'd recommend we keep the MOZHARNESS_TEST_PATHS env, but instead of a list of paths, make it JSON (and modify the mozharness scripts appropriately).

Comment 2

[Gabriel Vîjială [:gabriel-v]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Because of 3) the only real way to pass data down to the worker is either
> directly in the 'command' or via the 'env'. So I'd recommend we keep the
> MOZHARNESS_TEST_PATHS env, but instead of a list of paths, make it JSON (and
> modify the mozharness scripts appropriately).

So the above example will look like this:

{
  "templates":{
    "env":{
      "MOZHARNESS_TEST_PATHS":"{\"suite1\": [\"path1\"], \"suite2\": [\"path2\"]}"
    }
  },
  "tasks":["task1", "task2"]
}

Inside the tree, MOZHARNESS_TEST_PATHS is only set in the Path template from tools/tryselect/templates.py. I think we should keep its `paths` argument unchanged, to keep the functionality of `mach try fuzzy path1 path2` as it is.

Is Path.context() the correct place to instantiate a TestResolver and call resolver.resolve_tests(os.path.dirname(path)) for each test path, to obtain its suite name?

Comment 3

[Gabriel Vîjială [:gabriel-v]]

In bug 1429463 comment 23 you mentioned a test-verify-backfill UI that requests a try job. That UI should also send requests in the above format. Where is that UI's repository? I suppose the UI won't have means to use a TestResolver to find the suite name for each test path. Could we make the UI to send test paths under some other env, like MOZHARNESS_TEST_VERIFY_BACKFILL_PATHS, and resolve the test suite names in testing/mozharness/mozharness/mozilla/testing/per_test_base.py?

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Gabriel Vîjială [:gabriel-v] from comment #2)
> (In reply to Andrew Halberstadt [:ahal] from comment #1)
> > Because of 3) the only real way to pass data down to the worker is either
> > directly in the 'command' or via the 'env'. So I'd recommend we keep the
> > MOZHARNESS_TEST_PATHS env, but instead of a list of paths, make it JSON (and
> > modify the mozharness scripts appropriately).
> 
> So the above example will look like this:
> 
> {
>   "templates":{
>     "env":{
>       "MOZHARNESS_TEST_PATHS":"{\"suite1\": [\"path1\"], \"suite2\":
> [\"path2\"]}"
>     }
>   },
>   "tasks":["task1", "task2"]
> }

Yes, that format is what I was proposing.


> Inside the tree, MOZHARNESS_TEST_PATHS is only set in the Path template from
> tools/tryselect/templates.py. I think we should keep its `paths` argument
> unchanged, to keep the functionality of `mach try fuzzy path1 path2` as it
> is.

I think it should be possible to have the JSON format, but keep the command line unchanged. We'd need some sort of logic in the `tryselect` module that does that transformation (from [<paths>] to {<suite>: <paths>}).


> Is Path.context() the correct place to instantiate a TestResolver and call
> resolver.resolve_tests(os.path.dirname(path)) for each test path, to obtain
> its suite name?

Could you clarify a bit more? I'm not sure what `Path.context()` refers to. I can say that if you're proposing we run the TestResolver on the worker's end, I'm not sure if it'll work. We don't have a full source checkout there and I imagine the TestResolver might depend on some build modules that won't be available. If you can get this to work I agree it would be a good solution, but I think it'll be hard to do.

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> > Is Path.context() the correct place to instantiate a TestResolver and call
> > resolver.resolve_tests(os.path.dirname(path)) for each test path, to obtain
> > its suite name?
> 
> Could you clarify a bit more? I'm not sure what `Path.context()` refers to.
> I can say that if you're proposing we run the TestResolver on the worker's
> end, I'm not sure if it'll work. We don't have a full source checkout there
> and I imagine the TestResolver might depend on some build modules that won't
> be available. If you can get this to work I agree it would be a good
> solution, but I think it'll be hard to do.

Oh, I guess you mean in templates.py (sorry I haven't paged this stuff into my brain recently :)). Yes, I think that would be a good place to do the TestResolving.

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> > Inside the tree, MOZHARNESS_TEST_PATHS is only set in the Path template from
> > tools/tryselect/templates.py. I think we should keep its `paths` argument
> > unchanged, to keep the functionality of `mach try fuzzy path1 path2` as it
> > is.
> 
> I think it should be possible to have the JSON format, but keep the command
> line unchanged. We'd need some sort of logic in the `tryselect` module that
> does that transformation (from [<paths>] to {<suite>: <paths>}).

I also think I misread what you wrote and we are agreeing here.

Comment 7

[Geoff Brown [:gbrown]]

(In reply to Gabriel Vîjială [:gabriel-v] from comment #3)
> In bug 1429463 comment 23 you mentioned a test-verify-backfill UI that
> requests a try job. That UI should also send requests in the above format.
> Where is that UI's repository? I suppose the UI won't have means to use a
> TestResolver to find the suite name for each test path. Could we make the UI
> to send test paths under some other env, like
> MOZHARNESS_TEST_VERIFY_BACKFILL_PATHS, and resolve the test suite names in
> testing/mozharness/mozharness/mozilla/testing/per_test_base.py?

I don't know much about the implementation of test-verify-backfill, but I believe bug 1471227 is the most relevant -- have a look at that. Note that it is not specific to try, but can be applied to any tree. You can see it in action on treeherder: select a test task, select the "..." menu > "Custom Action", then in Custom Taskcluster Job Actions, select the "Backfill" action, and add testPath: <path> -- a TV-bf task should be spawned, running test-verify on the specified test.

Comment 8

[Tom Prince [:tomprince]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> To clarify, the data in "try_task_config.json" isn't available to the worker
> when it executes. It is only available to the taskgraph (decision task), and
> it is quite limited in how it can be applied to modify the taskgraph. There
> is a bit more information on this process here:
> https://firefox-source-docs.mozilla.org/taskcluster/taskcluster/try.
> html#modifying-tasks-in-a-try-push
> 
> So a couple points:
> 1) Using 'templates' is mandatory
> 2) Templates need to correspond to a JSON-e file in
> taskcluster/taskgraph/templates
> 3) Templates can only modify the task definition
> 
> Because of 3) the only real way to pass data down to the worker is either
> directly in the 'command' or via the 'env'. So I'd recommend we keep the
> MOZHARNESS_TEST_PATHS env, but instead of a list of paths, make it JSON (and
> modify the mozharness scripts appropriately).

Another option would be to make the test transform aware of a new entry in `try_task_config`, and change the task appropriately. (by looking in `config.params['try_task_config']` in the transforms, probably. This would decouple how the data gets passed in to the decision task, from how it is consumed by the task.

Comment 9

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1489100 - Specify test paths separately in try_task_config.json.

Comment 10

[Gabriel Vîjială [:gabriel-v]]

(In reply to Tom Prince [:tomprince] from comment #8)
> Another option would be to make the test transform aware of a new entry in
> `try_task_config`, and change the task appropriately. (by looking in
> `config.params['try_task_config']` in the transforms, probably. This would
> decouple how the data gets passed in to the decision task, from how it is
> consumed by the task.

I ended up adding a new template (test-paths) that clones what 'env' is doing. The tests transform picks that up and sets a 'test-tasks' entry when generating job descriptions. I also added the 'test-paths' entry to the job schema. A new job transform then selects test paths from 'test-tasks' and sets the MOZHARNESS_TEST_PATHS env on it.

I would prefer to put all this logic in the test transform, but I found no easy way to set an environment variable at that stage.

I think this solution is preferrable because it leaves the test runners unchanged (they still get a MOZHARNESS_TEST_PATHS).
This solution will also leave the test-verify-backfill UI unchanged, as setting the MOZHARNESS_TEST_PATHS in a try job will still work as expected if there's no 'test-paths' set on the try config template.

Another issue is the hardcoded flavor list I added for test-verify paths in the job.rewrite_test_paths_to_env transform. I figured this list out from testing/mozbase/moztest/moztest/resolve.py and don't know if it's correct.

What do you think?

Try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d53b33cac9e64450d68e4cb957748e77315414

Comment 11

[Marco Castelluccio [:marco]]

Attachment: WIP

This is a WIP patch, I think I've caught everything, but I need to double check that it works with all possible test suites, flavors, whatever.

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 9025699 [details] [diff] [review]
WIP

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

This is looking good to me! Sorry for the delay.

::: testing/mozbase/moztest/moztest/resolve.py
@@ +434,4 @@
>              tests = self._tests_by_path[p]
>  
>              for test in fltr(tests):
> +                test['src_path'] = p

Let's call this `srcdir_relpath`, bit less ambiguous.

::: tools/tryselect/tasks.py
@@ +145,5 @@
> +    print(suite_to_tests)
> +
> +    return {
> +        'MOZHARNESS_TEST_PATHS': json.dumps(suite_to_tests),
> +    }

I'd mildly prefer if this function just returned `json.dumps(suite_to_tests)` and wasn't aware of MOZHARNESS_TEST_PATHS. We could then call it something more generic like `resolve_tests_by_suite` or something similar.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch

Rebased after landing of bug 1507635.

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 9027072 [details] [diff] [review]
Patch

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

Looking good! r+ with nits/questions addressed.

::: taskcluster/taskgraph/util/perfile.py
@@ +50,5 @@
>      changed_files = set()
>      specified_files = []
>      if try_task_config:
> +        for suite, paths in json.loads(try_task_config).items():
> +            specified_files += paths

nit: just iterate over `json.loads(...).values()`

You could even use `itertools.chain` to join them all at once.

::: testing/mozbase/moztest/moztest/resolve.py
@@ +437,4 @@
>              tests = self._tests_by_path[p]
>  
>              for test in fltr(tests):
> +                test['srcdir_relpath'] = p

Sorry I didn't catch this the first time, but the proper place to put this is probably:
https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/backend/test_manifest.py#80

Though I guess leaving it here isn't the end of the world either. But the former will get it into the actual build metadata.

::: testing/mozharness/scripts/android_emulator_unittest.py
@@ +215,4 @@
>              ),
>          }
>  
> +        user_paths = json.loads(os.environ.get('MOZHARNESS_TEST_PATHS'), '""')

The python docs say the second argument is the encoding. What is the '""' for?

Comment 16

[Marco Castelluccio [:marco]]

(In reply to Andrew Halberstadt [:ahal] from comment #15)
> ::: testing/mozbase/moztest/moztest/resolve.py
> @@ +437,4 @@
> >              tests = self._tests_by_path[p]
> >  
> >              for test in fltr(tests):
> > +                test['srcdir_relpath'] = p
> 
> Sorry I didn't catch this the first time, but the proper place to put this
> is probably:
> https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/
> backend/test_manifest.py#80
> 
> Though I guess leaving it here isn't the end of the world either. But the
> former will get it into the actual build metadata.

I've added it here because the data in _tests_by_path seemed to be inconsistent (not all objects in there had the same properties), which makes me think the data is coming from different sources. Adding it here made it simpler as I wouldn't have to change all possible sources.

> 
> ::: testing/mozharness/scripts/android_emulator_unittest.py
> @@ +215,4 @@
> >              ),
> >          }
> >  
> > +        user_paths = json.loads(os.environ.get('MOZHARNESS_TEST_PATHS'), '""')
> 
> The python docs say the second argument is the encoding. What is the '""'
> for?

It was meant to be the second parameter of os.environ.get (so that user_paths is '' if MOZHARNESS_TEST_PATHS is not defined), I put it in the wrong place.

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #16)
> (In reply to Andrew Halberstadt [:ahal] from comment #15)
> > ::: testing/mozbase/moztest/moztest/resolve.py
> > @@ +437,4 @@
> > >              tests = self._tests_by_path[p]
> > >  
> > >              for test in fltr(tests):
> > > +                test['srcdir_relpath'] = p
> > 
> > Sorry I didn't catch this the first time, but the proper place to put this
> > is probably:
> > https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/
> > backend/test_manifest.py#80
> > 
> > Though I guess leaving it here isn't the end of the world either. But the
> > former will get it into the actual build metadata.
> 
> I've added it here because the data in _tests_by_path seemed to be
> inconsistent (not all objects in there had the same properties), which makes
> me think the data is coming from different sources. Adding it here made it
> simpler as I wouldn't have to change all possible sources.

It was coming from https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/python/mozbuild/mozbuild/backend/test_manifest.py#81 and https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/testing/mozbase/moztest/moztest/resolve.py#479, I've added srcdir_relpath to both now.

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920c0a874e73
Specify tests per suite in MOZHARNESS_TEST_PATHS. r=ahal

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/920c0a874e73

Comment 21

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

Comment on attachment 9028018 [details] [diff] [review]
Patch

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

::: taskcluster/taskgraph/actions/backfill.py
@@ +143,5 @@
>  
> +                    if 'testPath' in input:
> +                        task.task['payload']['env']['MOZHARNESS_TEST_PATHS'] = json.dums({
> +                            task.task['extra']['suite']['flavor']: input['testPath']
> +                        })

this is json.dums, it needs to be json.dumps