Cleanup A11y tests and test-suite organization

RESOLVED FIXED in mozilla14

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug][lang=js] → [lang=js]
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?
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.
(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
Whiteboard: [lang=js] → [good first bug][mentor=surkov.alexander@gmail.com][lang=js]
(Assignee)

Comment 4

7 years ago
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?
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(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.
(Assignee)

Comment 6

7 years ago
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 ...
Attachment #612525 - Flags: feedback?(surkov.alexander)
(Assignee)

Updated

7 years ago
Depends on: 739179
Attachment #612525 - Flags: review?(marco.zehe)
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.
Attachment #612525 - Flags: review?(marco.zehe)
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
(Assignee)

Comment 9

7 years ago
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...
Attachment #612525 - Attachment is obsolete: true
Attachment #612525 - Flags: feedback?(surkov.alexander)
Attachment #612634 - Flags: review?(marco.zehe)
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
Attachment #612634 - Flags: feedback+
(Assignee)

Comment 11

7 years ago
Created attachment 612984 [details] [diff] [review]
Patch (v3)

Here's the new one...........
Attachment #612984 - Flags: review?(marco.zehe)
Attachment #612634 - Attachment is obsolete: true
Attachment #612634 - Flags: review?(marco.zehe)
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.
Attachment #612984 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 13

7 years ago
Created attachment 613548 [details] [diff] [review]
Patch (v4) with last nits
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=js] → [autoland-try:613548][good first bug][mentor=surkov.alexander@gmail.com][lang=js]
Attachment #612984 - Attachment is obsolete: true

Updated

7 years ago
Whiteboard: [autoland-try:613548][good first bug][mentor=surkov.alexander@gmail.com][lang=js] → [autoland-in-queue][good first bug][mentor=surkov.alexander@gmail.com][lang=js]
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
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

Updated

7 years ago
Whiteboard: [autoland-in-queue][good first bug][mentor=surkov.alexander@gmail.com][lang=js] → [good first bug][mentor=surkov.alexander@gmail.com][lang=js]
https://hg.mozilla.org/mozilla-central/rev/671cc6b005f3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.