1014062 - Expose manifest and suite/config information to tests themselves via SimpleTest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

[:Gijs (out for now; he/him)]

Attachment: expose parameters and test list via SimpleTest,

Ugh. This mess of setup/TestRunner/SimpleTest and some of it not existing in some cases does my head in (OK, it being late and my not having slept/eaten/drunk enough probably doesn't help either). Here's my current WIP. The sanityParams test passes if you run the entirety of the harness_sanity tests, but it doesn't pass if run on its own, nor does the browser chrome test version pass. I think the latter is just because the code apparently has its own way of reading the config and loading SimpleTest for browser/chrome tests (sigh), but I don't understand the former. Apparently there is some magical way of loading just a single test, but I don't know what happens to the TestRunner object/script in that case, and what, if anything, loads setup.js and how I can pass things along to SimpleTest from there. Ted, can you give some pointers as to where to look to finish this off?

Comment 2

[:Gijs (out for now; he/him)]

Attachment: expose parameters and test list via SimpleTest,

So this works, AFAICT... I'm reverting (the revert in) bug 990289 which means I'm also requesting review from bz... Boris, I'm essentially making the iframe in use on the multitest page really big, and hiding the extraneous stuff, and attempting to ensure that it doesn't unload the test file when the test finishes. Can you verify that like this, your workflow isn't broken? If this is not good enough, can you indicate an easy way for me to reproduce what you do that this patch prevents, so I can look at how to accommodate it? Thanks.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8427006 [details] [diff] [review]
expose parameters and test list via SimpleTest,

> Can you verify that like this, your workflow isn't broken?

It's broken.

> If this is not good enough, can you indicate an easy way for me to reproduce
> what you do that this patch prevents, so I can look at how to accommodate it? 

Try this:

1) Edit content/html/document/test/test_documentAll.html so that hasName is an empty array.

2) mach mochitest-plain --keep-open content/html/document/test/test_documentAll.html

Without this patch, I get a page that has a red box that has failures listed, the ability to toggle on the list of passing tests as well, etc.  In fact, I don't even have to --keep-open to get that; it seems to be the default (and desired, in my case) behavior without this patch.

With this patch I get nothing.

Comment 4

[:Gijs (out for now; he/him)]

Attachment: expose parameters and test list via SimpleTest,

OK. So I had to also hide the interstitial. And then I figured I should really make the output take the full browser window... which took a little more fiddling. In any case, I believe this works. Even with --no-autorun (you'll see the 'wrapper' page first, but once you run the test, you see just the test frame, if you're only running a single test).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8427210 [details] [diff] [review]
expose parameters and test list via SimpleTest,

This is better, yes.  Reloading asserts (not surprising), and the --keep-open thing is now required, which is annoying....

The one really major annoyance is that running a different test is a bit of a pain; before I could just edit the URL.  Typical use case is running the tests that failed on tbpl on a push, one by one.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8427210 [details] [diff] [review]
> expose parameters and test list via SimpleTest,
> 
> This is better, yes.  Reloading asserts (not surprising), and the
> --keep-open thing is now required, which is annoying....

Gijs: can you make your patch pass down keep-open when running a single test? That's the existing behavior, and it seems reasonable.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8427210 [details] [diff] [review]
expose parameters and test list via SimpleTest,

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

That was a ridiculously long review delay, I apologize. I'm marking this r- because I have a few reservations, but if you can address those I'll re-review quickly.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +53,5 @@
> +
> +    if (parentRunner) {
> +        var parameterInfo = parentRunner.getParameterInfo();
> +        if (parameterInfo) {
> +            SimpleTest.testParameters = parameterInfo.params;

Calling this "testParameters" seems misleading, since this is really the harness parameters, no? (In the future I'd like to expose the values from the test manifest, which would make this doubly-confusing.)

@@ +54,5 @@
> +    if (parentRunner) {
> +        var parameterInfo = parentRunner.getParameterInfo();
> +        if (parameterInfo) {
> +            SimpleTest.testParameters = parameterInfo.params;
> +            SimpleTest.testList = parameterInfo.testList;

I'm really not sure of the value of having this. What I want in the long run is just a JSON version of the test manifest entry for this specific test.

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +312,5 @@
> +    var singleTestRun = this._urls.length <= 1 && TestRunner.repeat <= 1;
> +    TestRunner.showTestReport = singleTestRun;
> +    var frame = $('testframe');
> +    frame.src="";
> +    document.body.setAttribute("singletest", singleTestRun);

Seems like it'd be saner to only set this attribute if singleTestRun is true.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Comment on attachment 8427210 [details] [diff] [review]
> expose parameters and test list via SimpleTest,
> 
> Review of attachment 8427210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That was a ridiculously long review delay, I apologize. I'm marking this r-
> because I have a few reservations, but if you can address those I'll
> re-review quickly.

I'll do my best, but it looks like I'm going to be swamped for the upcoming week... depends how much spare time I find this weekend / Monday.

> ::: testing/mochitest/tests/SimpleTest/SimpleTest.js
> @@ +53,5 @@
> > +
> > +    if (parentRunner) {
> > +        var parameterInfo = parentRunner.getParameterInfo();
> > +        if (parameterInfo) {
> > +            SimpleTest.testParameters = parameterInfo.params;
> 
> Calling this "testParameters" seems misleading, since this is really the
> harness parameters, no? (In the future I'd like to expose the values from
> the test manifest, which would make this doubly-confusing.)

Good point. I can rename to "harnessParameters".

> 
> @@ +54,5 @@
> > +    if (parentRunner) {
> > +        var parameterInfo = parentRunner.getParameterInfo();
> > +        if (parameterInfo) {
> > +            SimpleTest.testParameters = parameterInfo.params;
> > +            SimpleTest.testList = parameterInfo.testList;
> 
> I'm really not sure of the value of having this. What I want in the long run
> is just a JSON version of the test manifest entry for this specific test.

By this you mean just the testList, right?

> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +312,5 @@
> > +    var singleTestRun = this._urls.length <= 1 && TestRunner.repeat <= 1;
> > +    TestRunner.showTestReport = singleTestRun;
> > +    var frame = $('testframe');
> > +    frame.src="";
> > +    document.body.setAttribute("singletest", singleTestRun);
> 
> Seems like it'd be saner to only set this attribute if singleTestRun is true.

OK.


(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> (In reply to Boris Zbarsky [:bz] from comment #5)
> > Comment on attachment 8427210 [details] [diff] [review]
> > expose parameters and test list via SimpleTest,
> > 
> > This is better, yes.  Reloading asserts (not surprising), and the
> > --keep-open thing is now required, which is annoying....
> 
> Gijs: can you make your patch pass down keep-open when running a single
> test? That's the existing behavior, and it seems reasonable.

It's been a while now, but I don't think I intended to change anything as to how 'keep-open' works... I'll have a look.

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to :Gijs Kruitbosch from comment #8)
> > I'm really not sure of the value of having this. What I want in the long run
> > is just a JSON version of the test manifest entry for this specific test.
> 
> By this you mean just the testList, right?

Correct.

> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> > (In reply to Boris Zbarsky [:bz] from comment #5)
> > > Comment on attachment 8427210 [details] [diff] [review]
> > > expose parameters and test list via SimpleTest,
> > > 
> > > This is better, yes.  Reloading asserts (not surprising), and the
> > > --keep-open thing is now required, which is annoying....
> > 
> > Gijs: can you make your patch pass down keep-open when running a single
> > test? That's the existing behavior, and it seems reasonable.
> 
> It's been a while now, but I don't think I intended to change anything as to
> how 'keep-open' works... I'll have a look.

Right, I think this is just natural fallout from your change. When a single test is loaded in the top-level tab it ignores the "closeWhenDone" logic. Now that you're loading the harness again it will honor it. To preserve the existing behavior you can special-case that so that if there's only one test being run you explicitly use the keep-open behavior.

Comment 10

[(not currently active) Ted Mielczarek]

This will fix bug 508664.

Comment 11

[:Gijs (out for now; he/him)]

Attachment: expose harness parameters via SimpleTest, always use the harness, pass keep-open for single test runs with mach,

I believe I've addressed all the comments (plus unbitrotted the various parts correctly...).

Comment 12

[:Gijs (out for now; he/him)]

Try run:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=2e2c3ec52c30

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8462664 [details] [diff] [review]
expose harness parameters via SimpleTest, always use the harness, pass keep-open for single test runs with mach,

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

Thanks!

::: testing/mochitest/mach_commands.py
@@ +338,5 @@
>  
>              manifest = TestManifest()
>              manifest.tests.extend(tests)
>  
> +            if (len(tests) == 1):

nit: don't need parens here

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 8462664 [details] [diff] [review]
expose harness parameters via SimpleTest, always use the harness, pass keep-open for single test runs with mach,

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

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +522,5 @@
>  
> +    var singleTestRun = this._urls.length <= 1 && TestRunner.repeat <= 1;
> +    TestRunner.showTestReport = singleTestRun;
> +    var frame = $('testframe');
> +    frame.src="";

Nit: add a space around '='.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch (Bugmail catchup, needinfo if urgent) from comment #12)
> Try run:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=2e2c3ec52c30

This was orange on debug-only, unfortunately, with no real information about what's wrong :|

Comment 16

[:Gijs (out for now; he/him)]

Attachment: expose harness parameters via SimpleTest, always use the harness, pass keep-open for single test runs with mach,

Re-ran try per discussion on IRC with jgraham:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=78d5dd9a98f4

This version fixes the two earlier nits.

Comment 17

[:Gijs (out for now; he/him)]

Try run now that bug 1048288 is fixed to see what assertions I'm triggering... :-\

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=39001fc02fd7
remote:   https://tbpl.mozilla.org/?tree=Try&rev=39001fc02fd7

Comment 18

[:Gijs (out for now; he/him)]

Well, uhh, I'll take it, I guess:

4032 INFO TEST-UNEXPECTED-OK | chrome://mochitests/content/chrome/content/xul/templates/tests/chrome/test_tmpl_storage_dynamicparameters.xul | Assertion count 0 is less than expected range 1-2 assertions.
17014 INFO TEST-UNEXPECTED-OK | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug495648.xul | Assertion count 15 is less than expected range 18-24 assertions.

(it's the same tests across the board, where sometimes the second one passes - presumably because the number of assertions goes up again, and so it is within the 18-24 range).

The tree is closed now, but unless anyone objects before it's open, I plan on landing this and removing the assertions clause for the first test, and reducing the lower bound for the second to 15.

Comment 19

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/85c7a24dc648

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85c7a24dc648

Comment 21

[Paul Rouget [:paul]]

I can't run tests anymore, and I think it's related to this bug:

> ./mach build testing/mochitest && ./mach mochitest-chrome docshell/test/chrome/test_bug112564.xul

 0:00.14 /usr/bin/make -C /home/paul/mozilla/src/obj.firefox -j4 -s backend.RecursiveMakeBackend
 0:00.24 Build configuration changed. Regenerating backend.
 0:00.41 Reticulating splines...
 0:06.10 Finished reading 2583 moz.build files in 1.71s
 0:06.10 Processed into 7187 build config descriptors in 1.37s
 0:06.10 Backend executed in 2.43s
 0:06.10 2328 total backend files; 0 created; 2 updated; 2326 unchanged; 0 deleted; 156 -> 900 Makefile
 0:06.10 Total wall time: 5.69s; CPU time: 5.35s; Efficiency: 94%; Untracked: 0.19s
 0:06.37 /usr/bin/make -C testing/mochitest -j4 -s
 0:06.48 /home/paul/mozilla/src/python/mozbuild/mozbuild/jar.py:56: UserWarning: Duplicate name: 'content/tests/SimpleTest/TestRunner.js'
 0:06.48   self._zipfile.writestr(self._name, self._inner.getvalue())
 0:06.60 ccache (direct) hit rate: 0.0%; (preprocessed) hit rate: 0.0%; miss rate: 0.0%
Your build was successful!
From _tests: Kept 18827 existing; Added/updated 0; Removed 0 files and 0 directories.
pk12util: PKCS12 IMPORT SUCCESSFUL
 0:03.02 MochitestServer : launching [u'/home/paul/mozilla/src/obj.firefox/dist/bin/xpcshell', '-g', u'/home/paul/mozilla/src/obj.firefox/dist/bin', '-v', '170', '-f', u'/home/paul/mozilla/src/obj.firefox/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpq6ldVC.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = true;", '-f', '/home/paul/mozilla/src/obj.firefox/_tests/testing/mochitest/server.js']
 0:03.02 runtests.py | Server pid: 17177
 0:03.03 runtests.py | Websocket server pid: 17179
 0:03.05 runtests.py | SSL tunnel pid: 17183
0 INFO SUITE-START | Running 1 tests
 0:04.06 runtests.py | Running tests: start.

 0:04.08 runtests.py | Application pid: 17201
1 INFO (process:17201): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
2 INFO -*- Webapps.jsm : Saving /tmp/tmpq6ldVC.mozrunner/webapps/webapps.json
3 INFO -*- Webapps.jsm : Saving /tmp/tmpq6ldVC.mozrunner/webapps/webapps.json
4 INFO SimpleTest START
5 INFO TEST-UNEXPECTED-FAIL: setup.js | error parsing http://mochi.test:8888/tests.json (TypeError: document.body is undefined)
6 INFO JavaScript error: chrome://mochikit/content/tests/SimpleTest/TestRunner.js, line 522: document.body is undefined

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e49a60f84f1

Comment 23

[Boris Zbarsky [:bzbarsky]]

Hmm.  So this change:

>-            if len(test_paths) == 1 and len(tests) == 1:
>-                options.testPath = test_paths[0]

Means the test name is no longer in the url bar; see comment 5.  That's turning out moderately annoying in practice.  :(

Can we do something with pushstate to make that happier?  Or would that just make things break if the user then hits enter in the url bar?

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> Hmm.  So this change:
> 
> >-            if len(test_paths) == 1 and len(tests) == 1:
> >-                options.testPath = test_paths[0]
> 
> Means the test name is no longer in the url bar; see comment 5.  That's
> turning out moderately annoying in practice.  :(
> 
> Can we do something with pushstate to make that happier?  Or would that just
> make things break if the user then hits enter in the url bar?

I suspect that's not going to make things much better (also because of bug 1060716 which you filed).

We could probably add a form to have a "url bar" for the iframe in which the test is loaded, and have that just remove and re-set src= to the input's contents on the iframe (plus whatever arcane test magic needs to happen that we added in this bug)? Would that work for your purposes (incl. whatever history black magic we're talking about in bug 1060716) ?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> and have that just remove and re-set src= to the input's contents on the iframe

We could try that, but I'm not sure it would trigger the "revalidate or bypass cache" behavior that's needed here, sadly.  :(  I seem to recall us doing something special for enter in url bar in terms of revalidation.... but maybe we're luck and I'm wrong.