938019 - Run mochitests from manifests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: WIP-Discover-mochi-tests-through-metadata-not-filesy.patch

This bug is very similar to bug 920849, but for mochitests instead of xpcshell tests.  A key benefit of doing this is that it would allow the developer to specify a test directory while still allowing the tests in that directory to be filtered according to the metadata in the manifest - ie, it's a general solution to what bug 932186 is solving.

This patch is very similar to, and depends on, the patch from gps in that bug, so requesting feedback from gps.  I suspect this would break people who explicitly run tests that aren't yet specified in a manifest, so bug 920185 blocks this.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 831336 [details] [diff] [review]
WIP-Discover-mochi-tests-through-metadata-not-filesy.patch

Review of attachment 831336 [details] [diff] [review]:
-----------------------------------------------------------------

Something like this should land eventually. However, landing this now would make some tests not runnable through mach since not every test is yet defined in manifest files. We don't want to upset people, so we should move every test to a manifest before this lands.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Gregory Szorc [:gps] from comment #1)
> However, landing this now would
> make some tests not runnable through mach since not every test is yet
> defined in manifest files. We don't want to upset people, so we should move
> every test to a manifest before this lands.

Thanks.  FTR, I said exactly that in comment 0:

> I suspect this would break people who explicitly run tests that aren't
> yet specified in a manifest, so bug 920185 blocks this.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-manifests

How does this look, Ted? It mostly works. I disabled it for b2g since I can't get it to work there. I still need to fix a few issues, but I want to make sure it's in the right direction before I spend more time on it.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8389649 [details] [diff] [review]
test-manifests

Review of attachment 8389649 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks sane.

::: testing/mochitest/runtests.py
@@ +443,5 @@
> +      masterName = self.getTestFlavor(options) + '.ini'
> +      masterPath = os.path.join(testRoot, masterName)
> +      if os.path.exists(masterPath):
> +        manifest = TestManifest(strict=False)
> +        manifest.read(masterPath)

You can just write
manifest = TestManifest([masterPath], strict=False)

@@ +458,3 @@
>  
>          # Filter out tests if we are using --test-path
> +        if testPath and not tp.startswith(testPath):

It'd be nice if manifestdestiny had a way to filter tests by path. File a followup?

@@ +459,5 @@
>          # Filter out tests if we are using --test-path
> +        if testPath and not tp.startswith(testPath):
> +          continue
> +
> +        if not self.isTest(options, tp):

Feels like we shouldn't have to do this test--if we have things that are not tests being returned in a test manifest that's bad.

@@ +467,5 @@
>          if test.has_key('disabled'):
>            testob['disabled'] = test['disabled']
>          paths.append(testob)
>  
> +      # Sort tests so they are run in a deterministic order.

Does the manifest parser not return tests in a deterministic order? I would think they'd be returned in the order they're listed in the manifests.

::: testing/mochitest/runtestsb2g.py
@@ +66,5 @@
>          self.test_script_args.append(test_url)
>  
> +    def buildTestPath(self, options):
> +        # Skip over the manifest building that happens on desktop.
> +        return self.buildTestURL(options)

Why are we skipping this on b2g?

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> > +    def buildTestPath(self, options):
> > +        # Skip over the manifest building that happens on desktop.
> > +        return self.buildTestURL(options)
> 
> Why are we skipping this on b2g?

Oops, missed your comment 3. Can you file a followup on fixing this? ahal should be able to help get it straightened out.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

has this patch been tested for android?  I don't see any changes to runtestsremote.py.  It would be great if none were needed.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

fwiw I think we have all the mochitest.ini files in the tree now as well as all possibly b2g*.json and android*.json rules converted to .ini.  let the games begin

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> ::: testing/mochitest/runtests.py
> @@ +443,5 @@
> > +      masterName = self.getTestFlavor(options) + '.ini'
> > +      masterPath = os.path.join(testRoot, masterName)
> > +      if os.path.exists(masterPath):
> > +        manifest = TestManifest(strict=False)
> > +        manifest.read(masterPath)
> 
> You can just write
> manifest = TestManifest([masterPath], strict=False)

Will fix.

> @@ +458,3 @@
> >  
> >          # Filter out tests if we are using --test-path
> > +        if testPath and not tp.startswith(testPath):
> 
> It'd be nice if manifestdestiny had a way to filter tests by path. File a
> followup?

I filed bug 983862 for this.

> @@ +459,5 @@
> >          # Filter out tests if we are using --test-path
> > +        if testPath and not tp.startswith(testPath):
> > +          continue
> > +
> > +        if not self.isTest(options, tp):
> 
> Feels like we shouldn't have to do this test--if we have things that are not
> tests being returned in a test manifest that's bad.

I turned this into an assertion, and it catches 162 issues in mochitest-plain manifests. I filed bug 983867 about this. I don't want to fix it in this bug. 

> @@ +467,5 @@
> >          if test.has_key('disabled'):
> >            testob['disabled'] = test['disabled']
> >          paths.append(testob)
> >  
> > +      # Sort tests so they are run in a deterministic order.
> 
> Does the manifest parser not return tests in a deterministic order? I would
> think they'd be returned in the order they're listed in the manifests.

I don't think they're listed in order in the manifests. I can't find any instances right now, but I remember seeing cases a few days ago.

> ::: testing/mochitest/runtestsb2g.py
> @@ +66,5 @@
> >          self.test_script_args.append(test_url)
> >  
> > +    def buildTestPath(self, options):
> > +        # Skip over the manifest building that happens on desktop.
> > +        return self.buildTestURL(options)
> 
> Why are we skipping this on b2g?

I filed bug 983877 about this.

Comment 9

[Gregory Szorc [:gps]]

Any chance you can use the code from python/mozbuild/mozbuild/testing.py to do the searching and filtering? A lot of this code seems redundant and consolidating to a single code path will really help with things such as implementing automatic test flavor detection and dispatching for |mach test|. If you need to move that code into mozbase and upload all-tests.json (or a variant thereof) as part of the tests archive, go for it.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Before I forget, testing/mochitest/tests/Harness_sanity/mochitest.ini is an example of a file where the tests are out of order.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Gregory Szorc [:gps] from comment #9)
> Any chance you can use the code from python/mozbuild/mozbuild/testing.py to
> do the searching and filtering? A lot of this code seems redundant and
> consolidating to a single code path will really help with things such as
> implementing automatic test flavor detection and dispatching for |mach
> test|. If you need to move that code into mozbase and upload all-tests.json
> (or a variant thereof) as part of the tests archive, go for it.

I feel like most of the complexity of this patch is in dealing with test naming and sorting. Once we fix some of the subsidiary bugs, those issues will go away. I think it makes more sense to unify the code at that point.

Also, it's not clear to me that we want to use all-tests.json instead of the master manifest files. The latter actually seem more useful to me because they already include all the conditional information.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-manifests v2

This fixes all the remaining issues that I'm aware of. I verified that we run exactly the same tests, in the same order, with and without the patch (except for bug 984002, which doesn't really matter). Everything works fine on android; there are no changes required to runtestsremote.py.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to Bill McCloskey (:billm) from comment #10)
> Before I forget, testing/mochitest/tests/Harness_sanity/mochitest.ini is an
> example of a file where the tests are out of order.

Presumably you mean "alphabetical order"? I don't think that's important. We should just run them in the order they are listed in the manifest, that feels sane. (I'm assuming that manifestdestiny returns them in that order, and not some arbitrary ordering.)

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8391731 [details] [diff] [review]
test-manifests v2

Review of attachment 8391731 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, I just have one quibble. Either convince me or change that and it's an easy r+.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +570,5 @@
> +            # including manifests from [include:foo] directives.
> +            for mpath in m.manifests():
> +                mpath = mozpath.normpath(mpath)
> +                out_path = mozpath.join(out_dir, mozpath.basename(mpath))
> +                obj.installs[mpath] = (out_path, False)

This would probably make sense to be split out as a separate patch, since it's fixing a bug in our manifest copying code, but I'm not going to make you do busywork for that. It would be good to add a unit test for this, though:
http://hg.mozilla.org/mozilla-central/annotate/77d29cbd8c47/python/mozbuild/mozbuild/test/frontend/test_emitter.py#l299

The data files for those tests are here:
http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/frontend/data/

::: testing/mochitest/runtests.py
@@ +373,5 @@
> +      return "webapprt-chrome"
> +    else:
> +      return "mochitest"
> +
> +  def isTest(self, options, filename):

Can you mention bug 983867 in a comment here, so we can remove this once we've cleaned that up?

@@ +415,5 @@
> +
> +  def buildTestURL(self, options):
> +    testHost = "http://mochi.test:8888"
> +    testPath = self.getTestPath(options)
> +    testURL = ("/").join([testHost, self.TEST_PATH, testPath])

Ditch the extraneous parens around the string while you're here.

@@ +436,1 @@
>      if options.manifestFile and os.path.isfile(options.manifestFile):

Can you file a followup on removing non-manifest codepaths here and elsewhere? We should be able to remove server.js in favor of a static index.html, for example.

@@ +465,5 @@
>          if test.has_key('disabled'):
>            testob['disabled'] = test['disabled']
>          paths.append(testob)
>  
> +      # Sort tests so they are run in a deterministic order.

I'd rather not do this.

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ +603,5 @@
>          return manifests in order in which they appear in the tests
>          """
>          if tests is None:
> +            # Make sure to return all the manifests, even ones without tests.
> +            return self.manifest_defaults.keys()

This could probably also stand a unit test in the manifestdestiny test suite.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> (In reply to Bill McCloskey (:billm) from comment #10)
> > Before I forget, testing/mochitest/tests/Harness_sanity/mochitest.ini is an
> > example of a file where the tests are out of order.
> 
> Presumably you mean "alphabetical order"? I don't think that's important. We
> should just run them in the order they are listed in the manifest, that
> feels sane. (I'm assuming that manifestdestiny returns them in that order,
> and not some arbitrary ordering.)

That's not what we do right now, of course... It seems fine if it doesn't break anything, but I wouldn't put money on that.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, as Ms2ger said, we're already sorting in server.js. The pessimist in me is worried that if we reorder the tests, they'll start to fail. I did a try push with the sorting removed. If it's green, I'm happy to remove the sort. If it's not, I would strongly favor keeping the sort and fixing the tests as a follow-up.

In the meantime, I'll work on some unit tests.

Comment 17

[(not currently active) Ted Mielczarek]

Okay, if it works without the sort then let's do that. If it does not, then you can have r=me to land the patch as-is.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is the try push:
https://tbpl.mozilla.org/?tree=Try&rev=118f43aa81ca

It's not looking too good. There are two failures that I think are my fault ("TypeError: active_tests() keywords must be strings"). I'll fix those. But the rest seem like the sort of thing one might expect to happen when reordering tests. I think we should leave the sorting as-is and file a bug to follow up on it.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-manifests v3

This addresses all the review comments except for the path sorting.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8392563 [details] [diff] [review]
test-manifests v3

Review of attachment 8392563 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Can you file a followup on the test manifest sorting? I'd be fine with fixing that by just sorting all the manifest contents, but that's kind of a PITA since manifestdestiny doesn't expose an API for that.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8392563 [details] [diff] [review]
test-manifests v3

Review of attachment 8392563 [details] [diff] [review]:
-----------------------------------------------------------------

This patch and all the work around manifests is making me weep with joy.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +570,5 @@
> +            # including manifests from [include:foo] directives.
> +            for mpath in m.manifests():
> +                mpath = mozpath.normpath(mpath)
> +                out_path = mozpath.join(out_dir, mozpath.basename(mpath))
> +                obj.installs[mpath] = (out_path, False)

I think this will also resolve bug 941319!

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks. I found one more problem. You can see it in this try push:
https://tbpl.mozilla.org/?tree=Try&rev=52a1340e65e7

Python is complaining that a dictionary key can't be used as a keyword argument. TBPL helpfully pointed me to bug 964379. The problem only happens on linux debug mochitests, which run on our own special Linux slaves. Based on philor's detective work, it looks like they still run python 2.6. I suspect it may be another issue with unicode keys and an outdated python version.

What should we do? It seems like we could write a gross patch like the one in bug 964379, but I'm not sure where it should go. Maybe in find_and_update_from_json?

Comment 23

[Gregory Szorc [:gps]]

Yes, a gross patch is in order. Hack it as close to the exception point as possible: we want to use Python 3 compatible unicode (over str) for as much code as possible to maximize our exposure to Python 3 compatibility issues sooner rather than later.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: mozinfo-fix

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8392665 [details] [diff] [review]
mozinfo-fix

Review of attachment 8392665 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/runtests.py
@@ +450,5 @@
> +      # rid ourselves of 2.6.
> +      info = {}
> +      for k, v in mozinfo.info.items():
> +        if isinstance(k, unicode):
> +          k = k.encode('ascii')

Why not utf-8 here? I guess if we ever see non-ascii this will throw. I wouldn't worry too much about it unless this actually fails.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Filed bug 984911 for the test sorting.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android 2.2 robocop bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/683b27fa1812

https://tbpl.mozilla.org/php/getParsedLog.php?id=36339649&tree=Mozilla-Inbound

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This should fix the robocop issue.
https://hg.mozilla.org/integration/mozilla-inbound/rev/023f5ed842c8

As a side note, it's confusing that pushing to try with |-u mochitests| doesn't run robocop, since it uses the mochitest test runner and appears in the M(...) list on tbpl.

Comment 29

[Phil Ringnalda (:philor)]

That's one of the very very few things about tryparser that would actually be easy to fix, http://mxr.mozilla.org/build/source/buildbotcustom/try_parser.py#16

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/023f5ed842c8

Comment 31

[(not currently active) Ted Mielczarek]

Great work, thanks Bill!

Comment 33

[Marco Castelluccio [:marco]]

I suspect this may have caused bug 986163. Have you tried to run webapprt tests with this patch?

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Canceling needinfo based on bug 986163.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2d71607466d