508664 - lots of mochitest harness features don't work when running a single test file

Status: RESOLVED DUPLICATE of bug 1014062

(Testing :: Mochitest, defect)

Description

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

Attachment: simpletest.js hack for logging and cli parsing

for windows mobile, I have found that we need to run mochitest without iframe support which boils down to executing 1 test file at a time.  We need to use the js file logging that is built into mochitest.

I have a patch for SimpleTest.js which will simulate the mochitest test harness that usually is there.  This patch doesn't interfere with a normal run of mochitest, so we have the option to put this in the core code.

I will also attach a simple runner.py script which will find and run the tests using the hacked version of SimpleTest.js.

Comment 1

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

Attachment: Simple script to run 1 file at a time through mochitest

use this with the simpletest.js patch to run and collect results of mochitest 1 file/time.

Comment 2

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

this won't work for the dom-level1-core, dom-level2-core and dom-level2-html test suites.  Each of these test suites have a DOMTestCase.js file which overrides the SimpleTest._logResult function:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/dom-level1-core/DOMTestCase.js#637

Commenting this _logResult function out allows the majority of the dom tests to run one at a time.

Comment 3

[cmtalbert]

Rob, It looks like you were the primary one adding in the DOM level 1 suites at least.  Why did they need to arrive with their own logging?  Can we just make the logging in the DOM tests be a wrapper around SimpleTest.js's standard logging?

Comment 4

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

Attachment: Patch for logging in simpletest.js

Updated patch to comment out dom specific logging.

Comment 5

[cmtalbert]

Comment on attachment 396496 [details] [diff] [review]
Patch for logging in simpletest.js

We shouldn't comment out stuff and leave it in.  We should just take it out if we don't need it.  We need to figure out if we need it. Sayrer, why did you add this code, and is it still needed?  The fact that it runs just fine with this commented out seems to indicate to me that we can remove it.  I'll do more testing on a normal Firefox build with this to be certain.

Quit.js and MozillaFileLogger.js:
It looks (from these comments) that you are wholesale copying code from somewhere else.  Why can't we just use the code from its original location?  I don't like creating duplicate copies of code because that generates a maintenance nightmare down the road.  Forgive me if that's not what you're doing.

Additions to remove testharness overhead:
What overhead does this remove?  I don't understand how this removes overhead.

I believe the changes at the end are required for the special case to remove the DOM specific logging, is that correct?  Those look good.  I'll run this on a normal build for some testing with "normal" mochitests (not 1 by 1).

I'd like to get the concerns up above worked out before I grant this review so r-.

Comment 6

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

Attachment: patch to allow for single mochitest test file logging

updated for bitrot.

Comment 7

[cmtalbert]

Attachment: Updated patch that applies against 1.9.2

Comment 8

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

Attachment: log output from a single mochitest

updated this patch to prevent bitrot and logical errors.  This patch will allow for the use of the logFile param in the url and will send output to the specified log file.  For example:
http://localhost:8888/tests/mytest.html?logFile=/home/joel/mytest.log

This test also does --autorun by default for a single file.  This is the same behavior as before this patch.  In addition I also do --close-when-done by default.

All of the logic I do to determine if I am a single test is by checking if parentRunner == undefined.  This appears to work on linux desktop (1.9.2 and trunk) as well as windows mobile.

Comment 9

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

Comment on attachment 406077 [details] [diff] [review]
log output from a single mochitest

Ted, same as the remote server bug, this is a simple version of the mochikit harness for a single test file.

Comment 10

[cmtalbert]

Comment on attachment 406077 [details] [diff] [review]
log output from a single mochitest

A quick drive by...

>diff --git a/testing/mochitest/tests/SimpleTest/SimpleTest.js b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>--- a/testing/mochitest/tests/SimpleTest/SimpleTest.js
>+++ b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>@@ -18,12 +18,143 @@ if (typeof(SimpleTest) == "undefined") {
>     var SimpleTest = {};
> }
> 
>+SimpleTest.onComplete = null;

>+    /**
>+     * from MozillaFileLogger.js
>+     **/
>+    try {
>+        netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+        const Cc = Components.classes;
>+        const Ci = Components.interfaces;
I worry that this is going to throw an error when a test declares these same constants.  I think these should be removed from the patch and we should just spell out the directives.

I believe I may have actually run into that last night when testing this code on wince.

Otherwise I think the patch looks good, but I'll let Ted be the final arbiter on that.

Comment 11

[(not currently active) Ted Mielczarek]

You're running this all with --test-path, right? I wonder if it would make sense to just hack --test-path so it works the same way in normal Mochitest as mochitest-chrome and mochitest-a11y, and just pass &testPath=foo/bar. Then we could fix the harness to only run the tests in testPath, so you'd just get the full harness page loaded, and run the individual test in the iframe. Does that make sense?

The simplest way to do that might be to hack testListing in server.js:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#510
which is what produces the index page containing the list of tests. You'd have to pull testPath out of metadata.queryString there and filter down the results.

What do you think?

Comment 12

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

Actually I am running it like this:
./fennec -no-remote -profile <path>/profile http://<server>:8888/tests/mytestfile.html?logFile=testfile.log

There are two reason for going this route:
1) running over a remote connection to device with no python requires us to issue a raw ./fennec command line and keep our local data minimal
2) bug 507410 - causing a crash of Fennec while loading the mochikit profile on windows mobile.  So I needed to find a way to run without the mochikit and iframe overhead.

I agree about fixing the logging for --test-path.

Comment 13

[(not currently active) Ted Mielczarek]

Ok, so you could still make the fix I suggested, and just run ... http://<server>:8888/tests/?testPath=mytestfile.html&logFile=testfile.log

Comment 15

[(not currently active) Ted Mielczarek]

The duplicate bug is another data point in favor of my suggestion. He had a test that failed when run in the iframe, but passed when run standalone. We should run individual tests in the full harness page with the iframe when using --test-path.

Comment 17

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

Attachment: Run single test in mochitest framework

This allows for the --test-path parameter to accept directories or files as before, but this time if a file is used, we load it up in the mochikit framework with the iframe.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 409998 [details] [diff] [review]
Run single test in mochitest framework

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>@@ -412,6 +412,8 @@
>   if options.browserChrome:
>     makeTestConfig(options)
>   else:
>+    if options.testPath:
>+      urlOpts.append("testPath=%s" % options.testPath)


I think this will get double-appended for the mochitest-{chrome,a11y} cases, since they do this same thing here:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py.in#393

You could just remove those two sections, and consolidate it into the one you added. Also, per those other ones, you should use encodeURIComponent on testPath.

>diff --git a/testing/mochitest/server.js b/testing/mochitest/server.js
>--- a/testing/mochitest/server.js
>+++ b/testing/mochitest/server.js
>@@ -318,7 +318,7 @@
>  * Builds an optionally nested object containing links to the
>  * files and directories within dir.
>  */
>-function list(requestPath, directory, recurse)
>+function list(requestPath, directory, filename, recurse)
>+
>+  if (!dir.exists()) {
>+    return 0;
>+  }

I don't think you can just return 0 here, can you? The function returns [list, count], so you should return [[], 0]. It seems like you could avoid this change entirely by just verifying that the dir exists in the testListing function.

>-        links[key] = true;
>+        if (filename == "") {
>+          links[key] = true;
>+        } else if (key.indexOf(filename) >= 0) {
>+          links[key] = true;
>+        }

Any reason you don't just say if (file.leafName == filename) here?


>+/**
>+ *  Convert relative link to queryString style
>+ */
>+function makeActiveLink(link)
>+{
>+  var parts = link.split('/');
>+  var newLink = "";
>+  if (parts.length < 3) {
>+    return link;
>+  }
>+
>+  for (var i = 2; i < parts.length; i++) {
>+    newLink += parts[i]
>+    if (i < (parts.length - 1))
>+      newLink += "/";
>+  }
>+  var retVal = "/tests/?testPath=" + newLink + "&autorun=1";
>+  return retVal;
>+}

This seems overly complicated. Why not just:
if (link.substr(0, 7) == "/tests/")
    return "/tests/?autorun=1&testPath=" + link.substr(7);
return link;

Do these need to have "autorun=1" on them? We have a "Run Tests" link you can click. Also the "active link" naming doesn't really mean much to me. Maybe you could just name the function more literally for what it does?

>+
> 
> /**
>  * Transform nested hashtables of paths to nested HTML lists.
>@@ -403,11 +433,12 @@
>         bug_num = bug_title[0].match(/\d+/);
>     }
> 
>+    var activeLink = makeActiveLink(link);
>     if ((bug_title == null) || (bug_num == null)) {
>-      response += LI({class: classVal}, A({href: link}, link), children);
>+      response += LI({class: classVal}, A({href: activeLink}, link), children);
>     } else {
>       var bug_url = "https://bugzilla.mozilla.org/show_bug.cgi?id="+bug_num;
>-      response += LI({class: classVal}, A({href: link}, link), " - ", A({href: bug_url}, "Bug "+bug_num), children);
>+      response += LI({class: classVal}, A({href: activeLink}, link), " - ", A({href: bug_url}, "Bug "+bug_num), children);
>     }
> 
>   }

I don't think you need these changes in linksToListItems. AFAICT, that's only used in the regularListing function, which isn't to produce test listings.

>@@ -427,11 +458,12 @@
> 
>     spacer = "padding-left: " + (10 * recursionLevel) + "px";
> 
>+    var activeLink = makeActiveLink(link);

The "active link" term is pretty confusing here, where you now have "link" and "activeLink" variables.

>@@ -509,11 +541,44 @@
>  */
> function testListing(metadata, response)
> {
>-  var [links, count] = list(metadata.path,
>-                            metadata.getProperty("directory"),
>+  var path = metadata.path;
>+  var filename = "";

I'd prefer you used null as an empty value for filename.

>+
>+  if (metadata.queryString != "") {
>+    var args = metadata.queryString.split('&');
>+    for (var i = 0; i < args.length; i++) {
>+      var pair = args[i].split('=');
>+      if (decodeURIComponent(pair[0]) == "testPath") {
>+        path = decodeURIComponent(pair[1]);

Don't reuse the path variable here as a temporary, it's confusing.

>+        
>+

Be careful not to stick in extra blank lines while you're here. One is ok, two is excessive.

>+        var parts = path.split('/');
>+        if (parts[parts.length - 1] == "") {
>+          parts.pop();
>+        }
>+        filename = parts[parts.length - 1];

I guess there's no easy way to detect if this is actually a filename and not a directory until you get the nsIFile below.

>+        var f = metadata.getProperty("directory");
>+        path = "/tests";
>+        for (var i = 0; i < parts.length; i++) {
>+          if (parts[i] != "" && !isTest(parts[i])) {

isTest feels like the wrong thing here. I'd suggest skipping that test, and then below:

>+            path += "/";
>+            f.append(parts[i]);

if (f.exists() && f.isDirectory()
>+            path += parts[i]

>+          }
>+        }
>+        f.append(filename);

You should probably check f.exists() here, and just throw("File not found: " + f.path) if it doesn't.

>+        metadata._ensurePropertyBag();
>+        metadata._bag.setPropertyAsInterface("directory", f.parent);

Instead of resetting the "directory" entry on metadata, it seems like it'd be cleaner to just do:
var directory = metadata.getProperty("directory");
if (metadata.queryString != "") {
 ...
 directory = f.parent;
}

Then just use directory in the call to list().

>+      }
>+    }
>+  }
>+
>+  var [links, count] = list(path,
>+                            metadata.getProperty("directory"), filename,
>                             true);
>   dumpn("count: " + count);
>-  var tests = jsonArrayOfTestFiles(links);
>+
>+  var tests = jsonArrayOfTestFiles(links, filename);

I think this change is unnecessary. (You didn't change jsonArrayOfTestFiles, and you shouldn't have to.)

Overall this looks like a good approach, just needs some cleanup.

Comment 20

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

this is not required for tests to run on winmo or remotely, but is a nice to have.

Comment 21

[(not currently active) Ted Mielczarek]

I bet it'd be really useful for stuff like bug 551256, where we want to run a unit test repeatedly until it fails.

Comment 23

[Nathan Froyd [:froydnj]]

Attachment: cleaned up, rebased patch

Since I got annoyed that close-when-done didn't work with single tests, Ted pointed me to this bug.  I rebased the patch and addressed Ted's comments on the earlier patch, probably introducing more problems by doing so.

Ted, WDYT about this version?  It seems to work for basic mochitests for me, though I haven't tried it with other tests...

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 634494 [details] [diff] [review]
cleaned up, rebased patch

I don't have time to look at this at the moment, and I'm on vacation next week, so if you'd like to get this done sooner you can request review from someone else. Otherwise I can look at it when I get back.

Comment 25

[Nathan Froyd [:froydnj]]

Waiting-a-month review ping.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 634494 [details] [diff] [review]
cleaned up, rebased patch

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

::: testing/mochitest/runtests.py
@@ +579,5 @@
> +        path = ("/").join([self.TEST_PATH, options.testPath])
> +        if options.repeat > 0:
> +          self.urlOpts.append("testname=%s" % path)
> +        else:
> +          self.urlOpts.append("singleTestPath=%s" % path)

We should probably just consolidate the repeat/singleTestPath bits. Is that too hard as a first pass?

::: testing/mochitest/server.js
@@ +374,4 @@
>   * Builds an optionally nested object containing links to the
>   * files and directories within dir.
>   */
> +function list(requestPath, directory, filename, recurse)

This could use a comment explaining what filename is for.

@@ +412,4 @@
>        key += "/";
>      }
>      if (recurse && file.isDirectory()) {
> +      [links[key], childCount] = list(key, file, filename, recurse);

Having a var "file" and also "filename" kind of sucks.

Comment 30

[:Gijs (he/him)]

Ted, this is fixed now, right? :-)

Comment 31

[(not currently active) Ted Mielczarek]

Yeah, we should just dupe it over, I guess.