Reduce opportunities for undetected failures in JavaScript tests.

* Redesign asyncRequirements/assertAsynchronousRequirement:
  * Instead of mucking with the page title etc, asyncRequirements is
    defined in terms of registering additional 'tests' that jsunit.js
    knows about; this prevents incorrect passing-and-then-failing, and
    means that assertAsynchronousRequirement is as fire-and-forget as
    its name suggests.
  * Instead of just a nominally-a-predicate, there is a second function
    parameter for additional asserts to be executed once the predicate
    is true.

* Fixed lack of verification of headless JS tests:
  * RhinoTestBed now actually tests that the jsunit tests it runs
    passed, rather than just that they did not throw an exception that
    escaped. (Future work: get rid of the _junit_.fail kludge, which has
    been left in for now for better error messages.)
    * Corrected implementation of document.title in third party env.js.
  * Added missing pass()es in tests that are now detected.
  * Added 'all tests passed' marker string to html-emitter-test.html
    which does not use jsunit.js.

* Added new browser test driver meta-test.js, which loads known-failing
  test suites into iframes to verify they fail in the expected fashion.

* Refinements and refactoring in jsunit.js and browser-test-case.js:
  * A test which passes and then fails, or fails or passes and then
    passes, is detected and considered failure.
    * Fixed undetected failures in es53-test-client-uri-rewriting.js
      (miswritten assert, not functionality error) and refactored.
    * Fixed duplicate pass in testProtoMention.
  * Added 'panic' facility, which guarantees the test page will fail,
    where throwing an exception that may be uncaught does not, and used
    it for all already caught cases of misuse of the test framework.
  * Test cases now individually remember their container elements,
    eliminating the need to pass around idClass info. The pass() wrapper
    around jsunitPass() is no longer necessary (but has been left for
    now).
    * Adjusted domado-dom-guest testReadOnly to be compatible (it had
      been replacing its testcontainer element with a clone).
  * jsunitRegisterIf is now defined inside jsunit.js.
  * jsunit.fail() added symmetric with jsunit.pass(), and both now
    update testcontainer elements.

* Incidental changes:
  * Renamed misnamed test method in CssStylesheetTest.java.
  * Renamed misnamed title of css-stylesheet-test.html.

@r5381

[felix8a, 2013-04-23 00:00:56]

partial review; I haven't finished looking at the assertasync stuff yet

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/js...
File tests/com/google/caja/plugin/jsunit.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/js...
tests/com/google/caja/plugin/jsunit.js:356: var node =
testRecord.resultContainer;
I'm a little worried that a test might accidentally or intentionally replace its
result container, so this might now be an orphan. But that seems really
unlikely, and I can't think of a case where that would actually matter. so ok.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
File tests/com/google/caja/plugin/meta-test-async-fails.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test-async-fails.js:15:
jsunitRegister('testAsyncFails', function() {
reading these files in isolation is slightly confusing. maybe add a brief
comment about what the metatest expects?

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
File tests/com/google/caja/plugin/meta-test.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:54: var w = frame.contentWindow;
is this reliable if the frame hasn't loaded yet? I can't remember.

you're only using the return value as an arg to pollTitle, so maybe return the
frame element and pass that to pollTitle instead?

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:90: if (pass === total) {
how about (total <= pass) instead?

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:109: assertEquals('all tests failed',
m[3], m[1]);
I'd prefer testing against a specific number here, to reduce the chance of false
positives.

maybe interpretTitle should return values like ['fail', 1, 2, 3] and ['pass', 0,
3, 3]?

every pollTitle callback immediately calls interpretTitle; maybe pollTitle
should do the parsing and pass the parsed values to the callback?

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:153: assertEquals('expected panics',
'2', m[1]);
this test is going to show "2/2 fail 1/2 pass"? that doesn't seem right

[kpreid2, 2013-04-23 18:43:17]

* Redesign asyncRequirements/assertAsynchronousRequirement:
  * Instead of mucking with the page title etc, asyncRequirements is
    defined in terms of registering additional 'tests' that jsunit.js
    knows about; this prevents incorrect passing-and-then-failing, and
    means that assertAsynchronousRequirement is as fire-and-forget as
    its name suggests.
  * Instead of just a nominally-a-predicate, there is a second function
    parameter for additional asserts to be executed once the predicate
    is true.

* Fixed lack of verification of headless JS tests:
  * RhinoTestBed now actually tests that the jsunit tests it runs
    passed, rather than just that they did not throw an exception that
    escaped. (Future work: get rid of the _junit_.fail kludge, which has
    been left in for now for better error messages.)
    * Corrected implementation of document.title in third party env.js.
  * Added missing pass()es in tests that are now detected.
  * Added 'all tests passed' marker string to html-emitter-test.html
    which does not use jsunit.js.

* Added new browser test driver meta-test.js, which loads known-failing
  test suites into iframes to verify they fail in the expected fashion.

* Refinements and refactoring in jsunit.js and browser-test-case.js:
  * A test which passes and then fails, or fails or passes and then
    passes, is detected and considered failure.
    * Fixed undetected failures in es53-test-client-uri-rewriting.js
      (miswritten assert, not functionality error) and refactored.
    * Fixed duplicate pass in testProtoMention.
  * Added 'panic' facility, which guarantees the test page will fail,
    where throwing an exception that may be uncaught does not, and used
    it for all already caught cases of misuse of the test framework.
  * Test cases now individually remember their container elements,
    eliminating the need to pass around idClass info. The pass() wrapper
    around jsunitPass() is no longer necessary (but has been left for
    now).
    * Adjusted domado-dom-guest testReadOnly to be compatible (it had
      been replacing its testcontainer element with a clone).
  * jsunitRegisterIf is now defined inside jsunit.js.
  * jsunit.fail() added symmetric with jsunit.pass(), and both now
    update testcontainer elements.

* Incidental changes:
  * Renamed misnamed test method in CssStylesheetTest.java.
  * Renamed misnamed title of css-stylesheet-test.html.

[kpreid2, 2013-04-23 18:44:44]

New snapshot.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/js...
File tests/com/google/caja/plugin/jsunit.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/js...
tests/com/google/caja/plugin/jsunit.js:356: var node =
testRecord.resultContainer;
On 2013/04/23 00:00:56, felix8a wrote:
> I'm a little worried that a test might accidentally or intentionally replace
> its result container, so this might now be an orphan. But that seems really
> unlikely, and I can't think of a case where that would actually matter. so ok.

This isn't just not unlikely: it actually happened. testReadOnly in domado tests
used a directAccess gimmick to replace its own testcontainer with a copy of
itself marked foreign, which (since this reference was pointing to the old node)
caused the new container to never get marked passed).

I have now added a check that the container is still in the document; if it is
not, it will be recreated and the test will be marked failed.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
File tests/com/google/caja/plugin/meta-test-async-fails.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test-async-fails.js:15:
jsunitRegister('testAsyncFails', function() {
On 2013/04/23 00:00:56, felix8a wrote:
> reading these files in isolation is slightly confusing. maybe add a brief
> comment about what the metatest expects?

Done.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
File tests/com/google/caja/plugin/meta-test.js (right):

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:54: var w = frame.contentWindow;
On 2013/04/23 00:00:56, felix8a wrote:
> is this reliable if the frame hasn't loaded yet? I can't remember.

Yes. .contentWindow is a proxy (!) for “the currently loaded page's Window”. I
learned this in detail while working on iframe loading. It is sufficiently
standard behavior that jQuery's test suite depends on it.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:90: if (pass === total) {
On 2013/04/23 00:00:56, felix8a wrote:
> how about (total <= pass) instead?

Added a specific assertion that pass + fail <= total under all circumstances,
and refactored the whole thing.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:109: assertEquals('all tests failed',
m[3], m[1]);
On 2013/04/23 00:00:56, felix8a wrote:
> I'd prefer testing against a specific number here, to reduce the chance of
> false positives.

Done.

> maybe interpretTitle should return values like ['fail', 1, 2, 3] and ['pass',
> 0, 3, 3]?
> 
> every pollTitle callback immediately calls interpretTitle; maybe pollTitle
> should do the parsing and pass the parsed values to the callback?

Done.

https://codereview.appspot.com/8907046/diff/1/tests/com/google/caja/plugin/me...
tests/com/google/caja/plugin/meta-test.js:153: assertEquals('expected panics',
'2', m[1]);
On 2013/04/23 00:00:56, felix8a wrote:
> this test is going to show "2/2 fail 1/2 pass"? that doesn't seem right

No, 2/3 fail 1/3 pass.

[felix8a, 2013-04-27 01:07:41]

cool. lgtm

[ihab.awad, 2013-04-27 01:17:15]

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:325:
jsunitRegisterAuxiliaryStatus(id);
Confused about what this does, exactly.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:387: record.message);
Need to capture 'record' due to hoisting, like you did above?

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-events-guest.html (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-events-guest.html:247: });
Whacky indentation in this and other places?

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-external-script-guest.html (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-external-script-guest.html:77:
assertContains("Render result", "123", testContainer.innerText);
Indentation?

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/jsunit.js (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:33: jsunit.beSynchronous = typeof _junit_
!== undefined;  // TODO(kpreid): kludge
Whatever sets _junit_ and whyever does it use this horrid mechanism thus
necessitating this most rebarbative of kludges? ;)

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:279: jsunitRunNext(testNames,
opt_asyncEval);
javascript kitteh can not haz recursive tail

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:348: throw new Error('bad jsunitFinished
status: ' + status);
jsunit.panic?

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:407: return node === document || (node
!== null && inDocument(node.parentNode));
Nice.

https://codereview.appspot.com/8907046/diff/7001/third_party/js/envjs/env.js
File third_party/js/envjs/env.js (right):

https://codereview.appspot.com/8907046/diff/7001/third_party/js/envjs/env.js#...
third_party/js/envjs/env.js:216: },
There we go again with that editing of 3pty.

[kpreid2, 2013-04-27 01:54:05]

* Redesign asyncRequirements/assertAsynchronousRequirement:
  * Instead of mucking with the page title etc, asyncRequirements is
    defined in terms of registering additional 'tests' that jsunit.js
    knows about; this prevents incorrect passing-and-then-failing, and
    means that assertAsynchronousRequirement is as fire-and-forget as
    its name suggests.
  * Instead of just a nominally-a-predicate, there is a second function
    parameter for additional asserts to be executed once the predicate
    is true.

* Fixed lack of verification of headless JS tests:
  * RhinoTestBed now actually tests that the jsunit tests it runs
    passed, rather than just that they did not throw an exception that
    escaped. (Future work: get rid of the _junit_.fail kludge, which has
    been left in for now for better error messages.)
    * Corrected implementation of document.title in third party env.js.
  * Added missing pass()es in tests that are now detected.
  * Added 'all tests passed' marker string to html-emitter-test.html
    which does not use jsunit.js.

* Added new browser test driver meta-test.js, which loads known-failing
  test suites into iframes to verify they fail in the expected fashion.

* Refinements and refactoring in jsunit.js and browser-test-case.js:
  * A test which passes and then fails, or fails or passes and then
    passes, is detected and considered failure.
    * Fixed undetected failures in es53-test-client-uri-rewriting.js
      (miswritten assert, not functionality error) and refactored.
    * Fixed duplicate pass in testProtoMention.
  * Added 'panic' facility, which guarantees the test page will fail,
    where throwing an exception that may be uncaught does not, and used
    it for all already caught cases of misuse of the test framework.
  * Test cases now individually remember their container elements,
    eliminating the need to pass around idClass info. The pass() wrapper
    around jsunitPass() is no longer necessary (but has been left for
    now).
    * Adjusted domado-dom-guest testReadOnly to be compatible (it had
      been replacing its testcontainer element with a clone).
  * jsunitRegisterIf is now defined inside jsunit.js.
  * jsunit.fail() added symmetric with jsunit.pass(), and both now
    update testcontainer elements.

* Incidental changes:
  * Renamed misnamed test method in CssStylesheetTest.java.
  * Renamed misnamed title of css-stylesheet-test.html.

[kpreid2, 2013-04-27 01:54:38]

New snapshot.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:325:
jsunitRegisterAuxiliaryStatus(id);
On 2013/04/27 01:17:16, ihab.awad wrote:
> Confused about what this does, exactly.

It creates a new, independent "test", which can then pass or fail. This is a way
of allowing the assertAsynchronousRequirement to be fully fire-and-forget: you
don't have to wait in the test for it to complete before pass()ing.

The remark above about " TODO(kpreid): Integrate this fully into jsunit.js." is
about making this less quirky.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:387: record.message);
On 2013/04/27 01:17:16, ihab.awad wrote:
> Need to capture 'record' due to hoisting, like you did above?

Done. Also revised indentation on both.

(I thought about writing a meta test testing for that mistake, but it would be
pulling out individual error messages via an onerror hook, and that seemed like
a little more effort than it's worth.)

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-events-guest.html (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-events-guest.html:247: });
On 2013/04/27 01:17:16, ihab.awad wrote:
> Whacky indentation in this and other places?

I argue this is perfectly conventional indentation for a
function-wrapped-in-stuff: Function body gets 2 space indent. Continuations of
the stuff introducing said function get 4 space indent. It just happens to be an
especially long head and short tail.

I feel this is especially appropriate as the function body is the 'continuation'
of control here.

...but if you don't, I'll change it.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-external-script-guest.html (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-external-script-guest.html:77:
assertContains("Render result", "123", testContainer.innerText);
On 2013/04/27 01:17:16, ihab.awad wrote:
> Indentation?

Same thing as before.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/jsunit.js (right):

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:33: jsunit.beSynchronous = typeof _junit_
!== undefined;  // TODO(kpreid): kludge
On 2013/04/27 01:17:16, ihab.awad wrote:
> Whatever sets _junit_

RhinoTestBed.java

> and whyever does it use this horrid mechanism

Because it was formerly horribly broken. It *ran* the JS of the JS tests, but
didn't actually check that the tests passed, just that no exception was thrown
at the Java level. _junit_ was primarily used to throw an error which would
*not* be caught by any JS try/catch, thus causing the driving Java test case to
fail.

I have now fixed it so that it does in fact verify that all tests passed...

> thus necessitating this most rebarbative of kludges? ;)

...however, it is not in a good position to, or at least I didn't want to write
the threaded code to, *wait* for the JS tests to pass. (In fact, that's another
way in which it was broken: ever since Felix introduced an async loop to jsunit,
RhinoTestBed could never have noticed a failure in anything but the very first
test.) Therefore, this flag causes jsunit to complete the entire run inside the
initial turn.

(I also suspect that the support for setTimeout that the RhinoTestBed provides
is thread-unsafe, but I haven't actually looked at that.)

I've elaborated the comment a bit.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:279: jsunitRunNext(testNames,
opt_asyncEval);
On 2013/04/27 01:17:16, ihab.awad wrote:
> javascript kitteh can not haz recursive tail

Yes, but this tail is short.

Would you care to propose a nice refactoring to do this Right™?

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:348: throw new Error('bad jsunitFinished
status: ' + status);
On 2013/04/27 01:17:16, ihab.awad wrote:
> jsunit.panic?

I didn't because this is more in the nature of argument validation.

Also, no need: If jsunitFinished is mis-called within a test, the error will
cause a failure. The worst that could happen is a test which passes-then-fails
(which can only happen if the test is miswritten in another way) will fail to
fail; furthermore, we don't expect jsunitFinished by itself to be called from
outside jsunit.js.

https://codereview.appspot.com/8907046/diff/7001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/jsunit.js:407: return node === document || (node
!== null && inDocument(node.parentNode));
On 2013/04/27 01:17:16, ihab.awad wrote:
> Nice.

Sometimes you can actually use that CS 101 stuff.

https://codereview.appspot.com/8907046/diff/7001/third_party/js/envjs/env.js
File third_party/js/envjs/env.js (right):

https://codereview.appspot.com/8907046/diff/7001/third_party/js/envjs/env.js#...
third_party/js/envjs/env.js:216: },
On 2013/04/27 01:17:16, ihab.awad wrote:
> There we go again with that editing of 3pty.

Indeed. Thus the comment (which I've revised to use the same wording as other
comments).

If this patch were to be lost, things would visibly break (in particular, all
the Rhino tests would fail to observe an 'all tests passed' title).

This env.js originally was one we were borrowing from the jQuery test suite,
which stopped being there and so I pulled it out into a separate directory.
Also, env.js has had a mildly complex history of maintainership since this one
was written, it seems. It would be worth looking into getting a newer one with
fewer bugs to patch.

[kpreid2, 2013-04-29 17:17:34]

requested ping

[ihab.awad, 2013-04-29 17:42:03]

lgtm

[kpreid2, 2013-04-29 19:06:31]

New snapshot :(

meta-test-fails is now HTML instead of bare JS. This is necessary for the tests
running under ES5/3 due to issues with the order of execution of bare JS,
creation of DOM, onload, and .run() callbacks. (Filing a separate issue for
that.)

No other changes.

[felix8a, 2013-04-29 19:30:09]

I don't see the new snapshot? I only see patch set 3, created 2 days ago

[kpreid2, 2013-04-29 19:30:42]

* Redesign asyncRequirements/assertAsynchronousRequirement:
  * Instead of mucking with the page title etc, asyncRequirements is
    defined in terms of registering additional 'tests' that jsunit.js
    knows about; this prevents incorrect passing-and-then-failing, and
    means that assertAsynchronousRequirement is as fire-and-forget as
    its name suggests.
  * Instead of just a nominally-a-predicate, there is a second function
    parameter for additional asserts to be executed once the predicate
    is true.

* Fixed lack of verification of headless JS tests:
  * RhinoTestBed now actually tests that the jsunit tests it runs
    passed, rather than just that they did not throw an exception that
    escaped. (Future work: get rid of the _junit_.fail kludge, which has
    been left in for now for better error messages.)
    * Corrected implementation of document.title in third party env.js.
  * Added missing pass()es in tests that are now detected.
  * Added 'all tests passed' marker string to html-emitter-test.html
    which does not use jsunit.js.

* Added new browser test driver meta-test.js, which loads known-failing
  test suites into iframes to verify they fail in the expected fashion.

* Refinements and refactoring in jsunit.js and browser-test-case.js:
  * A test which passes and then fails, or fails or passes and then
    passes, is detected and considered failure.
    * Fixed undetected failures in es53-test-client-uri-rewriting.js
      (miswritten assert, not functionality error) and refactored.
    * Fixed duplicate pass in testProtoMention.
  * Added 'panic' facility, which guarantees the test page will fail,
    where throwing an exception that may be uncaught does not, and used
    it for all already caught cases of misuse of the test framework.
  * Test cases now individually remember their container elements,
    eliminating the need to pass around idClass info. The pass() wrapper
    around jsunitPass() is no longer necessary (but has been left for
    now).
    * Adjusted domado-dom-guest testReadOnly to be compatible (it had
      been replacing its testcontainer element with a clone).
  * jsunitRegisterIf is now defined inside jsunit.js.
  * jsunit.fail() added symmetric with jsunit.pass(), and both now
    update testcontainer elements.

* Incidental changes:
  * Renamed misnamed test method in CssStylesheetTest.java.
  * Renamed misnamed title of css-stylesheet-test.html.

[kpreid2, 2013-04-29 19:30:48]

On 2013/04/29 19:30:09, felix8a wrote:
> I don't see the new snapshot? I only see patch set 3, created 2 days ago

Oops, done.

[felix8a, 2013-04-29 19:33:46]

lgtm