1149670 - Add a command line interface for selecting tests by tag or directory to run on try

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

This will likely be implemented as a mach command. The idea is to be able to run "./mach try <directory or tag>" and have that prepare a commit that will result in all tests for that directory or tag running on try across multiple harnesses, initially targeting flavors of mochitest and xpcshell.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

The first part of this is a combination of bug 978846 followed by bug 774137, and some harness work to introduce some uniformity for options that get passed to harnesses. I'm planning to start with bug 978846 so this can fit in to existing workflows for the time being.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to detect tests in specified directories and make a manifest of them to be run in automation.

Pull down this commit:

hg pull -r 4e6c281766f28542bd3d55399ade93cad8adc0f2 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1149670/chmanchester

/r/7921 - Bug 1149670 - Detect a user specified manifest from the tree and pass a reference to it from mozharness to the harness process.

Pull down this commit:

hg pull -r 4e99a6da252512ca27df3c35eba15769f20dbb2a https://reviewboard-hg.mozilla.org/build-mozharness

Comment 4

[Chris Manchester (limited bugmail, email directly)]

commment 2 and comment 3 are my work in progress. If we want to be able to kick off try pushes from some place other than the tree the stuff in comment 2 would need to be extracted.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r 9ed56bae118b150a8d661d32b436fa9cd3d9ea7b https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Accept manifest arguments to try syntax in mozharness and filter master test manifests to include only those tests.

Pull down this commit:

hg pull -r 90dabc75efa50eb21e15c0ae37554ffdfd4a71db https://reviewboard-hg.mozilla.org/build-mozharness

Comment 7

[Chris Manchester (limited bugmail, email directly)]

comment 5 and comment 6 rewrote this to depend less on things happening in the tree, so specifying manifests is just a matter of passing them through try syntax.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Accept manifest arguments to try syntax in mozharness and filter master test manifests to include only those tests.

Pull down this commit:

hg pull -r ebe75f2a2aa837513e1acd5ca83e5fd57077a0d9 https://reviewboard-hg.mozilla.org/build-mozharness

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r c0874ce891e5c83dd4c12b674ef8ae4973306e2f https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Accept manifest arguments to try syntax in mozharness and filter master test manifests to include only those tests.

Pull down this commit:

hg pull -r 217369f0f493e9acdb6a8ccb2dcd582507a6cf8d https://reviewboard-hg.mozilla.org/build-mozharness

Comment 11

[Chris Manchester (limited bugmail, email directly)]

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

Comment 12

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/7915/#review6875

::: testing/autotry.py:1
(Diff revision 3)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Can we put this in testing/tools?

::: testing/autotry.py:16
(Diff revision 3)
> +    test_flavors = ([

nit: extra bracket not necessary

::: testing/autotry.py:31
(Diff revision 3)
> +                                                cwd=self.mach_context.cwd))

Just be aware that mach_context.cwd might not work in the B2G context.

::: testing/autotry.py:42
(Diff revision 3)
> +                manifest = t['manifest'][len(self.topsrcdir) + 1:]

I guess this works, but might be better to use os.path.relpath

::: testing/autotry.py:62
(Diff revision 3)
> +        tests = ','.join(sum([flavor_suites[f] for f in flavors], []))

What's the second parameter to sum for? Also I think you can drop the square brackets around the first parameter.

::: testing/autotry.py:68
(Diff revision 3)
> +        args = ['hg']
> +        if not verbose:
> +            args.append('-q')
> +        args.extend(['qnew', 'try', '-m', msg])
> +        ret = subprocess.call(args)
> +        if ret:
> +            print('WARNING hg command %s returned %s' % (args, ret))

This pattern is repeated a lot, might be worth making a "run_hg" type of method.

::: testing/autotry.py:68
(Diff revision 3)
> +        args = ['hg']
> +        if not verbose:
> +            args.append('-q')
> +        args.extend(['qnew', 'try', '-m', msg])
> +        ret = subprocess.call(args)
> +        if ret:
> +            print('WARNING hg command %s returned %s' % (args, ret))

This pattern is repeated a lot, might be worth making a "run_hg" type of method.

::: testing/mach_commands.py:378
(Diff revision 3)
> +    @Command('autotry', category='testing', description=AUTO_TRY_HELP_MSG)

Imo |mach try| sounds better than |mach autotry|

::: testing/mach_commands.py:409
(Diff revision 3)
> +        if not len(manifests_by_flavor.keys()):

nit: if not manifests_by_flavor

::: testing/mach_commands.py:358
(Diff revision 3)
> +            exit(1)

Should this be sys.exit? Ditto for uses below.

::: testing/mach_commands.py:391
(Diff revision 3)
> +    @CommandArgument('--push', dest='push', action='store_true',

I kind of think this should be default, with a --no-push option. I guess we can leave it like this as a first iteration. Fwiw trychooser has --no-push as well.

Comment 13

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/8055/#review6877

::: mozharness/mozilla/testing/testbase.py:353
(Diff revision 3)
> +                # Lines are formatted as [include:<path>], we care about <path>.

Does this work with all-tests-dirs.list above?

::: mozharness/mozilla/testing/testbase.py:348
(Diff revision 3)
> +            if not os.path.exists(m):

Maybe use os.path.isfile to be safe?

::: mozharness/mozilla/testing/testbase.py:373
(Diff revision 3)
> +            self.info('TinderboxPrint: Tests will be run from the following specified '

nit: 'following specified manifests' sounds weird

Comment 14

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/7915/#review6881

> Imo |mach try| sounds better than |mach autotry|

Do we have a brand manager on the A-Team? I'm torn.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8055/#review6885

> Does this work with all-tests-dirs.list above?

Yeah oddly that's an ini file.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Accept manifest arguments to try syntax in mozharness and filter master test manifests to include only those tests.

Pull down this commit:

hg pull -r fe02843f9b9dad73f55e101caf3cb9b5d656461c https://reviewboard-hg.mozilla.org/build-mozharness

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r 71f80cdb2dba15c8d36811752f54f780c3952a3d https://reviewboard-hg.mozilla.org/gecko/

Comment 18

[Chris Manchester (limited bugmail, email directly)]

New try (typo fix on retriggers): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6296acaa7175

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Accept manifest arguments to try syntax in mozharness and filter master test manifests to include only those tests.

Pull down this commit:

hg pull -r 41a024d1e691ba9242fd7115e4232e84579ed19a https://reviewboard-hg.mozilla.org/build-mozharness

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r 041268a5ca83bc9e5db1f1e1281deeeb40b927ab https://reviewboard-hg.mozilla.org/gecko/

Comment 21

[Chris Manchester (limited bugmail, email directly)]

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

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r 56d62cc64ddf4fc81b946e5fe5e6ba299c0d199b https://reviewboard-hg.mozilla.org/gecko/

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

/r/7915 - Bug 1149670 - Add a mach command to find tests in specified directories and calculate an appropriate try syntax to run them.

Pull down this commit:

hg pull -r 24b4acffab4b1d104c463afee2c20a4a53b30145 https://reviewboard-hg.mozilla.org/gecko/

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/7915/#review6939

::: testing/tools/autotry/autotry.py:96
(Diff revision 7)
> +        self._run_hg('qnew', 'autotry', '-m', msg)

Please use shelve/unshelve for the trychooser commits. This is better than MQ because it isn't MQ. MQ is more or less deprecated by Mercurial.

An even better solution would be a Mercurial extension that does an in-memory commit for the lifetime of the push. Not terribly difficult to implement. But we'd need to write that. Probably best to repurpose an existing trychooser extension. But that is scope creep.

::: testing/tools/autotry/autotry.py:93
(Diff revision 7)
> +        if subprocess.check_output(['hg', 'diff']):

The Git users will not be pleased they are unable to participate in your brave new world.

Features like this are a major reason why I wish we only had to support a single VCS. I hate solving a problem N>1 times.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/7915/#review6941

> The Git users will not be pleased they are unable to participate in your brave new world.
> 
> Features like this are a major reason why I wish we only had to support a single VCS. I hate solving a problem N>1 times.

Git support is a stretch goal. Until recently git-push-to-try was the cutting edge for git users at mozilla, which does pretty much what I have here.

> Please use shelve/unshelve for the trychooser commits. This is better than MQ because it isn't MQ. MQ is more or less deprecated by Mercurial.
> 
> An even better solution would be a Mercurial extension that does an in-memory commit for the lifetime of the push. Not terribly difficult to implement. But we'd need to write that. Probably best to repurpose an existing trychooser extension. But that is scope creep.

I don't see how to do it with shelve.

Comment 26

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/7915/#review6959

> I don't see how to do it with shelve.

We discussed this a bit in #mozreview last night. We want a mercurial extension that generates a commit in memory with the try message, pushes it to try, and rolls back to the prior state of the repository. I'm going to look into this but will push a non-mq workaround in the mean time.

Comment 27

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/7915/#review6957

Looks good, I'll wait for the new version that has the new mercurial stuff in it before giving it a Ship It.

::: testing/mach_commands.py:335
(Diff revision 7)
> +ATTENTION: Autotry is in beta, please use at your own risk

nit: this should be HOLD THE PRESSES

::: testing/mach_commands.py:360
(Diff revision 7)
> +        if platforms is None:
> +            platforms = os.environ['AUTOTRY_PLATFORM_HINT']
> +        if builds is None:
> +            builds = 'do'

nit: these should be optional args in the method signature

::: testing/tools/autotry/autotry.py:79
(Diff revision 7)
> +        return AUTOTRY_TMPL % locals()

Clever, but seems like a failure waiting to happen.

Comment 28

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/8055/#review6961

Looks good, please address remaining issues.

::: mozharness/mozilla/testing/try_tools.py:53
(Diff revision 5)
> +                self.fatal('Try syntax does not support passing custom or store_const '
> +                           'arguments to the harness process.' %
> +                           opts['action'])

This string doesn't have a %s in which to substitute the action.

::: mozharness/mozilla/testing/try_tools.py:62
(Diff revision 5)
> +        (args, _) = parser.parse_known_args(all_try_args)

nit: brackets not necessary

::: mozharness/mozilla/testing/try_tools.py:70
(Diff revision 5)
> +        for (arg, value) in vars(args).iteritems():

nit: brackets not necessary

::: mozharness/mozilla/testing/testbase.py:250
(Diff revision 5)
>      def _parse_extra_try_arguments(self, known_try_arguments):
>          """
>          Given a buildbot config, parse an existing try syntax to extract additional
>          arguments to pass on to the test harness command line.
>  
>          Acceptable arguments are configured by a white-list in tree to reduce the likelihood
>          an off-label use of try syntax or noise from a commit message will send unexpected
>          arguments to the harness and fail a run.
>  
>          Extracting arguments from a commit message taken directly from the try_parser.
>          """
>          if not self.buildbot_config or self.buildbot_config['properties']['branch'] != 'try':
>              return
>  
>          comments = self.buildbot_config['sourcestamp']['changes'][-1]['comments']
> +        self.parse_extra_try_arguments(comments, known_try_arguments)

Why not just call self.parse_extra_try_arguments directly from \_read_tree_config? Or even make self.parse_extra_try_arguments an @PostScriptAction.

Comment 29

[Andrew Halberstadt [:ahal]]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

https://reviewboard.mozilla.org/r/8053/#review6963

Ship It!

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Mozharness: https://hg.mozilla.org/build/mozharness/rev/aea5cb0e9630

Comment 31

[Release Engineering SlaveAPI Service]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production

Comment 32

[Andrew Halberstadt [:ahal]]

Btw, if getting this to work without mq is a huge pita, I'd settle for a TODO and a follow-up bug to get things moving in the short term.

Comment 33

[Andrew Halberstadt [:ahal]]

Comment on attachment 8599612 [details]
MozReview Request: bz://1149670/chmanchester

Removing r? for now so I get a notification when next commit is ready.

Comment 34

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Add support for filtering reftest manifests by path-based test selections in mozharness.;r=ahal

Pull down this commit:

hg pull -r 055e72d53bae444c690dc1c1e901fd684723447b https://reviewboard-hg.mozilla.org/build-mozharness

Comment 35

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

Comment 36

[Chris Manchester (limited bugmail, email directly)]

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

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Sorry, the diff view in comment 34 is a mess because it includes my fix-ups since the last review. The relevant change is the diff from 5-6 in try_tools.py

Comment 38

[Andrew Halberstadt [:ahal]]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

https://reviewboard.mozilla.org/r/8053/#review7323

::: mozharness/mozilla/testing/try_tools.py:133
(Diff revisions 5 - 6)
> +            os.remove(m)

Use self.rmtree, which has retries and logging.

::: mozharness/mozilla/testing/try_tools.py:93
(Diff revisions 5 - 6)
> -        master_manifests = [
> +        master_ini_manifests = [
>              'mochitest/chrome/chrome.ini',
>              'mochitest/browser/browser-chrome.ini',
>              'mochitest/tests/mochitest.ini',
>              'xpcshell/tests/all-test-dirs.list',
>              'xpcshell/tests/xpcshell.ini',
>          ]
>  
> +        master_list_manifests = [
> +            'reftest/tests/layout/reftests/reftest.list',
> +            'reftest/tests/testing/crashtest/crashtests.list',
> +        ]

I think it would be simpler if these were in a single list of the form (manifest, function)..

    master_manifests = [
        ('foo/mochitest.ini', ini_parser),
        ('bar/reftest.list', reftest_parser),
    ]
    
Then you can have single for loop below and the parsing logic is abstracted.

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

/r/8055 - Bug 1149670 - Add support for filtering reftest manifests by path-based test selections in mozharness.;r=ahal

Pull down this commit:

hg pull -r 42eee160b2f0182f7a069103e9480aba14725149 https://reviewboard-hg.mozilla.org/build-mozharness

Comment 40

[Andrew Halberstadt [:ahal]]

Comment on attachment 8600424 [details]
MozReview Request: bz://1149670/chmanchester

https://reviewboard.mozilla.org/r/8053/#review7365

Thanks, r+ (for the try_tools.py changes.. I'm assuming the testbase.py changes are invalid).

Comment 41

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/build/mozharness/rev/e106a1a0d161

Comment 42

[Rail Aliiev [:rail]]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/1a2600cf03fc

Comment 43

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/7913/#review7681

::: testing/tools/autotry/autotry.py:98
(Diff revision 7)
> +    def push_to_try(self, verbose):

Can we also have a version of this that will work for the large number of people using git? I don't think it's very complex; just check |git status -z| to see if there is anything uncomitted in the tree, then |git commit --allow-empty -m [try string]|, then |git reset HEAD~| to remove the commit again.

Comment 44

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/7913/#review8081

> Can we also have a version of this that will work for the large number of people using git? I don't think it's very complex; just check |git status -z| to see if there is anything uncomitted in the tree, then |git commit --allow-empty -m [try string]|, then |git reset HEAD~| to remove the commit again.

:gps asked about this before, and I'm planning to work on it. The reference point I found for pushing to try from git is git-push-to-try (https://github.com/mozilla/moz-git-tools/blob/master/git-push-to-try), but this comment suggests we can push to git directly, which seems preferable.

Comment 45

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.

Comment 46

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/7915/#review8521

> We discussed this a bit in #mozreview last night. We want a mercurial extension that generates a commit in memory with the try message, pushes it to try, and rolls back to the prior state of the repository. I'm going to look into this but will push a non-mq workaround in the mean time.

The mercurial extension is in https://bugzilla.mozilla.org/show_bug.cgi?id=1162093 . I couldn't actually figure out a non-mq workaround (just commiting a dummy file and rolling back doesn't work if there are mq patches applied).

Comment 47

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8613247 [details]
MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

Comment 48

[Andrew Halberstadt [:ahal]]

Comment on attachment 8613247 [details]
MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

https://reviewboard.mozilla.org/r/7915/#review9095

::: testing/tools/autotry/autotry.py:111
(Diff revisions 8 - 9)
> +                print('ERROR git-cinnabar is required to push from git to try with'
> +                      'the autotry command.')

Would be nice to include a link to the project.

::: testing/tools/autotry/autotry.py:106
(Diff revisions 8 - 9)
> -        hg_args = ['hg', 'push-to-try', '-m', msg]
> +        if os.path.exists(os.path.join(self.topsrcdir, '.git')):

What if someone cloned an unrelated repo into their topsrcdir? Can we check for hg first, and failing that then check for git?

Comment 49

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8613247 [details]
MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

Comment 50

[Chris Manchester (limited bugmail, email directly)]

Feedback from comment 48 turned into a few changes, just looking for a quick review on the last diff before landing. Thanks!

Comment 51

[Andrew Halberstadt [:ahal]]

Comment on attachment 8613247 [details]
MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

https://reviewboard.mozilla.org/r/7915/#review9315

::: testing/mach_commands.py:408
(Diff revisions 9 - 10)
> -    @Command('autotry', category='testing', description=AUTO_TRY_HELP_MSG)
> +    @Command('try', category='testing', description=AUTOTRY_HELP_MSG)

+1, I like |mach try|

::: testing/mach_commands.py:441
(Diff revisions 9 - 10)
> +        if at.find_uncommited_changes():
> +            print('ERROR please commit changes before continuing')
> +            sys.exit(1)

This check might be better in AutoTry.__init__

Comment 52

[Andrew Halberstadt [:ahal]]

Comment on attachment 8613247 [details]
MozReview Request: Bug 1149670 - Add a mach command to find tests in specified directories and prepare a commit to push them to try.;r=ahal

https://reviewboard.mozilla.org/r/7915/#review9317

Ship It!

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec493d9f5996

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec493d9f5996

Comment 56

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1149670 - Detect a user specified manifest from the tree and pass a reference to it from mozharness to the harness process.

Comment 57

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1149670 - Add support for filtering reftest manifests by path-based test selections in mozharness.;r=ahal