Cleanup A11y tests and test-suite organization

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 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

5 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

5 years ago
Whiteboard: [good first bug][lang=js] → [lang=js]

Comment 1

5 years ago
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?

Updated

5 years ago
Blocks: 417472
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

5 years ago
(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

Updated

5 years ago
Whiteboard: [lang=js] → [good first bug][mentor=surkov.alexander@gmail.com][lang=js]
(Assignee)

Comment 4

5 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

Comment 5

5 years ago
(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

5 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

5 years ago
Depends on: 739179

Updated

5 years ago
Attachment #612525 - Flags: review?(marco.zehe)

Comment 7

5 years ago
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)

Comment 8

5 years ago
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

5 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)

Updated

5 years ago
Attachment #612634 - Flags: review?(marco.zehe)

Comment 10

5 years ago
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

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

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

Updated

5 years ago
Attachment #612984 - Flags: review?(marco.zehe)

Updated

5 years ago
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

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

Updated

5 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]

Updated

5 years ago
Attachment #612984 - Attachment is obsolete: true

Updated

5 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]

Comment 14

5 years ago
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

5 years ago
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

5 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]

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/671cc6b005f3
https://hg.mozilla.org/mozilla-central/rev/671cc6b005f3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.