1343327 - Make it easy to retrigger a mochitest with extra debugging options

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[William Lachance (:wlach)]

Let's file a new issue for tracking the mochitest retriggering part of bug 1322433. This will cover making it possible to retrigger mochitests to run a failing test multiple times with extra options (debugging level, custom environment, etc.)

Comment 1

[William Lachance (:wlach)]

Attachment: Bug 1343327 - Add taskcluster mochitest retrigger action

Review commit: https://reviewboard.mozilla.org/r/116062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116062/

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

This isn't quite done, but I wouldn't mind some feedback at this point. In particular I'm interested in the question of how I should pass down the various harness options (e10s enabled, mochitest flavor) to the mach invocation (if people are satisfied with that approach).

You can see an example of this in action here, look for the jobs marked "custom" (they are failing because I need to add an argument to the mach invocation to stop the harness when done):

https://treeherder.allizom.org/#/jobs?repo=try&revision=81b63571c415049ff0ad6da10283f340102e41b8&selectedJob=74930365

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review117584

::: taskcluster/actions/mochitest-retrigger-action.py:55
(Diff revision 1)
> +                'type': 'object',
> +                'default': {'MY_ENV_1': 'myvalue1'},
> +                'title': 'Extra environment variables',
> +                'description': 'Extra environment variables to use for this run'
> +            }
> +        }

You should specify `additionalProperties: false` and probably `required: [...]` if you have any required properties... looks like all of yours have defaults.

::: taskcluster/actions/mochitest-retrigger-action.py:60
(Diff revision 1)
> +        }
> +    }
> +)
> +def mochitest_retrigger_action(parameters, input, task_group_id, task_id, task):
> +    logger.info("Fetching task definition for mochitest task id %s...", task_id)
> +    task_definition = requests.get(url="{}/{}".format(TASKCLUSTER_QUEUE_URL,

No need, `task` is the selected task definition.

::: taskcluster/actions/mochitest-retrigger-action.py:75
(Diff revision 1)
> +    new_task_definition['payload']['command'] += ['--no-run-tests']
> +
> +    custom_mach_command = ['mochitest']
> +    custom_mach_command += ['--log-mach=-',
> +                            '--log-mach-level={}'.format(input['logLevel'])]
> +    if input['runUntilFail']:

For this to work with an optional property we would have to automatically add the `default` values into the validated data...

I think this is a very sane thing to do. But it's not exactly a trivial thing to do.

AJV can do it when we validate the JSON against the schema. But I'm not familiar with any python libraries that easily do it for us.

It would be a weird requirement that the javascript-side (in browser) must do this. The requirement is that it submits JSON that is in compliance with the schema, which includes JSON that doesn't have the optional properties.

Hence, you won't be able to access the default values this way. I suggest that we use `required: ['runUntilFail', ...]` in the schema to require that all properties are defined. IMO there is no reason that can't work in combination with a `default` value.

Comment 4

[William Lachance (:wlach)]

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

Comment 5

[William Lachance (:wlach)]

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

Comment 6

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98ab1eb6e257

Comment 7

[William Lachance (:wlach)]

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

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review117998

I'm not at all familiar with the taskcluster action task bits, but overall looks reasonable.

::: taskcluster/actions/mochitest-retrigger-action.py:58
(Diff revision 1)
> +                'description': 'Extra environment variables to use for this run'
> +            }
> +        }
> +    }
> +)
> +def mochitest_retrigger_action(parameters, input, task_group_id, task_id, task):

I don't know anything about actions, but do we really want N different <suite>_retrigger_actions? I'd bet with enough massaging, we could get everything in the 'test' kind (i.e things that use mozharness) using the same retrigger action.

::: taskcluster/actions/mochitest-retrigger-action.py:73
(Diff revision 1)
> +    custom_mach_command += ['--log-mach=-',
> +                            '--log-mach-level={}'.format(input['logLevel'])]

The original task uses --log-tbpl.. might be weird if the re-trigger had a different log format.

::: testing/mozharness/scripts/desktop_unittest.py:153
(Diff revision 1)
> +        [["--test-path"], {
> +            "action": "extend",
> +            "dest": "test_path",
> +            "type": "string",
> +            "help": "Specify which test path to run (in lieu of chunking)"
> +        }
> +         ],
> +        [["--run-until-fail"], {
> +            "action": "store_true",
> +            "dest": "run_until_fail",
> +            "default": False,
> +            "help": "Passes the --run-until-fail option to the mochitest harness"}
> +         ],

I *really* want to avoid duplicating all the test harness arguments in mozharness again. Instead could we create a single "--extra-arg" parameter that can be passed in multiple times and whose contents will be appended to the command? E.g:

    --extra-arg "--test-path=path" --extra-arg "--run-until-failure"

Comment 9

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19507038e147

Comment 10

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a89a16a9247

Comment 11

[William Lachance (:wlach)]

Attachment: Bug 1343327 - Export more test job state as environment vars in taskcluster

Review commit: https://reviewboard.mozilla.org/r/116750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116750/

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/1-2/

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review118008

::: taskcluster/actions/mochitest-retrigger-action.py:55
(Diff revision 1)
> +                'type': 'object',
> +                'default': {'MY_ENV_1': 'myvalue1'},
> +                'title': 'Extra environment variables',
> +                'description': 'Extra environment variables to use for this run'
> +            }
> +        }

I think only the path should be required.

::: taskcluster/actions/mochitest-retrigger-action.py:58
(Diff revision 1)
> +                'description': 'Extra environment variables to use for this run'
> +            }
> +        }
> +    }
> +)
> +def mochitest_retrigger_action(parameters, input, task_group_id, task_id, task):

I'm fine with making one action handle multiple test kinds in the future, but I think it makes sense to name this thing mochitest retrigger for now (since it only works with mochitest)

::: taskcluster/actions/mochitest-retrigger-action.py:60
(Diff revision 1)
> +        }
> +    }
> +)
> +def mochitest_retrigger_action(parameters, input, task_group_id, task_id, task):
> +    logger.info("Fetching task definition for mochitest task id %s...", task_id)
> +    task_definition = requests.get(url="{}/{}".format(TASKCLUSTER_QUEUE_URL,

Ok, maybe that was the intention but current this is value of the ACTION_TASK environment variable, which is null.

http://searchfox.org/mozilla-central/source/taskcluster/actions/registry.py#287

https://tools.taskcluster.net/task-inspector/#Whqxb1BRS16BJ4pCpollGg/

Should I file a bug to change this?

::: taskcluster/actions/mochitest-retrigger-action.py:73
(Diff revision 1)
> +    custom_mach_command += ['--log-mach=-',
> +                            '--log-mach-level={}'.format(input['logLevel'])]

Does it? I can't find --log-tbpl anywhere using a search. I'll change this regardless.

::: taskcluster/actions/mochitest-retrigger-action.py:75
(Diff revision 1)
> +    new_task_definition['payload']['command'] += ['--no-run-tests']
> +
> +    custom_mach_command = ['mochitest']
> +    custom_mach_command += ['--log-mach=-',
> +                            '--log-mach-level={}'.format(input['logLevel'])]
> +    if input['runUntilFail']:

I think in this case we don't require the property (we can just say it's false if not present).

::: testing/mozharness/scripts/desktop_unittest.py:153
(Diff revision 1)
> +        [["--test-path"], {
> +            "action": "extend",
> +            "dest": "test_path",
> +            "type": "string",
> +            "help": "Specify which test path to run (in lieu of chunking)"
> +        }
> +         ],
> +        [["--run-until-fail"], {
> +            "action": "store_true",
> +            "dest": "run_until_fail",
> +            "default": False,
> +            "help": "Passes the --run-until-fail option to the mochitest harness"}
> +         ],

It turns out that these mozharness modifications weren't needed (since I'm running the actual tests via mach instead). I'll take this out in a future version of the patch.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review117584

> You should specify `additionalProperties: false` and probably `required: [...]` if you have any required properties... looks like all of yours have defaults.

I'm going to call this resolved, but feel free to reopen if you're not happy with my answer. :)

> No need, `task` is the selected task definition.

Could you open a bug for making task being the selected task definition, if that's the behaviour you want? I don't think we need block this on that.

Comment 15

[William Lachance (:wlach)]

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

Comment 16

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c366248fe78

Comment 17

[William Lachance (:wlach)]

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

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8843007 [details]
Bug 1343327 - Export more test job state as environment vars in taskcluster

https://reviewboard.mozilla.org/r/116750/#review118416

Comment 19

[William Lachance (:wlach)]

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

Comment 20

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/2-3/

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review118820

This is looking really cool! In addition to the issues below, I don't understand the changes in registry.py, and I'm also not familiar with actions at all. My comments are around how the test harness is getting invoked, you'll probably still want dustin or someone else familiar with actions to take a look at this as well.

::: taskcluster/actions/mochitest-retrigger-action.py:95
(Diff revision 3)
> +    if enable_e10s:
> +        custom_mach_command += ['--e10s']

--e10s is not a valid argument. The test harnesses run with e10s by default, and you need to pass in --disable-e10s if you want to override that.

::: taskcluster/actions/mochitest-retrigger-action.py:117
(Diff revision 3)
> +
> +    # update environment
> +    new_task_definition['payload']['env'].update(input.get('environment', {}))
> +
> +    # tweak the treeherder symbol
> +    new_task_definition['extra']['treeherder']['symbol'] = 'custom'

Maybe this should be set to the original symbol with '-custom' appended to it so it's easier to tell where this came from.

::: taskcluster/ci/test/tests.yml:2
(Diff revision 3)
>  # Each stanza here describes a particular test suite or sub-suite.  These are
> +

Accidental whitespace?

::: taskcluster/mach_commands.py:3
(Diff revision 3)
>  # -*- coding: utf-8 -*-
>  
> +

Accidental whitespace?

::: taskcluster/mach_commands.py:310
(Diff revision 3)
> -        actions.trigger_action_callback()
> +        try:
> +            self.setup_logging()
> +            return actions.trigger_action_callback()
> +        except Exception:
> +            traceback.print_exc()
> +            sys.exit(1)

Why is this change needed? Wouldn't allowing the traceback to exit be better?

::: taskcluster/scripts/tester/test-ubuntu.sh:190
(Diff revision 3)
> +# Run a custom mach command (this is typically used by action tasks to run
> +# harnesses in a particular way)
> +if [ "$CUSTOM_MACH_COMMAND" ]; then
> +    eval "/home/worker/workspace/build/tests/mach ${CUSTOM_MACH_COMMAND}"
> +fi

Won't this run the tests twice, once via mozharness, then again with mach? Or is TASKCLUSTER_INTERACTIVE set for these things. If it's the latter, then this should probably be an elsif.

Comment 22

[Dustin J. Mitchell [:dustin] (he/him)]

> Why is this change needed? Wouldn't allowing the traceback to exit be better?

I think Jonas landed the same change.  All mach commands need that boilerplate, otherwise mach prints the traceback with lots of "uhoh something went wrong with mach, this should never happen" text.

Comment 23

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review118820

> Why is this change needed? Wouldn't allowing the traceback to exit be better?

Yeah, I'll be dropping this from the patchset as it's already landed in m-c.

Comment 24

[William Lachance (:wlach)]

Comment on attachment 8843007 [details]
Bug 1343327 - Export more test job state as environment vars in taskcluster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116750/diff/1-2/

Comment 25

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/3-4/

Comment 26

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review118820

> Won't this run the tests twice, once via mozharness, then again with mach? Or is TASKCLUSTER_INTERACTIVE set for these things. If it's the latter, then this should probably be an elsif.

We pass --no-run-tests to mozharness so it doesn't actually run the tests. I admit that this is a little confusing, if you can think of a better way of defining the behaviour here I'm open to hearing it.

Comment 27

[William Lachance (:wlach)]

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

Comment 28

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/4-5/

Comment 29

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86377de6580f

Comment 30

[Andrew Halberstadt [:ahal]]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

https://reviewboard.mozilla.org/r/116062/#review119746

Thanks, looks good to me.

Comment 31

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66ca41cfcddd

Comment 32

[William Lachance (:wlach)]

Comment on attachment 8843007 [details]
Bug 1343327 - Export more test job state as environment vars in taskcluster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116750/diff/2-3/

Comment 33

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/5-6/

Comment 34

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/6-7/

Comment 35

[William Lachance (:wlach)]

Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116062/diff/7-8/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebdd7d5fa745
Export more test job state as environment vars in taskcluster r=dustin
https://hg.mozilla.org/integration/autoland/rev/452781c4ee87
Add taskcluster mochitest retrigger action r=ahal,jonasfj

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ebdd7d5fa745
https://hg.mozilla.org/mozilla-central/rev/452781c4ee87