Closed Bug 416553 Opened 12 years ago Closed 12 years ago

option to run accessibility tests

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

spun off bug 415730

Rob wrote:

(In reply to comment #6)
> I think we should do something similar to the way we're running browser-chrome
> and chrome tests. Provide a separate option to runtests.pl|y (--a11y ?) that
> runs tests from a separate location. We might want to keep them isolated.

attached file works though it doesn't allow to run all tests. I cannot find where similar feature of usual tests are implemented.
Attached patch patch (obsolete) — Splinter Review
will the a11y tests run with chrome privilegs?
(In reply to comment #2)
> will the a11y tests run with chrome privilegs?
> 

they need to operate with XPCOM.
Attached patch patch2 (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #302302 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #302368 - Flags: review?(rcampbell)
Why can't you use the existing --chrome method of running tests for this?  Under that setup, tests run from chrome://mochitest/content/<something more I can't remember>, and you have chrome privileges, can open XUL windows to avoid influence of browser chrome, etc.  From my point of view, the fewer functionality-specific tests pushed into runtests.p[ly].in, the better, and I don't see anything at a glance that requires this to be pushed into a type of testing.
Attached patch patch3 (obsolete) — Splinter Review
use -chrome method as Jeff suggested
Attachment #302368 - Attachment is obsolete: true
Attachment #302518 - Flags: review?(rcampbell)
Attachment #302368 - Flags: review?(rcampbell)
That's not quite what I suggested -- what I meant was don't even change runtests.p[yl].in or any of the test harness at all (so nothing in testing/mochitest would change), and just run the accessibility tests within the --chrome setup.  The tests get run from chrome: URLs, and I don't see what you can't do with them that you can only do with yet another flavor of Mochitest harness.
Yes, but that's Rob suggested to have an option to run accessibility tests separately at least because some mochitests become broken with enabled accessibility. Sure those tests should fail with enabled accessibility but I think it's ok to have an ability to run accessibility related tests separately.
Attached patch patch4Splinter Review
added testcase for bug 308564.
Attachment #302518 - Attachment is obsolete: true
Attachment #302599 - Flags: review?(rcampbell)
Attachment #302518 - Flags: review?(rcampbell)
(In reply to comment #7)
> That's not quite what I suggested -- what I meant was don't even change
> runtests.p[yl].in or any of the test harness at all (so nothing in
> testing/mochitest would change), and just run the accessibility tests within
> the --chrome setup.  The tests get run from chrome: URLs, and I don't see what
> you can't do with them that you can only do with yet another flavor of
> Mochitest harness.

Jeff: I'd prefer having the a11y tests running separately at the moment. It's probably possible to use the --chrome option with some specific prefs enabled to run the tests, but I didn't want to contaminate any of the existing test harnesses with side-effects (or the main tree).

Thanks for the patches, alexander. I'll get to a review in a couple of days when I've dug out of my bugmail.
Blocks: 416742
Comment on attachment 302599 [details] [diff] [review]
patch4

one pervasive nit: change occurrences of "ally" to "a11y" (l->1) and "ALLY" to "A11Y". This includes filenames, directories, code and comments.

remove hard tabs in mozilla/testing/mochitest/harness-overlay.xul.

otherwise, I think this patch looks good. I didn't have an a11y tests to run in it, but the harness brings up minefield (on mac, at least). I also checked that it didn't break --chrome or --browser-chrome and those seemed fine as well.

I'm going to give a conditional r+ based on the above nits being addressed.

Thanks for your help!
Attachment #302599 - Flags: review?(rcampbell) → review+
Rob, I forgot to fix your comments and checked in the patch as is. Your comments doesn't look critical so let me do this in following up patch.
(In reply to comment #11)
> (From update of attachment 302599 [details] [diff] [review])
> one pervasive nit: change occurrences of "ally" to "a11y" (l->1) and "ALLY" to
> "A11Y". This includes filenames, directories, code and comments.

though not for this one. Rob if you really don't like ally (instead of a11y) though they both are used in the same sense then for sure I'll back out the patch and will fix it.
Ok. I looked at usage of ally and a11y. It sounds ally word is used not offical, in the speach. So I'm going to back up the patch to fix ally/a11y.
checked in with Rob's comments.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: a11ytestdev
alexander: in the future, if someone gives you a conditional review, make the changes before checking in, especially if the nits involve file and directory naming. It's a pain to rename patches after the fact.

Also, when checking in, please, include the cvs logs in the bug either as an attachment or appended as a comment. Thanks.
(In reply to comment #16)
> alexander: in the future, if someone gives you a conditional review, make the
> changes before checking in, especially if the nits involve file and directory
> naming. It's a pain to rename patches after the fact.

Sure, I just missed this, sorry. But in the end I checked in patch with your comments.

> Also, when checking in, please, include the cvs logs in the bug either as an
> attachment or appended as a comment. Thanks.
> 

If it should be helpful then I will.
Alexander, currently, this --a11y switch only picks up .xul files/chrome tests, right? However, we need to be able to also run HTML-based tests. These are equally important. Right now, no HTML files are picked up at all if I run with the --a11y switch.
(In reply to comment #18)
> Alexander, currently, this --a11y switch only picks up .xul files/chrome tests,
> right?

Not sure I got you. -a11y picks up tests from _tests/testing/mochitests/a11y directory. Currently only accessible/tests/mochitest tests are moved there.

 However, we need to be able to also run HTML-based tests. These are
> equally important. Right now, no HTML files are picked up at all if I run with
> the --a11y switch.
> 

I tried html mochitest from bug 416742 it works fine.
(In reply to comment #19)
> I tried html mochitest from bug 416742 it works fine.

What command did you use to run it? Did it show up when you simply ran runtests.py --a11y? It doesn't for me.
(In reply to comment #17)
> (In reply to comment #16)
> > alexander: in the future, if someone gives you a conditional review, make the
> > changes before checking in, especially if the nits involve file and directory
> > naming. It's a pain to rename patches after the fact.
> 
> Sure, I just missed this, sorry. But in the end I checked in patch with your
> comments.

Yes, true. I'm just saying that when dealing with the source repository order does matter. I think had you tried to do the changes in place with the code checked in already, it would have required backing out and then re-submitting. A seemingly minor point that a correct order would have avoided.

Anyway, you got it and that's the main thing. :)(In reply to comment #17)
> > Also, when checking in, please, include the cvs logs in the bug either as an
> > attachment or appended as a comment. Thanks.
> 
> If it should be helpful then I will.

It's useful because you can see the version numbers associated with the files in the bug itself if there's ever a need to backout a change.
(In reply to comment #20)
> (In reply to comment #19)
> > I tried html mochitest from bug 416742 it works fine.
> 
> What command did you use to run it? Did it show up when you simply ran
> runtests.py --a11y? It doesn't for me.
> 

Did you put it into accessible/tests/mochitest, change makefile and build the tree? Or did you put it directly to _tests/testing/mochitests/a11y/accessible? I'll try one more time today.
Marco, if you put it into the right directory then you probably gave it wrong name, the file name should contain "test" word to be recognized as mochitest. For that mochitest I use "test_bug410052.html". It works fine with me.
(In reply to comment #23)
> Marco, if you put it into the right directory then you probably gave it wrong
> name, the file name should contain "test" word to be recognized as mochitest.
> For that mochitest I use "test_bug410052.html". It works fine with me.
That was it, works now, thanks!
Status: RESOLVED → VERIFIED
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
You need to log in before you can comment on or make changes to this bug.