Closed Bug 938019 Opened 6 years ago Closed 6 years ago

Run mochitests from manifests

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: markh, Assigned: billm)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attachment #831336 - Flags: feedback?(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.
Attachment #831336 - Flags: feedback?(gps) → feedback+
(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.
Attached patch test-manifests (obsolete) — Splinter Review
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.
Assignee: nobody → wmccloskey
Attachment #831336 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8389649 - Flags: feedback?(ted)
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?
Attachment #8389649 - Flags: feedback?(ted) → feedback+
Summary: Discover mochitests through metadata, not filesystem → Run mochitests from manifests
(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.
has this patch been tested for android?  I don't see any changes to runtestsremote.py.  It would be great if none were needed.
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
> ::: 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.
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.
Before I forget, testing/mochitest/tests/Harness_sanity/mochitest.ini is an example of a file where the tests are out of order.
(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.
Attached patch test-manifests v2 (obsolete) — Splinter Review
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.
Attachment #8389649 - Attachment is obsolete: true
Attachment #8391731 - Flags: review?(ted)
(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 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.
Attachment #8391731 - Flags: review?(ted)
(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.
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.
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.
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.
This addresses all the review comments except for the path sorting.
Attachment #8391731 - Attachment is obsolete: true
Attachment #8392563 - Flags: review?(ted)
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.
Attachment #8392563 - Flags: review?(ted) → review+
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!
Blocks: 941319
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?
Flags: needinfo?(ted)
Flags: needinfo?(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.
Flags: needinfo?(ted)
Flags: needinfo?(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.
Attachment #8392665 - Flags: review?(gps) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/023f5ed842c8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Great work, thanks Bill!
Duplicate of this bug: 938496
I suspect this may have caused bug 986163. Have you tried to run webapprt tests with this patch?
Flags: needinfo?(wmccloskey)
Canceling needinfo based on bug 986163.
Depends on: 986163
Duplicate of this bug: 461349
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.