ensure --tag try syntax (parsed by mozharness) works in taskcluster

RESOLVED FIXED in mozilla51

Status

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: jmaher, Assigned: cmanchester)

Tracking

unspecified
mozilla51
Dependency tree / graph

Details

Attachments

(2 attachments)

in a simple experiment we found that adding |--tag testing/mochitest| to a try push, ended up running all the tests in a chunk, not just the directory specified:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8494401673af

I assume this feature of --tag requires buildbot and we will have to revisit this for the future of taskcluster.
jmaher: is --tag a feature supported on Buildbot? or is this a request to implement it for both Buildbot and TaskCluster?
this is supported on buildbot, this is work that :chmanchester did a few quarters ago to allow for specific tests to be run on try.  when pushing with this in a taskcluster job, we ignore it and run the job.  I believe the tool that uses this pulls data from buildbot properties.

:chmanchester, can you fill in some of the missing details here?
Flags: needinfo?(cmanchester)
When :ahal and I worked on re-implementing try syntax in TC, I don't recall anything about this.  It doesn't seem supported by trychooser, either.

In https://bugzilla.mozilla.org/show_bug.cgi?id=1247703 we're planning to write a compatibility translator from the command-line format used for try now into a task query language, so we can include such features as this as long as we know how they work.
There are several arguments that mozharness will pick out of try syntax, which it finds in this function: https://dxr.mozilla.org/mozilla-central/rev/9da51cb4974e03cdd8fa45a34086fe1033abfeaf/testing/mozharness/mozharness/mozilla/testing/try_tools.py#77

I could see that failing because we don't have a buildbot config. The syntax in the push from comment 0 doesn't look right to me, but when I try a different syntax that would work in buildbot we have a different error in the decision task: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c36ccbc18dae

Overloading and re-parsing try syntax is a bit of a hack in the first place, and I wonder if task query language could grow to express test selection, but the shortest path to  parity with buildbot here will be passing try syntax to mozharness test jobs in taskcluster.
Flags: needinfo?(cmanchester)
OK, thanks -- at least I don't need to re-implement this functionality.  It does seem like something of a hack, though, doesn't it..
Component: General → Integration
Summary: ensure test selection via try syntax works in taskcluster → ensure --tag try syntax (parsed by mozharness) works in taskcluster
Assignee: nobody → dustin
Component: Integration → Platform and Services
Chris, is this something you could work on?  It might be as simple as configuring test jobs to pass the first line of the commit message in a COMMIT_MESSAGE env var, and then discovering that from mozharness.
Assignee: dustin → nobody
Flags: needinfo?(cmanchester)
Assignee: nobody → cmanchester
Flags: needinfo?(cmanchester)
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

https://reviewboard.mozilla.org/r/75382/#review73660

::: taskcluster/taskgraph/transforms/tests/make_task_description.py:182
(Diff revision 1)
>  
>      if 'actions' in mozharness:
>          env['MOZHARNESS_ACTIONS'] = ' '.join(mozharness['actions'])
>  
> +    if config.params['project'] == 'try':
> +        env['TRY_COMMIT_MSG'] = config.params['message']

Is 'message' the comment on the tipmost changeset?

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:117
(Diff revision 1)
>          """
> -        if (not self.buildbot_config or 'properties' not in self.buildbot_config or
> +
> +        # For taskcluster jobs, we will get our try syntax from an environment variable.
> +        # TRY_COMMIT_MSG will only be set if this job is on try.
> +        if 'TRY_COMMIT_MSG' in os.environ:
> +            msg = HTMLParser().unescape(os.environ['TRY_COMMIT_MSG'])

Why an HTMLParser()?
I thought we're dealing with a one line string with the try syntax.
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

https://reviewboard.mozilla.org/r/75382/#review73660

> Is 'message' the comment on the tipmost changeset?

It's that change's commit message, according to http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/taskcluster/docs/parameters.rst#42

> Why an HTMLParser()?
> I thought we're dealing with a one line string with the try syntax.

We are, but I found it contains html entity references (see https://queue.taskcluster.net/v1/task/TzDHttd4TNCvAtK3RwPqmA/runs/0/artifacts/public%2Fparameters.yml for instance) that needed to be unescaped. I'm not really sure why.
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

https://reviewboard.mozilla.org/r/75382/#review73660

> We are, but I found it contains html entity references (see https://queue.taskcluster.net/v1/task/TzDHttd4TNCvAtK3RwPqmA/runs/0/artifacts/public%2Fparameters.yml for instance) that needed to be unescaped. I'm not really sure why.

Could you please ask dustin if that is intentional?
If he has no objections go ahead and land it.

Please add a comment if we need to keep the html parser.

Here's a sample from the referenced file:
> message: 'try: -b do -p linux64 -u mochitest-browser-chrome-1,mochitest-devtools-chrome-1,mochitest-e10s-browser-chrome-1,mochitest-e10s-devtools-chrome-1,mochitest-o
>   -t none --tag trackingprotection --try-test-paths browser-chrome:browser/base/content/test/general
>   browser-chrome:browser/components/privatebrowsing/test/browser
>   chrome:toolkit/components/url-classifier/tests/mochitest devtools-chrome:devtools/client/webconsole/test'
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

https://reviewboard.mozilla.org/r/75382/#review73684

oh mozreview...
Attachment #8786447 - Flags: review?(armenzg) → review+
Dustin, can you comment on comment 11? Thanks.
Flags: needinfo?(dustin)
It's not intentional, no.  My guess is that mozilla-taskcluster is quoting it, since we're using mustache which is intended for web templating.  So let's fix that first.
Flags: needinfo?(dustin)
https://www.npmjs.com/package/mustache

"All variables are HTML-escaped by default. If you want to render unescaped HTML, use the triple mustache: {{{name}}}. You can also use & to unescape a variable."
I bet this would fix it (although at the risk of `'` characters in the message causing the decision task to fail)

diff --git a/.taskcluster.yml b/.taskcluster.yml
--- a/.taskcluster.yml
+++ b/.taskcluster.yml
@@ -99,17 +99,17 @@ tasks:
           - -cx
           - >
               cd /home/worker/checkouts/gecko &&
               ln -s /home/worker/artifacts artifacts &&
               ./mach --log-no-times taskgraph decision
               --pushlog-id='{{pushlog_id}}'
               --pushdate='{{pushdate}}'
               --project='{{project}}'
-              --message='{{comment}}'
+              --message='{{{comment}}}'
               --owner='{{owner}}'
               --level='{{level}}'
               --base-repository='https://hg.mozilla.org/mozilla-central'
               --head-repository='{{{url}}}'
               --head-ref='{{revision}}'
               --head-rev='{{revision}}'
               --revision-hash='{{revision_hash}}'
https://github.com/taskcluster/mozilla-taskcluster/pull/98 should address that risk; in the interim, the patch in comment 16 should get you unstuck.
(In reply to Dustin J. Mitchell [:dustin] from comment #18)
> https://github.com/taskcluster/mozilla-taskcluster/pull/98 should address
> that risk; in the interim, the patch in comment 16 should get you unstuck.

I don't think we can land that -- commit messages with ' in them probably aren't that rare. Why not unescape in mozharness for now?
Because then mozharness will be surprised when it gets unescaped input.  Perhaps better to wait until mozilla-taskcluster is patched to create shell-quoted input.
The mozilla-taskcluster patch landed Wednesday.
Comment on attachment 8787738 [details]
Bug 1252235: shell-quote the commit message;

https://reviewboard.mozilla.org/r/76422/#review74584

Thanks. I'll fold this into the original patch when landing.
Attachment #8787738 - Flags: review?(cmanchester) → review+
Something about the patch in comment 22 appears to prevent decision tasks from running on try. Here it is folded in:

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

and without:

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

Dustin, any idea what's going on here?
Flags: needinfo?(dustin)
Yeah, it looks like the bit of mozilla-taskcluster that posts error messages from instantiating the decision task still doesn't work, so I can see the error in the logs but it doesn't appear in treeherder.  Aside from that there's some error in my implementation in https://github.com/taskcluster/mozilla-taskcluster/pull/98/files.  I'll fix both :)
Flags: needinfo?(dustin)
I M dumb .. https://github.com/taskcluster/mozilla-taskcluster/pull/100

Greg's already fixed the task-error issue.  Sorry for the noise!
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

I had to rebase this significantly, re-requesting review.
Attachment #8786447 - Flags: review+ → review?(armenzg)
Comment on attachment 8786447 [details]
Bug 1252235 - Make try syntax available to mozharness from TaskCluster through an environment variable.

https://reviewboard.mozilla.org/r/75380/#review77444

LGTM
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fdc068413084
Make try syntax available to mozharness from TaskCluster through an environment variable. r=armenzg
Attachment #8786447 - Flags: review?(armenzg) → review+
https://hg.mozilla.org/mozilla-central/rev/fdc068413084
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: Platform and Services → Services
You need to log in before you can comment on or make changes to this bug.