Closed Bug 508664 Opened 11 years ago Closed 6 years ago

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


(Testing :: Mochitest, defect)

Windows Mobile 6 Professional
Not set


(Not tracked)



(Reporter: jmaher, Assigned: Gijs)


(Depends on 1 open bug, Blocks 1 open bug)



(3 files, 5 obsolete files)

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 script which will find and run the tests using the hacked version of SimpleTest.js.
use this with the simpletest.js patch to run and collect results of mochitest 1 file/time.
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:

Commenting this _logResult function out allows the majority of the dom tests to run one at a time.
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?
Blocks: 512246
No longer blocks: 491903
Updated patch to comment out dom specific logging.
Assignee: nobody → jmaher
Attachment #392790 - Attachment is obsolete: true
Attachment #396496 - Flags: review?(ctalbert)
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-.
Attachment #396496 - Flags: review?(ctalbert) → review-
updated for bitrot.
Attachment #396496 - Attachment is obsolete: true
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:

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.
Attachment #404512 - Attachment is obsolete: true
Attachment #405142 - Attachment is obsolete: true
Attachment #406077 - Flags: review?(ctalbert)
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.
Attachment #406077 - Flags: review?(ctalbert) → review?(ted.mielczarek)
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 {
>+        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.
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:
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?
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.
Ok, so you could still make the fix I suggested, and just run ... http://<server>:8888/tests/?testPath=mytestfile.html&logFile=testfile.log
Duplicate of this bug: 510812
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.
Duplicate of this bug: 456876
Version: unspecified → Trunk
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.
Attachment #406077 - Attachment is obsolete: true
Attachment #409998 - Flags: review?(ted.mielczarek)
Attachment #406077 - Flags: review?(ted.mielczarek)
Comment on attachment 409998 [details] [diff] [review]
Run single test in mochitest framework

>diff --git a/testing/mochitest/ b/testing/mochitest/
>--- a/testing/mochitest/
>+++ b/testing/mochitest/
>@@ -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:

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 = ""+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.
Attachment #409998 - Flags: review?(ted.mielczarek) → review-
Duplicate of this bug: 399732
this is not required for tests to run on winmo or remotely, but is a nice to have.
No longer blocks: 512246
I bet it'd be really useful for stuff like bug 551256, where we want to run a unit test repeatedly until it fails.
Depends on: 622390
Duplicate of this bug: 661981
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...
Attachment #634494 - Flags: feedback?(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.
Waiting-a-month review ping.
Comment on attachment 634494 [details] [diff] [review]
cleaned up, rebased patch

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

::: testing/mochitest/
@@ +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.
Attachment #634494 - Flags: feedback?(ted.mielczarek) → feedback+
Duplicate of this bug: 872532
Duplicate of this bug: 866189
Summary: add ability for mochitest to run standalone test with logging → lots of mochitest harness features don't work when running a single test file
Blocks: 906623
Duplicate of this bug: 979467
Ted, this is fixed now, right? :-)
Flags: needinfo?(ted)
Yeah, we should just dupe it over, I guess.
Assignee: jmaher → gijskruitbosch+bugs
Flags: needinfo?(ted)
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1014062
You need to log in before you can comment on or make changes to this bug.