Closed Bug 868158 Opened 11 years ago Closed 11 years ago

mochitests should support manifest format

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: jmaher, Assigned: jmaher)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mozbase])

Attachments

(3 files, 5 obsolete files)

mochitest support for manifests (2.0)
11.67 KB, patch
ted
: review+
Details | Diff | Splinter Review
changes needed to support nits and bitrot (1.1)
23.58 KB, patch
ted
: review+
Details | Diff | Splinter Review
combined patch (original + nits)
21.75 KB, patch
Details | Diff | Splinter Review 
after much deliberation and meetings, I finally have something to show for this.  Mochitests of all flavors can reap the rewards of all the cooler test harnesses by using manifests.  Better yet, there is a tool called manifestdestiny which works well with mochitest.
this patch can apply to m-c and run tests by using the --test-manifest cli flag. 

todo:
1) ensure the code is clean and solid
2) remove some duplicated in server.js/chrome-harness.js
3) resolve what to do with android.json which currently uses -test-manifest
4) ensure --test-path works with this
5) hook up filtering which is done via manifestparser

Once this patch is checked in, we can discuss the route for creating and integrating the specific manifest files into the tree and build system.  I would like somebody to pick that part up so we can start having a crystal clear list of next steps once this bug is resolved.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Comment on attachment 744799 [details] [diff] [review]
manifest support for mochitest (WIP)

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

::: testing/mochitest/runtests.py
@@ +29,5 @@
> +
> +try:
> +  import json
> +except:
> +  import simplejson as json

I'm 95% sure we have a modern enough Python everywhere now that you don't need to import simplejson.
gps- can you help figure out how we could integration TEST_MANIFEST=mochitest.ini into moz.build such that it could replace the Makefile logic for defining and copying files?

jhammel- can you help figure out in manifestparser.py how we should list dependency files?  I assume it would be:
[test_case.html]
skip-if = os=='linux'
depends = setup.js, test_case_window.html, test_case.js

ted- how do you think we should handle the scenarios where we depend on files from other directories?  adding 'depends ../../../media/tests/script.js' doesn't seem valid, but we usually copy those files into our existing directory...we need a solution.
First, read https://developer.mozilla.org/en-US/docs/How_Mozilla%27s_build_system_works. Then look at the patches in bug 846634, bug 818246, and bug 864774 for examples.

Also, don't feel obliged to write the build system integration yourself. The patch may likely be more involved than your liking. We have joey and mshal available to work on moz.build work full time.

The priority for moz.build files is currently getting stuff in them quickly, ideally with a good definition language. Correctness and optimized build system experience can be improved in follow-up bugs. What I'm trying to say is if you make something quick and dirty that piggybacks off existing rules in rules.mk, that's fine for the short term.
Blocks: nomakefiles
Specifying relative paths to dependent files seems fine, we have lots of reftests that do HTTP(../..) to be able to access files from other dirs. Clearly there's a need to do this, so we shouldn't make it hard for developers.
I agree we should keep it as simple as possible while encouraging as much code reuse as possible.  My concern is using a manifest format to specify the dependent files and how we support that in the manifest itself.

Would we do something like this?:
[test_case.html]
depends-on = ../../../chrome/tests/utils.js, test_case_window.html
That seems reasonable, but I worry that we're adding overhead here for minimal gain. If someone fails to specify these files but they get copied anyway (via other tests), the tests will probably just work, right? But then if a reference to that file got removed elsewhere they'd just randomly break.
yeah, I am concerned about that as well.  My only line of questioning for this is that we need to solve moving over to moz.build files and the general consensus on the phone call a month ago was that we would specify all files in the manifest (i.e. mochitest.ini) that are currently specified in the Makefile.in file right now.
ok, I didn't think this through so well, but there are two types of files:
1) files which are copied into the directory (as we have in Makefile.in right now)
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/Makefile.in#42

2) files which are referenced from inside the test case but not in the current directory:
http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_defaultMuted.html?force=1#8


I believe item #1 is easy to solve and what we have all agreed upon that moz.build could parse and ensure we copy.

For item #2, that is a scenario that would be nice to just copy the files and keep them all locally when we create a tests.zip package (otherwise symlink).

In general, we should choose a standard and enforce it whenever possible inside of mochitest.  Personally I vote for removing option #2 and making it all conform to #1.
FWIW I support :jmaher's general plan in comment 9 ; that is, copy all files into the directory, if possible/not too much work.  I also support the `depends-on` syntax as given in comment 6.  Personally, I'd prefer whitespace as a separator vs. comma separators.  If we do use comma separators, I advise using the csv module, which will of course enforce the csv format ( http://tools.ietf.org/html/rfc4180 ):

>>> csv.reader(['foo,bar,fleem']).next()
['foo', 'bar', 'fleem']

Note also that as per ConfigParser .ini syntax, these value parts of `key = value` may span lines, if useful: https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L358
I'd have to see the specifics of what tests are doing what, but I know we have quite a few media tests that use common media files, so we should make sure that we're not going to wind up copying a bunch of large media files into separate test dirs if we go down that route.
this supports --test-path, all test types, and filtering.  There is no clean solution for >1 manifest, but before we are ready to run with manifests, we will need to convert the android.json into this manifest format.

We could use a different cli flag (i.e. --mochitest-manifest) instead of --test-manifest.
Attachment #744799 - Attachment is obsolete: true
Attachment #749335 - Flags: review?(ted)
Comment on attachment 749335 [details] [diff] [review]
add backend support for manifests in all mochitests (1.0)

one issue with android, will upload a new patch when it is ready.
Attachment #749335 - Flags: review?(ted)
updated patch to fix android to not use testManifest internally.  Both android and b2g use the --run-only-tests cli flag, so we are safe here.
Attachment #749335 - Attachment is obsolete: true
Attachment #750519 - Flags: review?(ted)
Comment on attachment 750519 [details] [diff] [review]
add backend support for manifests in all mochitests (1.1)

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

This looks like the right approach, but I have a short novella I'd like you to read.

::: testing/mochitest/runtests.py
@@ +317,4 @@
>          self.error("%s not found, cannot automate VMware recording." %
>                     mochitest.vmwareHelperPath)
>  
> +    if options.testManifest != None and options.runOnlyTests != None:

Can you switch this to just:
if options.testManifest and options.runOnlyTests:

@@ +322,1 @@
>        

Can you remove this trailing whitespace while you're here?

@@ +513,5 @@
>      return os.path.normpath(os.path.join(self.oldcwd, os.path.expanduser(path)))
>  
>    def buildTestPath(self, options):
> +    # Handle filenames in mozInfo
> +    # TODO: do we want to pass this in via the cli

The reason we have that for xpcshell is to support the running-from-objdir case:
http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#283

We clearly need to make that work, although the xpcshell situation is awkward. You could conditionally use the method I added in bug 855262 to locate mozinfo.json in the root of the objdir when running from an objdir, something like:
try:
  from mozbuild.base import MozbuildObject
  build = MozbuildObject.from_environment()
  mozInfoFile = os.path.join(build.objdir, "mozinfo.json")
except ImportError:
  mozInfoFile = None

that plus the "look next to the harness" behavior:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#143

should make it work in both cases (and now you can probably see why I'd like to wrap this behavior up in a module somewhere.)

@@ +514,5 @@
>  
>    def buildTestPath(self, options):
> +    # Handle filenames in mozInfo
> +    # TODO: do we want to pass this in via the cli
> +    mozInfoFile = 'mozinfo.json'

I'd prefer if you explicitly used SCRIPT_DIR (or SCRIPT_DIRECTORY, I think they're the same) here, so that we might someday be able to get rid of the os.chdir.

@@ +516,5 @@
> +    # Handle filenames in mozInfo
> +    # TODO: do we want to pass this in via the cli
> +    mozInfoFile = 'mozinfo.json'
> +    _mozInfo = json.loads(open(mozInfoFile).read())
> +    mozinfo.update(_mozInfo)

We should add a method to mozinfo to load a JSON file directly. Can you file a bug on that?

@@ +518,5 @@
> +    mozInfoFile = 'mozinfo.json'
> +    _mozInfo = json.loads(open(mozInfoFile).read())
> +    mozinfo.update(_mozInfo)
> +
> +    if options.testManifest and os.path.exists(options.testManifest):

os.path.isfile

@@ +521,5 @@
> +
> +    if options.testManifest and os.path.exists(options.testManifest):
> +      manifest = TestManifest(strict=False)
> +      manifest.read(options.testManifest)
> +      #TODO: should we return disabled tests in the list?  

I think yeah, we should be getting all tests and writing a disabled attribute in the JSON manifest so we can display something like "INFO skipping test". We can probably punt that to a followup though, maybe just file a bug and put the bug number here? (also there's trailing whitespace in this comment)

@@ +533,5 @@
> +          continue
> +
> +        paths.append({'path': tp})
> +
> +      with open(os.path.join(options.profilePath, 'tests.json'), 'w') as manifest:

You're re-using the "manifest" variable name here, you used it above.

@@ +534,5 @@
> +
> +        paths.append({'path': tp})
> +
> +      with open(os.path.join(options.profilePath, 'tests.json'), 'w') as manifest:
> +        manifest.write(json.dumps(paths))

I feel like this whole method ought to just be part of ManifestDestiny: "write out a JSON version of this manifest, optionally with test paths relative to this path". That'd force us to more-formally describe the JSON format of a Manifest too, which would be a good thing.

We can punt that to a followup as well.

::: testing/mochitest/server.js
@@ +595,5 @@
> +    Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +  var str = NetUtil.readInputStreamToString(fileInStream, fileInStream.available());
> +  fileInStream.close();
> +  return JSON.parse(str);
> +}

This is kind of gross, I was hoping to make server.js *less* complex with this change.

@@ +636,5 @@
> +  } else {
> +    [links, count] = list(metadata.path,
> +                          metadata.getProperty("directory"),
> +                          true);
> +  }

I have an alternate proposal here: write the tests.json file in the root http directory. Just put "testManifest=1" in the URL. In this code, simply use that to short-circuit the "build a list of tests" code, and don't provide a list of tests at all. In the JS harness in the browser, if "testManifest=1" is in the URL, use XHR to fetch /tests.json and populate the test array from it.

As a follow-up to that we could rip all the HTML generating logic out of server.js and just require the JS harness to do it (considering we're hiding the results table in normal execution anyway that'd probably save time + bytes).

Then, in a better world when we've made manifest use mandatory, we could eviscerate server.js (probably everything but sjs handling) and have a static index.html that does all the work in client-side JS.
Attachment #750519 - Flags: review?(ted) → review-
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> @@ +516,5 @@
> > +    # Handle filenames in mozInfo
> > +    # TODO: do we want to pass this in via the cli
> > +    mozInfoFile = 'mozinfo.json'
> > +    _mozInfo = json.loads(open(mozInfoFile).read())
> > +    mozinfo.update(_mozInfo)
> 
> We should add a method to mozinfo to load a JSON file directly. Can you file
> a bug on that?

This is already done in bug 871746. Though it might still need to be mirrored to m-c.
...and now I remember that you pointed me at that bug and I forgot about it, thanks.
Depends on: 871746
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> Comment on attachment 750519 [details] [diff] [review]
<snip/>
> @@ +513,5 @@
> >      return os.path.normpath(os.path.join(self.oldcwd, os.path.expanduser(path)))
> >  
> >    def buildTestPath(self, options):
> > +    # Handle filenames in mozInfo
> > +    # TODO: do we want to pass this in via the cli
> 
> The reason we have that for xpcshell is to support the running-from-objdir
> case:
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.
> mk#283
> 
> We clearly need to make that work, although the xpcshell situation is
> awkward. You could conditionally use the method I added in bug 855262 to
> locate mozinfo.json in the root of the objdir when running from an objdir,
> something like:
> try:
>   from mozbuild.base import MozbuildObject
>   build = MozbuildObject.from_environment()
>   mozInfoFile = os.path.join(build.objdir, "mozinfo.json")
> except ImportError:
>   mozInfoFile = None
> 
> that plus the "look next to the harness" behavior:
> http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/
> runxpcshelltests.py#143
> 
> should make it work in both cases (and now you can probably see why I'd like
> to wrap this behavior up in a module somewhere.)

A big +1 from me.  Short term, I'd put in mozbuild since we're importing it anyway.  You'd still need the try/except clause, but at least it'd be four lines (and really one line) vs several.  Longer term....guessing moztest makes sense?
(In reply to Andrew Halberstadt [:ahal] from comment #16)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> > @@ +516,5 @@
> > > +    # Handle filenames in mozInfo
> > > +    # TODO: do we want to pass this in via the cli
> > > +    mozInfoFile = 'mozinfo.json'
> > > +    _mozInfo = json.loads(open(mozInfoFile).read())
> > > +    mozinfo.update(_mozInfo)
> > 
> > We should add a method to mozinfo to load a JSON file directly. Can you file
> > a bug on that?
> 
> This is already done in bug 871746. Though it might still need to be
> mirrored to m-c.

filed bug 877733 for this
> @@ +534,5 @@
> > +
> > +        paths.append({'path': tp})
> > +
> > +      with open(os.path.join(options.profilePath, 'tests.json'),
> 'w') as manifest:
> > +        manifest.write(json.dumps(paths))

> I feel like this whole method ought to just be part of
> ManifestDestiny: "write out a JSON version of this manifest,
> optionally with test paths relative to this path". That'd force us
> to more-formally describe the JSON format of a Manifest too, which
> would be a good thing.

There's a bit to do (although, not too much) to get manifestdestiny to support JSON format.  I'd love to hash this out on a ticket.  My general vision is that both .ini and .json should support an internal format (already formally defined though undocumented, a list of dictionaries with string keys and values) and then have a format have a format interface (load, save, etc).
Blocks: 877733
No longer blocks: 877733
Depends on: 877733
I filed bug 881417 and have a patch there to implement the "find and load mozinfo.json" behavior.
Blocks: 883858
addressed previous comments, currently testing on try server, passed local tests.
Attachment #750519 - Attachment is obsolete: true
Attachment #764216 - Flags: review?(ted)
Comment on attachment 764216 [details] [diff] [review]
mochitest support for manifests (2.0)

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

I have lots of comments, but overall this is an r+ with some cleanup. Thanks for fixing it up, this looks so much nicer than the previous revision!

::: testing/mochitest/Makefile.in
@@ +43,4 @@
>  		$(topsrcdir)/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py \
>  		$(topsrcdir)/testing/mozbase/mozdevice/mozdevice/droid.py \
>  		$(topsrcdir)/testing/mozbase/mozdevice/mozdevice/Zeroconf.py \
> +		$(topsrcdir)/testing/mozbase/mozinfo/mozinfo/mozinfo.py \

We shouldn't need this since we should be able to pull mozinfo in from the virtualenv, right?

::: testing/mochitest/chrome-harness.js
@@ +323,5 @@
>  }
>  
> +function readConfig(filename) {
> +  if (filename === undefined) {
> +    filename = "testConfig.js";

Could just write this as:
filename = filename || "testConfig.js";

@@ +385,5 @@
> +      path = baseurl + '/' + params.testRoot + '/' + path;
> +      links[path] = true
> +    }
> +    return links
> +  }

If we standardize the JSON manifest format as part of ManifestDestiny we should move this all into a JS lib we can share between the plain and chrome harnesses.

::: testing/mochitest/runtests.py
@@ +263,5 @@
>      defaults["immersiveMode"] = False
>  
> +    self.add_option("--build-info-json",
> +                    action = "store", dest = "mozInfo",
> +                    help = "path to mozinfo.json to determine build time options")

I know you're copy/pasting here, but the extra spaces around the = are weird.

@@ +385,4 @@
>        if not options.repeat:
>          options.repeat = 29
>  
> +    if not options.mozInfo:

This case won't ever be true, since you specified a default for this option. Maybe you want to default it to None?

@@ +388,5 @@
> +    if not options.mozInfo:
> +      options.mozInfo = os.path.join(self._scriptdir, 'mozinfo.json')
> +
> +    if not os.path.isfile(options.mozInfo):
> +      self.error("Unable to file build information file (mozinfo.json) at this location: %s" % options.mozInfo)

I really want you to use mozinfo.find_and_update_from_json(scriptdir). I don't think this code as-written will work right for the "running from the objdir" case.

@@ +557,5 @@
> +          continue
> +
> +        paths.append({'path': tp})
> +
> +      # Bug 883865 - add this functionality into manifestDestiny

Thanks for filing the followup bugs here!

@@ +559,5 @@
> +        paths.append({'path': tp})
> +
> +      # Bug 883865 - add this functionality into manifestDestiny
> +      with open(os.path.join(options.profilePath, 'tests.json'), 'w') as manifestFile:
> +        manifestFile.write(json.dumps(paths))

Really nitpicky, but most JSON tends to have an object as the root and not an array. Apparently some parts of the JSON spec require you to have an object. Maybe this could be {"tests": paths}?

@@ +560,5 @@
> +
> +      # Bug 883865 - add this functionality into manifestDestiny
> +      with open(os.path.join(options.profilePath, 'tests.json'), 'w') as manifestFile:
> +        manifestFile.write(json.dumps(paths))
> +      options.testManifest = 'tests.json'

I don't like the reuse of options.testManifest here for both the .ini manifest and the .json manifest.

@@ +566,1 @@
>      """ Build the url path to the specific test harness and test file or directory """

You bumped the docstring of this function down, that doesn't seem right. (Might want to mention that it writes a manifest now, too.)

::: testing/mochitest/server.js
@@ +583,5 @@
> +  if (metadata.queryString.indexOf('testManifest') == -1) {
> +    [links, count] = list(metadata.path,
> +                          metadata.getProperty("directory"),
> +                          true);
> +  }

This is *much* nicer!

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +114,5 @@
>  }
>  
> +function getTestManifest() {
> +  if (params.testManifest) {
> +    var datafile = "http://mochi.test:8888/" + params.testManifest;

You should just be able to request "/" + params.testManifest, since the host will be the same.

@@ +115,5 @@
>  
> +function getTestManifest() {
> +  if (params.testManifest) {
> +    var datafile = "http://mochi.test:8888/" + params.testManifest;
> +    var objXml = new XMLHttpRequest();

It's more common to name the XHR object "req", FWIW.

@@ +116,5 @@
> +function getTestManifest() {
> +  if (params.testManifest) {
> +    var datafile = "http://mochi.test:8888/" + params.testManifest;
> +    var objXml = new XMLHttpRequest();
> +    objXml.open("GET",datafile,false);

I'm sure someone would yell at you for using a sync XHR here. If it's not a huge PITA it would be nice to make it async, but I'm not that hung up on it.

@@ +119,5 @@
> +    var objXml = new XMLHttpRequest();
> +    objXml.open("GET",datafile,false);
> +    objXml.send(null);
> +    try {
> +      testManifest = JSON.parse(objXml.responseText);

You can set .responseType = "json" and then .response will be a JSON object automatically:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest?redirectlocale=en-US&redirectslug=DOM%2FXMLHttpRequest#Properties
Attachment #764216 - Flags: review?(ted) → review+
Joel, is this good to check in?

Talking to :gps in person, he'd prefer to have working manifests in place first, but with or without them he plans on taking the tests out of makefiles and putting in .build files, say, ~1 month from now.
Whiteboard: [mozbase]
there are some small nits to fix, feel free to address them as I have a lot of my plate this week and will be on PTO next week.
Depends on: 896021
I have added a set of changes which I have made to support the nits addressed here.  As it is considerably more than a few hundred lines, this really warrants a new review.

With that said, I have tested locally many times and on try a few times and things are looking great.
Attachment #782108 - Flags: review?(ted)
Comment on attachment 782108 [details] [diff] [review]
changes needed to support nits and bitrot (1.0)

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

::: testing/mochitest/browser-harness.xul
@@ +196,4 @@
>                                    gConfig.thisChunk, gConfig.chunkByDir);
>        }
>  
> +      createTester(fileNames.map(function (f) new browserTest(f)));

We're going to drop this syntax eventually; use f => new browserTest(f) or function (f) { new browserTest(f) }.

@@ +204,4 @@
>      }
>  
>      function runTests() {
> +      listTests();

... so "listTests()" means "runTests()"? Seems like a strange name, then.

::: testing/mochitest/chrome-harness.js
@@ +352,5 @@
> +  var manifestFile = ioSvc.newFileURI(testsURI)
> +                  .QueryInterface(Components.interfaces.nsIFileURL).file;
> +
> +  Components.manager.QueryInterface(Components.interfaces.nsIComponentRegistrar).
> +    autoRegister(manifestFile);

Placement of dots near newlines and indentation is pretty inconsistent here.

::: testing/mochitest/manifestLibrary.js
@@ +1,1 @@
> +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Java?

@@ +7,5 @@
> +function parseTestManifest(testManifest, callback) {
> +  var links = {};
> +  var paths = [];
> +
> +  for (obj in testManifest['tests']) {

for (var obj...). This pattern defines a new global variable.

If testManifest['tests'] is an array, please use for .. of

@@ +9,5 @@
> +  var paths = [];
> +
> +  for (obj in testManifest['tests']) {
> +    path = testManifest['tests'][obj]['path'];
> +      dump("JMAHER: params.testroot: " + params.testRoot + "\n");

Hi jmaher!

@@ +23,5 @@
> +}
> +
> +function getTestManifest(datafile, callback) {
> +  var req = new XMLHttpRequest();
> +  req.open("GET",datafile,true);

Spaces around commas. Async defaults to true, so drop the argument.

@@ +27,5 @@
> +  req.open("GET",datafile,true);
> +  req.onload = function() {
> +    if (req.readyState == 4) {
> +      if (req.status == 200) {
> +        parseTestManifest(JSON.parse(req.responseText), callback);

You could use responseType == "json"

@@ +29,5 @@
> +    if (req.readyState == 4) {
> +      if (req.status == 200) {
> +        parseTestManifest(JSON.parse(req.responseText), callback);
> +      } else {
> +        dump("INFO: setup.js | error loading " + datafile + "\n");

Just INFO?

@@ +33,5 @@
> +        dump("INFO: setup.js | error loading " + datafile + "\n");
> +      }
> +    }
> +  }
> +  req.send(null);

We haven't needed the argument since Fx3, IIRC.

@@ +51,5 @@
> +    return gTestList;
> +  }
> +
> +  var datafile = "http://mochi.test:8888/" + filterFile;
> +  var objXml = new XMLHttpRequest();

req

@@ +52,5 @@
> +  }
> +
> +  var datafile = "http://mochi.test:8888/" + filterFile;
> +  var objXml = new XMLHttpRequest();
> +  objXml.open("GET",datafile,false);

Commas. Sync? :/

@@ +65,5 @@
> +  if ('runtests' in filter) {
> +    runtests = filter.runtests;
> +  }
> +  if ('excludetests' in filter)
> +    excludetests = filter.excludetests;

{}

@@ +70,5 @@
> +  if (!('runtests' in filter) && !('excludetests' in filter)) {
> +    if (runOnly == 'true') {
> +      runtests = filter;
> +    } else
> +      excludetests = filter;

{}

@@ +79,5 @@
> +  // filteredTests.
> +  if (Object.keys(runtests).length) {
> +    for (var i = 0; i < gTestList.length; i++) {
> +      var test_path = gTestList[i];
> +      var tmp_path = test_path.replace(/^\//, '');

JS doesn't do underscores

@@ +93,5 @@
> +        }
> +      }
> +    }
> +  }
> +  else {

Cuddle else

@@ +99,5 @@
> +  }
> +
> +  // Continue with filteredTests, and deselect everything that's in
> +  // excludedtests.
> +  if (Object.keys(excludetests).length) {

How about

if (!...) {
  return filteredTests;
}

...
return refilteredTests;

::: testing/mochitest/mochitest_options.py
@@ +273,4 @@
>          { "action": "store",
>            "type": "string",
>            "dest": "testManifest",
> +          "help": "JSON list of tests to specify 'runtests'.  Old format for mobile specific tests",

Single space after period.

We'll be removing this? Perhaps use "deprecated".

::: testing/mochitest/runtests.py
@@ +250,5 @@
>        if options.runSlower:
>          self.urlOpts.append("runSlower=true")
>  
>    def buildTestPath(self, options):
> +    """ 

Trailing ws.

@@ +276,4 @@
>          paths.append({'path': tp})
>  
>        # Bug 883865 - add this functionality into manifestDestiny
> +      with open(os.path.join('tests.json'), 'w') as manifestFile:

os.path.join('tests.json')? What does that do?

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +203,5 @@
> +
> +  if (params.testManifest) {
> +    datafile = params.testManifest;
> +  } else if (params.manifestFile) {
> +    datafile = params.manifestFile;

Could be

  var datafile = params.testManifest || params.manifestFile || "";
updated to support the large majority of nits that :Ms2ger mentioned!  All well needed for cleanup.  Looking good on try except for the perma oranges on windows debug which are seen on inbound.
Attachment #782108 - Attachment is obsolete: true
Attachment #782108 - Flags: review?(ted)
Attachment #782337 - Flags: review?(ted)

        Attachment #782109 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 782337 [details] [diff] [review]
changes needed to support nits and bitrot (1.1)

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

Just some nits here, overall looks good.

::: testing/mochitest/browser-harness.xul
@@ +169,5 @@
>  
>      // Returns an array of browserTest objects for all the selected tests
> +    function runTests() {
> +      params = gConfig;
> +      params.baseurl = "chrome://mochitests/content";

It took me a bit to realize that you're setting params as a global here so you can use it from manifestLibrary.js. :-/

::: testing/mochitest/mach_commands.py
@@ +137,4 @@
>          options.testingModulesDir = os.path.join(tests_dir, 'modules')
>          options.extraProfileFiles.append(os.path.join(self.distdir, 'plugins'))
>          options.symbolsPath = os.path.join(self.distdir, 'crashreporter-symbols')
> +        options.mozInfo = os.path.join(self.topobjdir, 'mozinfo.json')

This stuff would be unnecessary if you could just use the method I added in bug 881417...

::: testing/mochitest/manifestLibrary.js
@@ +15,5 @@
> +  }
> +
> +  for (var obj of testManifest['tests']) {
> +    path = obj['path'];
> +    if (params.testRoot != 'tests' && params.testRoot !== undefined) {

I'd prefer if params was passed in as a parameter, sharing globals across script files is confusing.

@@ +18,5 @@
> +    path = obj['path'];
> +    if (params.testRoot != 'tests' && params.testRoot !== undefined) {
> +      links[params.baseurl + '/' + params.testRoot + '/' + path] = true
> +    } else {
> +      gTestList.push(params.testPrefix + path);

Given that this is a separate file, it might be nicer to pass gTestList in as a parameter.

Also, this logic is a little confusing, can you put a comment here explaining why some things go into links and some directly into gTestList?

@@ +24,5 @@
> +  }
> +  callback(links);
> +}
> +
> +function getTestManifest(datafile, callback) {

I probably would have gone with "url" for the parameter.

@@ +32,5 @@
> +    if (req.readyState == 4) {
> +      if (req.status == 200) {
> +        parseTestManifest(JSON.parse(req.responseText), callback);
> +      } else {
> +        dump("TEST-ERROR: setup.js | error loading " + datafile + "\n");

This probably wants an error callback that will quit the harness, doesn't it? Or will it just quit because there are no tests to run anyway?

@@ +42,5 @@
> +
> +// Test Filtering Code
> +
> +// Open the file referenced by runOnly|exclude and use that to compare against
> +// gTestList.  Return a modified version of gTestList

Your comment says gTestList, but you seem to use the testList parameter. Actually this whole comment is kind of confusing. Could you spell out what all the parameters do? I also don't understand the "open the file" bit. Is that just leftover from old code?

@@ +117,5 @@
> +    if (!found) {
> +      refilteredTests.push(testpath);
> +    }
> +  }
> +  filteredTests = refilteredTests;

This reassignment doesn't seem super useful, just return refilteredTests.

::: testing/mochitest/mochitest_options.py
@@ +279,5 @@
> +        [["--manifest"],
> +        { "action": "store",
> +          "type": "string",
> +          "dest": "manifestFile",
> +          "help": "JSON list of tests to specify 'runtests'.",

The "runtests" is a little confusing. Also, is this actually supposed to be a JSON file, or a ManifestDestiny ini file?

::: testing/mochitest/runtests.py
@@ +252,5 @@
>  
>    def buildTestPath(self, options):
> +    """
> +        Build the url path to the specific test harness and test file or directory
> +        Build a manifest of tests to run and write out a json file for the harness to read

The indentation here is a little wonky.

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +130,5 @@
>  var RunSet = {}
>  RunSet.runall = function(e) {
>    // Filter tests to include|exclude tests based on data in params.filter.
>    // This allows for including or excluding tests from the gTestList
> +  if ("testManifest" in params && params.testManifest) {

I think you can safely just say if (params.testManifest) {

@@ +131,5 @@
>  RunSet.runall = function(e) {
>    // Filter tests to include|exclude tests based on data in params.filter.
>    // This allows for including or excluding tests from the gTestList
> +  if ("testManifest" in params && params.testManifest) {
> +    getTestManifest("http://mochi.test:8888/" + params.testManifest, function(filter) { gTestList = filterTests(filter, gTestList, params.runOnly); RunSet.runtests(); });

You should just be able to fetch "/" + params.testManifest, right?

        Attachment #782337 -
        Flags: review?(ted) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c116372d7ad4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

    
    

    
  
This breaks mochitest-browser tests on Mac for me, filed bug 901136.

    
  
Blocks: 901990

    
  
Depends on: 906045

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: