Diagnose problems with jQuery tests and file bugs

Diagnose problems with jQuery tests and file bugs

[ihab.awad, 2013-07-16 19:31:56]

Now ready for review.

[kpreid_google, 2013-07-16 19:49:18]

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/generic-host-page.html (right):

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/generic-host-page.html:119: .api({
The point of this page is to provide an _unmodified_ Caja environment. I can see
the use of this, but you should make it opt-in — just add another checkbox to
the form.

(We also ought to have a single standard implementation of "just pass on
alert()", but ...)

(Now, the _right_ solution for debuggability is to provide 'console' by default,
but that's more work.)

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/third-party-tests.json (right):

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:6: "guest-template":
"/third_party/js/jqueryjs/test/index.html?dev=true&module=*",
What does dev=true do?

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:13:
"https://code.google.com/p/google-caja/issues/detail?id=1199",
Links are good. Just links, not so good. This doesn't help the reader figure out
whether the failure they're seeing is known.

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:57: "comment": [
delete this

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:87: "TODO(ihab.awad): Amidst
this file with diagnosis and bug filing"
That's a bit cryptic in case someone else is picking up the work.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
File third_party/js/jqueryjs/test/unit/attributes.js (right):

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:82: // TODO(ihab.awad): 'target'
attributes are rewritten in Caja.
My practice has been to label modifications "Patched for Caja" so we can
uniformly grep and forward-port them. If you want to use other labels, please
add documentation of them to jqueryjs/README-Caja.txt.

(Actually, that file doesn't even mention "Patched for Caja". I see I confused
it with third_party/js/proxies/README-Caja which has a similar role. Please add
suitable text to the one for jQuery.)

And unless you're planning to fix the visibility of attribute rewriting, I don't
see that this is a TODO. 

tab indent lines in this file for consistency

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:157: // TODO(ihab.awad):
'onclick' JavaScript attributes are rewritten in Caja.
tab indent

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:350: // TODO(ihab.awad): Setting
'autofocus' property throws, so we need to add the
That was fixed in r5483.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:375: // TODO(ihab.awad):
document.createAttrbute() is nonexistent, so we need to
typo: createAttribute (also occurs below)

[ihab.awad, 2013-07-22 20:20:21]

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/generic-host-page.html (right):

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/generic-host-page.html:119: .api({
On 2013/07/16 19:49:18, kpreid_google wrote:
> The point of this page is to provide an _unmodified_ Caja environment. I can
see
> the use of this, but you should make it opt-in — just add another checkbox to
> the form.
> 
> (We also ought to have a single standard implementation of "just pass on
> alert()", but ...)
> 
> (Now, the _right_ solution for debuggability is to provide 'console' by
default,
> but that's more work.)

Done.

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/third-party-tests.json (right):

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:6: "guest-template":
"/third_party/js/jqueryjs/test/index.html?dev=true&module=*",
On 2013/07/16 19:49:18, kpreid_google wrote:
> What does dev=true do?

Use un-minified jQuery files.

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:13:
"https://code.google.com/p/google-caja/issues/detail?id=1199",
On 2013/07/16 19:49:18, kpreid_google wrote:
> Links are good. Just links, not so good. This doesn't help the reader figure
out
> whether the failure they're seeing is known.

I don't want to add redundant hand-curated info, so maybe we can make the
renderer render the bug synopsis or something, but for the JSON, just links is
necessary and sufficient.

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:57: "comment": [
On 2013/07/16 19:49:18, kpreid_google wrote:
> delete this

Done.

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:87: "TODO(ihab.awad): Amidst
this file with diagnosis and bug filing"
On 2013/07/16 19:49:18, kpreid_google wrote:
> That's a bit cryptic in case someone else is picking up the work.

Done.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
File third_party/js/jqueryjs/test/unit/attributes.js (right):

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:82: // TODO(ihab.awad): 'target'
attributes are rewritten in Caja.
On 2013/07/16 19:49:18, kpreid_google wrote:
> My practice has been to label modifications "Patched for Caja" so we can
> uniformly grep and forward-port them. If you want to use other labels, please
> add documentation of them to jqueryjs/README-Caja.txt.
> 
> (Actually, that file doesn't even mention "Patched for Caja". I see I confused
> it with third_party/js/proxies/README-Caja which has a similar role. Please
add
> suitable text to the one for jQuery.)
> 
> And unless you're planning to fix the visibility of attribute rewriting, I
don't
> see that this is a TODO. 
> 
> tab indent lines in this file for consistency

Done.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:157: // TODO(ihab.awad):
'onclick' JavaScript attributes are rewritten in Caja.
On 2013/07/16 19:49:18, kpreid_google wrote:
> tab indent

Done.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:350: // TODO(ihab.awad): Setting
'autofocus' property throws, so we need to add the
On 2013/07/16 19:49:18, kpreid_google wrote:
> That was fixed in r5483.

Done.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:375: // TODO(ihab.awad):
document.createAttrbute() is nonexistent, so we need to
On 2013/07/16 19:49:18, kpreid_google wrote:
> typo: createAttribute (also occurs below)

Done.

[kpreid_google, 2013-07-22 20:33:38]

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/third-party-tests.json (right):

https://codereview.appspot.com/11291043/diff/3001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/third-party-tests.json:6: "guest-template":
"/third_party/js/jqueryjs/test/index.html?dev=true&module=*",
On 2013/07/22 20:20:21, ihab.awad wrote:
> On 2013/07/16 19:49:18, kpreid_google wrote:
> > What does dev=true do?
> 
> Use un-minified jQuery files.

Instead of hardcoding it here, have qunit-test-driver.js read the minified= URL
param and insert it there (make sure it works the same way for jQuery UI). That
way, automated tests (ant runtpt) still test against the minified files.

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
File third_party/js/jqueryjs/test/unit/attributes.js (right):

https://codereview.appspot.com/11291043/diff/3001/third_party/js/jqueryjs/tes...
third_party/js/jqueryjs/test/unit/attributes.js:82: // TODO(ihab.awad): 'target'
attributes are rewritten in Caja.
On 2013/07/22 20:20:21, ihab.awad wrote:
> On 2013/07/16 19:49:18, kpreid_google wrote:
> > My practice has been to label modifications "Patched for Caja" so
> > we can uniformly grep and forward-port them. If you want to use
> > other labels, please add documentation of them to
> > jqueryjs/README-Caja.txt.
> > 
> > (Actually, that file doesn't even mention "Patched for Caja". I see I
> > confused it with third_party/js/proxies/README-Caja which has
> > a similar role. Please add suitable text to the one for jQuery.)
> [...]
> 
> Done.

Not done? I see no changes to README-Caja.

https://codereview.appspot.com/11291043/diff/10001/tests/com/google/caja/plug...
File tests/com/google/caja/plugin/generic-host-page.html (right):

https://codereview.appspot.com/11291043/diff/10001/tests/com/google/caja/plug...
tests/com/google/caja/plugin/generic-host-page.html:81: <label><input
type="checkbox" name="alert" value="true"> Provide <code>alert</code></label>
long line — existing line was long, but both should be fixed. Suggest breaking
between ">" and "Provide".

https://codereview.appspot.com/11291043/diff/10001/third_party/js/jqueryjs/te...
File third_party/js/jqueryjs/test/unit/event.js (right):

https://codereview.appspot.com/11291043/diff/10001/third_party/js/jqueryjs/te...
third_party/js/jqueryjs/test/unit/event.js:1849: test('ihab got up to here',
function() {
"Patched for Caja" somewhere in here

[ihab.awad, 2013-07-22 21:25:00]

Verified "dev=" works for jQuery ... will get to jQuery UI when we start
debugging there.

https://codereview.appspot.com/11291043/diff/10001/tests/com/google/caja/plug...
File tests/com/google/caja/plugin/generic-host-page.html (right):

https://codereview.appspot.com/11291043/diff/10001/tests/com/google/caja/plug...
tests/com/google/caja/plugin/generic-host-page.html:81: <label><input
type="checkbox" name="alert" value="true"> Provide <code>alert</code></label>
On 2013/07/22 20:33:39, kpreid_google wrote:
> long line — existing line was long, but both should be fixed. Suggest breaking
> between ">" and "Provide".

Done.

https://codereview.appspot.com/11291043/diff/10001/third_party/js/jqueryjs/te...
File third_party/js/jqueryjs/test/unit/event.js (right):

https://codereview.appspot.com/11291043/diff/10001/third_party/js/jqueryjs/te...
third_party/js/jqueryjs/test/unit/event.js:1849: test('ihab got up to here',
function() {
On 2013/07/22 20:33:39, kpreid_google wrote:
> "Patched for Caja" somewhere in here

Done.

[kpreid_google, 2013-07-22 21:37:24]

LGTM

[ihab.awad, 2013-07-24 16:39:29]

@5499