Make it easy to retrigger a mochitest with extra debugging options

RESOLVED FIXED in Firefox 55

Status

Testing
Mochitest
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.)
Assignee: nobody → wlachance
Comment hidden (mozreview-request)
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
Attachment #8842124 - Flags: feedback?(jopsen)
Attachment #8842124 - Flags: feedback?(bstack)
Attachment #8842124 - Flags: feedback?(ahalberstadt)

Comment 3

a year ago
mozreview-review
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.
Attachment #8842124 - Flags: review+

Comment 8

a year ago
mozreview-review
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"
Attachment #8842124 - Flags: feedback?(ahalberstadt) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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.
Attachment #8843007 - Flags: review?(dustin)

Comment 18

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

https://reviewboard.mozilla.org/r/116750/#review118416
Attachment #8843007 - Flags: review?(dustin) → review+
Comment hidden (mozreview-request)

Comment 21

a year ago
mozreview-review
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.
Attachment #8842124 - Flags: review?(ahalberstadt) → review-
> 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.
(Assignee)

Comment 23

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 30

a year ago
mozreview-review
Comment on attachment 8842124 [details]
Bug 1343327 - Add taskcluster mochitest retrigger action

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

Thanks, looks good to me.
Attachment #8842124 - Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

a year ago
Pushed by wlachance@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ebdd7d5fa745
https://hg.mozilla.org/mozilla-central/rev/452781c4ee87
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
No longer depends on: 1347654
You need to log in before you can comment on or make changes to this bug.