Closed Bug 1489100 Opened Last year Closed 10 months ago

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

Categories

(Firefox Build System :: Try, enhancement)

enhancement
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: gabriel-v, Assigned: marco)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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"]
}
Blocks: 1488850, 1429463
Assignee: nobody → gabriel.vijiala
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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).
(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?
Flags: needinfo?(ahal)
Summary: Specify test paths separately for each suite in try_task_config.json by removing MOZHARNESS_TEST_PATHS → Specify test paths separately for each suite in try_task_config.json MOZHARNESS_TEST_PATHS
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?
Flags: needinfo?(gbrown)
(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.
Flags: needinfo?(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.
(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.
(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.
Flags: needinfo?(gbrown)
(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.
Attachment #9014360 - Flags: feedback?(mozilla)
Attachment #9014360 - Flags: feedback?(ahal)
(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
Attachment #9014360 - Flags: feedback?(mozilla)
Attachment #9014360 - Flags: feedback?(ahal) → feedback-
Blocks: 1488842
Blocks: 1488844
Blocks: 1507108
No longer blocks: 1429463
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: gabriel.vijiala → mcastelluccio
Attachment #9014360 - Attachment is obsolete: true
Attachment #9025699 - Flags: feedback?(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.
Attachment #9025699 - Flags: feedback?(ahal) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Attachment #9025699 - Attachment is obsolete: true
Attachment #9026722 - Flags: review?(ahal)
Attached patch Patch (obsolete) — Splinter Review
Rebased after landing of bug 1507635.
Attachment #9026722 - Attachment is obsolete: true
Attachment #9026722 - Flags: review?(ahal)
Attachment #9027072 - Flags: review?(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?
Attachment #9027072 - Flags: review?(ahal) → review+
(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.
(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.
Attached patch PatchSplinter Review
Carrying r+.
Attachment #9027072 - Attachment is obsolete: true
Attachment #9028018 - Flags: review+
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920c0a874e73
Specify tests per suite in MOZHARNESS_TEST_PATHS. r=ahal
Blocks: 1510452
https://hg.mozilla.org/mozilla-central/rev/920c0a874e73
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Blocks: 1513598
Depends on: 1515653
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
Target Milestone: Firefox 65 → mozilla65
Depends on: 1523717
Regressions: 1541424
No longer blocks: 1523303
Regressions: 1523303
You need to log in before you can comment on or make changes to this bug.