Closed Bug 988169 Opened 7 years ago Closed 7 years ago

Running the mochitest harness in a directory no longer gives a list of test files

Categories

(Testing :: Mochitest, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: khuey, Assigned: froydnj)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

If I run mochitests for a single directory (say dom/indexedDB) the list of tests does not appear at the bottom of the harness page like it used to.  This means I can't click on a single test to run it or see the pass/fail counts for a given test.  Not sure when we regressed this, but I suspect it was recent.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> If I run mochitests for a single directory (say dom/indexedDB) the list of
> tests does not appear at the bottom of the harness page like it used to. 
> This means I can't click on a single test to run it or see the pass/fail
> counts for a given test.  Not sure when we regressed this, but I suspect it
> was recent.

I think this has regressed for a while; I don't recall seeing a list like this for the last while.  Certainly when I was hacking on mochitest bits, I didn't see anything like this.
No, this definitely has not been gone for years.
This doesn't seem like a super valuable feature. You can just pass the test name on the commandline.
So the issue is that if we're getting stuff from a manifest we just skip constructing the table.[0]  Can we fix that?  I think we might have to construct the table on the client side since it appears that we don't process the manifests at all on the server.

jmaher, can you take this?

[0] http://hg.mozilla.org/mozilla-central/diff/c116372d7ad4/testing/mochitest/server.js#l1.10
Flags: needinfo?(jmaher)
> You can just pass the test name on the commandline.

That doesn't work if you want to run several tests from the directory.  Unless you restart between them, which takes _forever_, and is a serious productivity drag, as I discovered today.  :(
Severity: normal → major
(In reply to Boris Zbarsky [:bz] from comment #7)
> > You can just pass the test name on the commandline.
> 
> That doesn't work if you want to run several tests from the directory. 
> Unless you restart between them, which takes _forever_, and is a serious
> productivity drag, as I discovered today.  :(

The mach mochitest commands accept multiple test file/path/name arguments.
> The mach mochitest commands accept multiple test file/path/name arguments.

Yes, but I want to run one test, keep running it until it passes, as I edit it, then switch to the next one.  I do that all the time...
I spend some time looking into this and running the tests without a manifest still do not yield a table of results.  Is this desirable?  if so we should figure out the real cause of it and then make sure it works with manifests :)
Flags: needinfo?(jmaher)
The real cause is the code Kyle pointed out in comment 6.  That code just needs to be able to parse the manifests into whatever format the current code wants, rather than ignoring the manifest.
I backed that out and hacked out the manifest stuff locally- then ran with a single directory and there was not a list of tests.
So this turns out to be the combination of a couple of things:

- The patch in bug 868158 turned off computation of the results table if we
  provided a manifest.  Sadly, it looks like a previous version of the patch
  that landed had code to compute all the bits we needed from manifests.  That
  code appears to have died following review comments.

- Bug 938019 forcefully enabled manifests to be turned on all the time, which
  means that the code from bug 868158 triggers always nowadays.

- The code in server.js to figure out what directory we're supposed to be
  finding test files from relied on certain URLs being hit to start the test
  suite.  mach's logic for passing this information along from the command line
  was busted.

The attached patch makes:

  mach mochitest-plain --keep-open --quiet layout/style/

display nice HTML tables for all the tests again.  Could interested parties
please confirm that it does the desired thing for their workflow?

I don't think this can break things in automation, but you never know...
Attachment #8426394 - Flags: review?(jmaher)
Attachment #8426394 - Flags: feedback?(khuey)
Attachment #8426394 - Flags: feedback?(bzbarsky)
Comment on attachment 8426394 [details] [diff] [review]
display the list of mochitests again

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

a --test-path can be a directory (which can contain subdirs), so I don't think what is written here will work.
Attachment #8426394 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #14)
> a --test-path can be a directory (which can contain subdirs), so I don't
> think what is written here will work.

The example command in comment 13 winds up passing a directory as --test-path (or, rather, the equivalent in the Python options object)--even a directory that doesn't have mochitest.ini in it--and everything works out just fine.  Could you elaborate on your objection?
Flags: needinfo?(jmaher)
My objection is that we use --test-dir for directories that have manifests (i.e. --ipcplugins in automation maps to --test-path=dom/plugins/test).  I know this is for mach, but if we are depending on a manifest to skip a test and now we are building the test list by iterating a directory, I see confusion.
Flags: needinfo?(jmaher)
After looking at what it would take to have the server send a static HTML file,
generated by the runtests.py script, I think it will be much easier to just
parse the manifest in the server itself.  I don't think this is *exactly* the
display format that was there before, but it's good enough to give a list of
tests and turn things green/red as they proceed.

Feedback welcome on the workflow.
Attachment #8426394 - Attachment is obsolete: true
Attachment #8426394 - Flags: feedback?(khuey)
Attachment #8426394 - Flags: feedback?(bzbarsky)
Attachment #8426519 - Flags: feedback?(khuey)
Attachment #8426519 - Flags: feedback?(jmaher)
Attachment #8426519 - Flags: feedback?(bzbarsky)
Comment on attachment 8426519 [details] [diff] [review]
display the list of mochitests again

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

looking over this, it seems logical. I don't see why we need to modify DeepTreeWalker bits here?
Attachment #8426519 - Flags: feedback?(jmaher) → feedback+
(In reply to Joel Maher (:jmaher) from comment #18)
> looking over this, it seems logical. I don't see why we need to modify
> DeepTreeWalker bits here?

Because my git commit --amend ways apparently amended a little too far back somehow.  Whoops!
I'm kind of hoping to strip server.js down and use a static index.html in the near future. Would your approach be any harder that way?
Comment on attachment 8426519 [details] [diff] [review]
display the list of mochitests again

This looks great!
Attachment #8426519 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8426519 [details] [diff] [review]
display the list of mochitests again

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

<3
Attachment #8426519 - Flags: feedback?(khuey) → feedback+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> I'm kind of hoping to strip server.js down and use a static index.html in
> the near future. Would your approach be any harder that way?

It shouldn't cause any problems to stripping things down, no.  If this code can be implemented in Python rather than in JS, so much the better.
Now with a fix so non-mochitest-plain mochitests don't blow up.
Attachment #8426519 - Attachment is obsolete: true
Attachment #8430121 - Flags: review?(jmaher)
Comment on attachment 8430121 [details] [diff] [review]
display the list of mochitests again

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

a few nits- overall this is a great patch!

::: testing/mochitest/server.js
@@ +587,5 @@
> +  var manifestStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +  manifestStream.init(manifestFile, -1, 0, 0);
> +
> +  var manifestObj = JSON.parse(NetUtil.readInputStreamToString(manifestStream,
> +                                                               manifestStream.available()));

if this throws an error in json parsing, what happens to the script?  do we get an error that makes sense?

@@ +595,5 @@
> +    var path = p.path;
> +    var components = path.split('/');
> +    components.unshift(root);
> +    var current = links;
> +        

nit: whitepace here, a blank link should be blank

@@ +601,5 @@
> +      var soFar = '/' + components.slice(0, index + 1).join('/')
> +        if (index == components.length - 1) {
> +          current[soFar] = true;
> +        } else {
> +          var key = soFar + '/';

why do we set current[soFar] = true, but then adjust it so current[sofar +'/'] = something else?
Attachment #8430121 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #25)
> ::: testing/mochitest/server.js
> @@ +587,5 @@
> > +  var manifestStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> > +  manifestStream.init(manifestFile, -1, 0, 0);
> > +
> > +  var manifestObj = JSON.parse(NetUtil.readInputStreamToString(manifestStream,
> > +                                                               manifestStream.available()));
> 
> if this throws an error in json parsing, what happens to the script?  do we
> get an error that makes sense?

JS errors got dumped to the browser when I was developing this (e.g. undefined variables), and I think that's appropriate behavior.  If we have errors parsing the manifest, it probably means the Python automation is busted.

> @@ +601,5 @@
> > +      var soFar = '/' + components.slice(0, index + 1).join('/')
> > +        if (index == components.length - 1) {
> > +          current[soFar] = true;
> > +        } else {
> > +          var key = soFar + '/';
> 
> why do we set current[soFar] = true, but then adjust it so current[sofar
> +'/'] = something else?

The |current[soFar] = true| is the basename case, the |soFar + '/'| case is when we still have a directory.  We wind up with a gnarly nested object:

  { '/path':
    { '/path/to':
      { '/path/to/test1.js': true,
        '/path/to/test2.js': true
      }
      ...
    }
    ...
  }

I think things would actually work correctly if we just dumped all the pathnames into an object like so:

  { '/path/to/test1.js': true,
    '/path/to/test2.js': true,
    ...
  }

I think mochitest-chrome does it this simpler way:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/harness.xul#49

You can see that the linksToTableRows tries to be clever in the nested object case:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#486

I was trying to match what the non-manifest case did, and am happy to change it to the simpler way if you think that would be better.
Flags: needinfo?(jmaher)
if it will work in a simpler way, we should do that.  My goal was more of confusion here (or confusion of another developer) and how to reduce that.  Quite possibly this could take a lot of your time up and we should just leave what you have in place maybe with a comment :)
Flags: needinfo?(jmaher)
I landed this with a straightforward fix to display the list without all the fancy indentation:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f68a689a3593
Flags: in-testsuite-
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/f68a689a3593
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.