Closed
Bug 988169
Opened 11 years ago
Closed 11 years ago
Running the mochitest harness in a directory no longer gives a list of test files
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: khuey, Assigned: froydnj)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
2.77 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•11 years ago
|
||
(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.
Comment 2•11 years ago
|
||
Wasn't that bug 479352?
Reporter | ||
Comment 3•11 years ago
|
||
No, this definitely has not been gone for years.
Comment 5•11 years ago
|
||
This doesn't seem like a super valuable feature. You can just pass the test name on the commandline.
Reporter | ||
Comment 6•11 years ago
|
||
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)
![]() |
||
Comment 7•11 years ago
|
||
> 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
Comment 8•11 years ago
|
||
(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.
![]() |
||
Comment 9•11 years ago
|
||
> 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...
Comment 10•11 years ago
|
||
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)
![]() |
Assignee | |
Comment 11•11 years ago
|
||
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.
Comment 12•11 years ago
|
||
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.
![]() |
Assignee | |
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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-
![]() |
Assignee | |
Comment 15•11 years ago
|
||
(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)
Comment 16•11 years ago
|
||
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)
![]() |
Assignee | |
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 19•11 years ago
|
||
(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!
Comment 20•11 years ago
|
||
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 21•11 years ago
|
||
Comment on attachment 8426519 [details] [diff] [review]
display the list of mochitests again
This looks great!
Attachment #8426519 -
Flags: feedback?(bzbarsky) → feedback+
Reporter | ||
Comment 22•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 23•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 26•11 years ago
|
||
(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)
Comment 27•11 years ago
|
||
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)
![]() |
Assignee | |
Comment 28•11 years ago
|
||
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 | |
Updated•11 years ago
|
Assignee: nobody → nfroyd
Comment 29•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•