739612 - Cleanup A11y tests and test-suite organization

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

The Accessibility test suite has grown in complexity and could use a logical re-structuring. ("Section" tests from bug614310 might have benefited here).

The requirements here are not yet finalized. Count on the mentor, and any additional follow-up comments for direction.

Comment 1

[alexander :surkov (:asurkov)]

1) move test_role_nsHyperTextAcc.html to role/test_general.html under "nsHyperTextAcc tests" section
2) move HTML role tests from elm/test_landmark.html to role/test_nsHyperTextAcc.html
3) move test_aria_roles.html to role/test_aria.html
4) move role tests from test_aria_role_article.html and test_aria_role_equation.html to role/test_aria.html
5) move test_aria_roles.xul to role/test_aria.xul
6) move xml-roles tests from elm/test_landmark.html to attributes/test_xml-roles.html
7) move tag tests from elm/test_landmark.html to attributes/test_tag.html
8) remove elm/test_landmark.html
9) move name tests from test_aria_role_article.html and test_aria_role_equation.html (expect img name test from alt attribute) to name/test_general.html under "name from children" section

If no objections then let's mark it as good first bug.

to David: this approach is API oriented (as we do it now), i.e. not spec oriented (as you would like). I see benefits of spec oriented testing but I think we should stay on API oriented approach since it's closer to development process. Nevertheless I suggest to start the work on spec oriented test suite in parallel. Wanna file bug for that?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Let's go with your plan. Note we usually use the plural form (like "roles") for our directories. The spec tests will likely be done outside the mochitest infrastructure. Possibly MATS, but not sure yet.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)
> Let's go with your plan. Note we usually use the plural form (like "roles")
> for our directories.

we use plural for things that are plural, compare events, attributes vs name, value.

> The spec tests will likely be done outside the
> mochitest infrastructure. Possibly MATS, but not sure yet.

we can go with either way

Comment 4

[Mark Capella [:capella]]

Alex, I'll need a new bug soon, and this one should keep me busy. Would you want to do this all at once? Or would it be easier for you to review and approve smaller blocks?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #4)
> Alex, I'll need a new bug soon,

If you'd like c++ instead then take a look at bug 678429. Or as next one :)

> and this one should keep me busy. Would you
> want to do this all at once? Or would it be easier for you to review and
> approve smaller blocks?

It's up to you, I can handle either case.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v1)

Here's a rought first draft.

I couldn't locate test_aria_role_equation.html in item #9 ...

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 612525 [details] [diff] [review]
Patch (v1)

This patch is still missing various important pieces, but the general principle is correct.

>diff --git a/accessible/tests/mochitest/test_aria_roles.html b/accessible/tests/mochitest/role/test_aria.html
>rename from accessible/tests/mochitest/test_aria_roles.html
>rename to accessible/tests/mochitest/role/test_aria.html
>--- a/accessible/tests/mochitest/test_aria_roles.html
>+++ b/accessible/tests/mochitest/role/test_aria.html
>@@ -14,23 +14,35 @@ https://bugzilla.mozilla.org/show_bug.cg
>           src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> 
>   <script type="application/javascript"
>           src="common.js"></script>
>   <script type="application/javascript"
>           src="role.js"></script>
 
In this, and other files you move to directories of a different depth, you must adjust the include file patchs to include "../" like above, or the test files won't find the common.js files.

Second, the Makefile.in files must be adjusted, entries moved or removed depending on whether you move or remove files.

Also, if you move stuff, don't remove the comments in testing what type of landmarks are being tested.

Cancelling review and waiting for next round.

Comment 8

[alexander :surkov (:asurkov)]

btw, I think

1) move test_role_nsHyperTextAcc.html to role/test_general.html under "nsHyperTextAcc tests" section
2) move HTML role tests from elm/test_landmark.html to role/test_nsHyperTextAcc.html

2nd item is sort of misspelling, those roles should be under test_general.html. sorry for that

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v2)

Alright! I corrected the 2nd item (sort of misspelling) and assumed the main requirements now to be stable.

I've gone back and made another pass at the comments, ensuring all were moved / copied / split out to follow their tests, put the makefile.in changes in place, and corrected the include file patchs to include "../". Nice catch on that one, it didn't occur to me but sure showed up during testing!

I re-built and re-tested all successfully. With any luck, that should cover major issues and we're down to nits...

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 612634 [details] [diff] [review]
Patch (v2)

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

::: accessible/tests/mochitest/attributes/Makefile.in
@@ +56,2 @@
>  		test_text.html \
> +		test_xml_roles.html \

please name it test_xml-roles.html since xml-roles is object attribute name

::: accessible/tests/mochitest/name/test_general.html
@@ +19,5 @@
> +    {
> +      var eqAcc = getAccessible(aID);
> +      if (eqAcc)
> +        is(eqAcc.name, "x^2 + y^2 + z^2", "Wrong name for " + aID + "!");
> +    }

I'd prefer to not have a function for this, just use testName()

@@ +145,5 @@
>        testName("label_with_acronym", "O A T F World Wide Web");
>  
> +      var articleAcc = getAccessible("testArticle");
> +      if (articleAcc)
> +        is(articleAcc.name, "Test article", "Wrong name for article!");

testName please

::: accessible/tests/mochitest/test_aria_roles.html
@@ +6,5 @@
>  https://bugzilla.mozilla.org/show_bug.cgi?id=481114
>  https://bugzilla.mozilla.org/show_bug.cgi?id=469688
>  https://bugzilla.mozilla.org/show_bug.cgi?id=520188
>  https://bugzilla.mozilla.org/show_bug.cgi?id=529289
>  -->

you can just remove whole this block

@@ +24,5 @@
> +
> +    function testThis(aID)
> +    {
> +      getRole(aID, ROLE_FLAT_EQUATION);
> +    }

you don't need function for this

@@ +96,5 @@
>        document.body.setAttribute("role", "application");
>        testRole(testDoc, ROLE_APPLICATION);
>  
> +      // Test equation image
> +      testThis("img_eq");

just testRole

::: accessible/tests/mochitest/test_role_nsHyperTextAcc.html
@@ +21,5 @@
> +      testRole("header", ROLE_HEADER);
> +      testRole("footer", ROLE_FOOTER);
> +      testRole("article", ROLE_DOCUMENT);
> +      testRole("aside", ROLE_NOTE);
> +      testRole("section", ROLE_SECTION); // XXX bug 739612: not a landmark

no XXX, since this bug you work on is mentioned bug

actually all these test should be under "nsHyperTextAccessible tests section"

@@ +24,5 @@
> +      testRole("aside", ROLE_NOTE);
> +      testRole("section", ROLE_SECTION); // XXX bug 739612: not a landmark
> +
> +      testRole("main", ROLE_DOCUMENT);
> +      testRole("form", ROLE_FORM);

these two should go into test_aria.html

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v3)

Here's the new one...........

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 612984 [details] [diff] [review]
Patch (v3)

>+      // Test equation image
>+      getRole("img_eq", ROLE_FLAT_EQUATION);
>+
>+      // Test textual equation
>+      getRole("txt_eq", ROLE_FLAT_EQUATION);

I believe you meant to write "testRole" instead.

>+      // nsHyperTextAcc tests section
>       // Test html:form.
>+      testRole("nav", ROLE_SECTION);
>+      testRole("header", ROLE_HEADER);
>+      testRole("footer", ROLE_FOOTER);
>+      testRole("article", ROLE_DOCUMENT);
>+      testRole("aside", ROLE_NOTE);
>+      testRole("section", ROLE_SECTION);
>       testRole("frm", ROLE_FORM);

Please put the landmark tests in their own section and preceede with an appropriate comment.

r=me with those two fixed.

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v4) with last nits

Comment 14

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613548
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b0e41b7026f1
Try run started, revision b0e41b7026f1. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b0e41b7026f1

Comment 15

[Mozilla RelEng Bot]

Try run for b0e41b7026f1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b0e41b7026f1
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b0e41b7026f1

Comment 16

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/671cc6b005f3

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/671cc6b005f3