Last Comment Bug 736886 - test_nsIAccessibleDocument.html silently dies
: test_nsIAccessibleDocument.html silently dies
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 735805
Blocks: a11ytestdev 736517 441737
  Show dependency treegraph
 
Reported: 2012-03-18 13:58 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2012-05-18 19:19 PDT (History)
2 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (2.73 KB, patch)
2012-05-16 11:35 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (final) (3.09 KB, patch)
2012-05-17 04:27 PDT, Mark Capella [:capella]
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-18 13:58:14 PDT
accessible/tests/mochitest/test_nsIAccessibleDocument.html seems to be running no tests, as indicated by <https://tbpl.mozilla.org/php/getParsedLog.php?id=10128146&tree=Try#error1>.  Serge says in bug 735805 comment 36:

> This a11y test is broken: getRootDirectory() is defined in
> chrome-harness.js, but including the latter just silently breaks somehow.
> Move this issue to a blocking bug.

For now I'll most likely have to add a todo(), because I can't figure out exactly how it's broken -- I have trouble getting it to work on localhost.
Comment 1 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-18 14:47:05 PDT
I got the test to work on localhost with Serge's help, and tracked down the basic problem -- including chrome-harness.js seems to break.  I commented it out for now, along with the check that relied on it, in a patch I'll attach to bug 735805.  I'm leaving this open as a followup to actually fix the test.  (Previously the test wasn't running any checks at all, but with the patch it's at least running some of them.)
Comment 2 Mark Capella [:capella] 2012-05-16 11:35:34 PDT
Created attachment 624467 [details] [diff] [review]
Patch (v1)

Small change ... asking surkov for feedback ...
Comment 3 Serge Gautherie (:sgautherie) 2012-05-16 12:03:40 PDT
Mark, do you have any idea about what goes wrong when using the chrome:// url in this test?

http://mxr.mozilla.org/mozilla-central/search?string=%2Fchrome-harness.js&case=1&find=%2Faccessible%2Ftests%2Fmochitest%2F
Comment 4 Mark Capella [:capella] 2012-05-16 13:15:52 PDT
Well, I played with it until I got the correct syntax. The below substitution works also...

Your original line was... src="chrome://mochikit/content/chrome-harness.js"/>
You could change it to... src="chrome://mochikit/content/chrome-harness.js"></script>
Comment 5 Serge Gautherie (:sgautherie) 2012-05-16 15:01:04 PDT
(In reply to Mark Capella [:capella] from comment #4)
> You could change it to...
> src="chrome://mochikit/content/chrome-harness.js"></script>

I would prefer that syntax over the '..' one in your patch ;-)

Yet, my question was really "is the breakage due to some code is that file?"...
Comment 6 alexander :surkov 2012-05-17 00:31:03 PDT
Comment on attachment 624467 [details] [diff] [review]
Patch (v1)

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

thank you for fixing this!

::: accessible/tests/mochitest/test_nsIAccessibleDocument.html
@@ +18,2 @@
>    <script type="application/javascript"
> +    src="../../chrome-harness.js"></script>

nit: please align it properly

@@ +39,5 @@
>          attributes = docAcc.attributes;
>          is(attributes.getStringProperty("tag"), "body",
>             "Wrong attribute on document!");
>  
> +        // Test for correct root directory.

if you want comments for this then it'd be nice to have
// Document URL

and

// Document title and mime type.

@@ +50,4 @@
>             "Wrong title for document!");
>          is(docAcc.mimeType, "text/html",
>             "Wrong mime type for document!");
> +

nit: whitespace on empty line
Comment 7 alexander :surkov 2012-05-17 00:32:24 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #5)

> Yet, my question was really "is the breakage due to some code is that
> file?"...

it seems so, HTML is not friendly to <script> close tag.
Comment 8 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-05-17 02:41:01 PDT
I feel extremely stupid about this.  Feeding it into validator.nu would have showed up the problem immediately -- self-closing tags are not allowed in .html files and don't do anything.  <foo /> is exactly the same as <foo>.  So it was being parsed as a <script> with both src="" and contents, and the contents were therefore being ignored.

Thanks for spotting this!  This is a motivation for fixing bug 736517, which would have caught the problem -- although apparently that's not easy.
Comment 9 Mark Capella [:capella] 2012-05-17 04:27:45 PDT
Created attachment 624706 [details] [diff] [review]
Patch (final)

This is the final version.
Comment 10 alexander :surkov 2012-05-17 06:29:17 PDT
(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 624706 [details] [diff] [review]
> Patch (final)
> 
> This is the final version.

cool, thank you!
Comment 11 Serge Gautherie (:sgautherie) 2012-05-17 07:32:20 PDT
Comment on attachment 624706 [details] [diff] [review]
Patch (final)

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> http://mxr.mozilla.org/mozilla-central/search?string=%2Fchrome-harness.
> js&case=1&find=%2Faccessible%2Ftests%2Fmochitest%2F

Then: 1 XML with />, 2 HTML with </, and this broken HTML with />.


(In reply to Serge Gautherie (:sgautherie) from comment #5)
> (In reply to Mark Capella [:capella] from comment #4)
> > You could change it to...
> > src="chrome://mochikit/content/chrome-harness.js"></script>
> 
> I would prefer that syntax over the '..' one in your patch ;-)

Please, do use </script> as in the 2 other HTML files.
Comment 12 Mark Capella [:capella] 2012-05-17 07:45:26 PDT
Serge, is there something you'd like to suggest we change in the final patch? I don't see your concern at this stage.
Comment 13 Serge Gautherie (:sgautherie) 2012-05-17 09:24:08 PDT
I meant "use your comment 4 suggestion": no need to add a new/third (less good) syntax.
Comment 14 Mark Capella [:capella] 2012-05-17 11:29:17 PDT
Without objections.

TRY push ... https://tbpl.mozilla.org/?tree=Try&rev=a77b0a028414
Comment 15 Mark Capella [:capella] 2012-05-17 17:13:31 PDT
Inbound status:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a56ebbf0f293
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-05-18 13:25:55 PDT
https://hg.mozilla.org/mozilla-central/rev/a56ebbf0f293
Comment 17 Serge Gautherie (:sgautherie) 2012-05-18 19:19:36 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337377564.1337381641.22500.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 14:46:04
{
16725 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html | Wrong URL for document! - chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html should equal chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html
16726 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html | Wrong title for document! - nsIAccessibleDocument chrome tests should equal nsIAccessibleDocument chrome tests
}

V.Fixed

Note You need to log in before you can comment on or make changes to this bug.