Last Comment Bug 739612 - Cleanup A11y tests and test-suite organization
: Cleanup A11y tests and test-suite organization
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on: 739179
Blocks: a11ytestdev
  Show dependency treegraph
 
Reported: 2012-03-27 08:11 PDT by Mark Capella [:capella]
Modified: 2012-04-11 09:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (13.28 KB, patch)
2012-04-05 06:51 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (26.18 KB, patch)
2012-04-05 12:05 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v3) (25.76 KB, patch)
2012-04-06 12:55 PDT, Mark Capella [:capella]
mzehe: review+
Details | Diff | Splinter Review
Patch (v4) with last nits (26.03 KB, patch)
2012-04-10 04:40 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Mark Capella [:capella] 2012-03-27 08:11:33 PDT
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 2012-03-27 23:27:53 PDT
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] 2012-03-28 06:38:45 PDT
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 2012-03-28 06:43:41 PDT
(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] 2012-04-04 09:09:33 PDT
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 2012-04-04 20:46:24 PDT
(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] 2012-04-05 06:51:51 PDT
Created attachment 612525 [details] [diff] [review]
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) 2012-04-05 07:58:03 PDT
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 2012-04-05 08:01:57 PDT
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] 2012-04-05 12:05:40 PDT
Created attachment 612634 [details] [diff] [review]
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 2012-04-05 20:42:06 PDT
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] 2012-04-06 12:55:56 PDT
Created attachment 612984 [details] [diff] [review]
Patch (v3)

Here's the new one...........
Comment 12 Marco Zehe (:MarcoZ) 2012-04-10 03:29:49 PDT
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] 2012-04-10 04:40:05 PDT
Created attachment 613548 [details] [diff] [review]
Patch (v4) with last nits
Comment 14 Mozilla RelEng Bot 2012-04-10 11:40:51 PDT
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 2012-04-10 16:01:06 PDT
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/autolanduser@mozilla.com-b0e41b7026f1
Comment 17 Matt Brubeck (:mbrubeck) 2012-04-11 09:36:55 PDT
https://hg.mozilla.org/mozilla-central/rev/671cc6b005f3

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