The current test tries to sniff for an a11y document load event for the right URL, grab a reference to its window and close it when the test cleans up. We can't get a reference for the outer window from a document accessible in e10s (or at least its not easy?)

MozReview-Commit-ID: 6drHKshtAjX

MozReview-Commit-ID: 4UDrxbq903j

Comment on attachment 8870897 [details] [diff] [review] Convert states/test_link.html to browser test. r?surkov Review of attachment 8870897 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/browser/states/browser_test_link.js @@ +35,5 @@ > + > + // a: no traversed state > + testStates(_("link_traversed"), 0, 0, STATE_TRAVERSED); > + > + let onUpdate = waitForEvent(EVENT_STATE_CHANGE, "link_traversed"); afaik Yura wanted to replace mochitests events.js queues and stuff on promise-based methods for browser tests. I agree it'd be good to share event constants between the tests suites, but it should be nice to give a second promise-like life for other stuff. What do you think?

Comment on attachment 8870897 [details] [diff] [review] Convert states/test_link.html to browser test. r?surkov Review of attachment 8870897 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/browser/states/browser_test_link.js @@ +35,5 @@ > + > + // a: no traversed state > + testStates(_("link_traversed"), 0, 0, STATE_TRAVERSED); > + > + let onUpdate = waitForEvent(EVENT_STATE_CHANGE, "link_traversed"); oh, I missed there's a browser tests's version of events.js, so please ignore my previous comment

Comment on attachment 8870897 [details] [diff] [review] Convert states/test_link.html to browser test. r?surkov Review of attachment 8870897 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/browser/states/browser_test_link.js @@ +11,5 @@ > + > +async function runTests(browser, accDoc) { > + let _ = id => findAccessibleChildByID(accDoc, id); > + > + testStates(_("link_href"), STATE_LINKED); it is a fancy name, I got usual to explicit getAccessible name, but maybe it's ok @@ +35,5 @@ > + > + // a: no traversed state > + testStates(_("link_traversed"), 0, 0, STATE_TRAVERSED); > + > + let onUpdate = waitForEvent(EVENT_STATE_CHANGE, "link_traversed"); maybe rename onUpdate to onLinkClicked/onLinkTraversed or something else more explicit? btw, events.js and head.js define both waitForEvent functions, which is confusing a bit, it'd be good to have one shared version to avoid possible conflicts ::: accessible/tests/mochitest/states/test_link.html @@ -84,5 @@ > - // a (no @href, no click event listener) > - testStates("link_notlink", 0, 0, STATE_LINKED); > - > - // a: no traversed state > - testStates("link_traversed", 0, 0, STATE_TRAVERSED); should we keep these in mochitests and move event queue part only to the browser tests? or probably keep two separate versions (mochitest and browser ones) of this test? at least until we have a certain plan about mochitest. otherwise it may appear confusing that some tests are in mochitests and some in browser tests, and it's not clear what is the guideline for adding new tests.

(In reply to alexander :surkov from comment #6) > Comment on attachment 8870897 [details] [diff] [review] > Convert states/test_link.html to browser test. r?surkov > > Review of attachment 8870897 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/tests/browser/states/browser_test_link.js > @@ +11,5 @@ > > + > > +async function runTests(browser, accDoc) { > > + let _ = id => findAccessibleChildByID(accDoc, id); > > + > > + testStates(_("link_href"), STATE_LINKED); > > it is a fancy name, I got usual to explicit getAccessible name, but maybe > it's ok You mean the _ function? I can change it to getAccessible. > > @@ +35,5 @@ > > + > > + // a: no traversed state > > + testStates(_("link_traversed"), 0, 0, STATE_TRAVERSED); > > + > > + let onUpdate = waitForEvent(EVENT_STATE_CHANGE, "link_traversed"); > > maybe rename onUpdate to onLinkClicked/onLinkTraversed or something else > more explicit? onStateChanged? > > btw, events.js and head.js define both waitForEvent functions, which is > confusing a bit, it'd be good to have one shared version to avoid possible > conflicts Ah, didn't see that till now. The one in head.js is only used in the tests directly in the 'browser' dir (not the 'states' or 'e10s' subdirs), I think that should be revisited in another bug to remove one of those implementations. > > ::: accessible/tests/mochitest/states/test_link.html > @@ -84,5 @@ > > - // a (no @href, no click event listener) > > - testStates("link_notlink", 0, 0, STATE_LINKED); > > - > > - // a: no traversed state > > - testStates("link_traversed", 0, 0, STATE_TRAVERSED); > > should we keep these in mochitests and move event queue part only to the > browser tests? I would like to see us rely less on eventQueue and use async/await where appropriate, it makes it easier to read tests. I think there are a lot of new features in JS that make writing and reading async tests simpler, and we should take advantage of them. For more complicated stuff, there may be need for eventQueue (or something similar), but for basic automation we don't need it. > or probably keep two separate versions (mochitest and browser > ones) of this test? Because non-e10s mode is going away, some mochitests (like this one) will break permanently. I think the browser ones should replace them, and we should delete the mochitest ones. Then there will be no confusion as to which test is canonical. > at least until we have a certain plan about mochitest. > otherwise it may appear confusing that some tests are in mochitests and some > in browser tests, and it's not clear what is the guideline for adding new > tests. Yeah, I think we need clarity as to what we need to exercise and check for: 1. xpc API of content in same process? 2. xpc API of child content in parent process? .. and what form of test is good for that. Right now the browser tests do a pretty good job of that because they run both in e10s mode and non-e10s mode. The mochitests are only run in non-e10s mode. I'm assuming non-e10s is going away altogether, which leaves us with the above dilemma. As for style, I am biased towards how our browser tests are written. They are more modern, readable, and easier for newcomers to understand. So, if it were up to me, I would say all new tests should be written as browser tests, and we should slowly migrate mochitests to browser tests. You may have a different opinion :)

(In reply to Eitan Isaacson [:eeejay] from comment #7) > > > +async function runTests(browser, accDoc) { > > > + let _ = id => findAccessibleChildByID(accDoc, id); > > > + > > > + testStates(_("link_href"), STATE_LINKED); > > > > it is a fancy name, I got usual to explicit getAccessible name, but maybe > > it's ok > > > You mean the _ function? I can change it to getAccessible. Yeah. I don't have strong opinion on it. Don't people use $() for that? I think I saw it somewhere in the tests. > > > maybe rename onUpdate to onLinkClicked/onLinkTraversed or something else > > more explicit? > > onStateChanged? sounds good > > > > btw, events.js and head.js define both waitForEvent functions, which is > > confusing a bit, it'd be good to have one shared version to avoid possible > > conflicts > > Ah, didn't see that till now. The one in head.js is only used in the tests > directly in the 'browser' dir (not the 'states' or 'e10s' subdirs), I think > that should be revisited in another bug to remove one of those > implementations. new bug for that then? > > > - // a: no traversed state > > > - testStates("link_traversed", 0, 0, STATE_TRAVERSED); > > > > should we keep these in mochitests and move event queue part only to the > > browser tests? > > I would like to see us rely less on eventQueue and use async/await where > appropriate, it makes it easier to read tests. > I think there are a lot of new features in JS that make writing and reading > async tests simpler, and we should take advantage of them. For more > complicated stuff, there may be need for eventQueue (or something similar), > but for basic automation we don't need it. I'm good with this, however it doesn't mean the transition from mochitest to browser tests in general, correct? And those particular tests I was talking about are not about eventing. > > > or probably keep two separate versions (mochitest and browser > > ones) of this test? > > Because non-e10s mode is going away, some mochitests (like this one) will > break permanently. I think the browser ones should replace them, and we > should delete the mochitest ones. Then there will be no confusion as to > which test is canonical. I meant testStates() part, we have many mochitest in states folder, and if we move some of them into browsers tests leaving others in mochitests, then it's confusing. > > at least until we have a certain plan about mochitest. > > otherwise it may appear confusing that some tests are in mochitests and some > > in browser tests, and it's not clear what is the guideline for adding new > > tests. > > Yeah, I think we need clarity as to what we need to exercise and check for: > 1. xpc API of content in same process? I would say that most tests should fall into this category. They don't depend on the specificity of e10s a11y implementation and as a consequence the logic under the hood is simpler, which means it's easier to locate and fix the problem(regression). > 2. xpc API of child content in parent process? we should have some of them too, or - it'd be ideal - find a way how to run same tests in 1 and 2 categories. > .. and what form of test is good for that. Right now the browser tests do a > pretty good job of that because they run both in e10s mode and non-e10s > mode. The mochitests are only run in non-e10s mode. is my understanding correct that mochitests are still good for #1 category, i.e. mochitest can be run in e10s mode in general, and all we need is to fix bunch of failing tests? > I'm assuming non-e10s is > going away altogether, which leaves us with the above dilemma. > > As for style, I am biased towards how our browser tests are written. They > are more modern, readable, and easier for newcomers to understand. So, if it > were up to me, I would say all new tests should be written as browser tests, > and we should slowly migrate mochitests to browser tests. You may have a > different opinion :) except eventQueue thing, are there other non-modern stuff specific to our mochitests?

(In reply to alexander :surkov from comment #8) > (In reply to Eitan Isaacson [:eeejay] from comment #7) > > > > > +async function runTests(browser, accDoc) { > > > > + let _ = id => findAccessibleChildByID(accDoc, id); > > > > + > > > > + testStates(_("link_href"), STATE_LINKED); > > > > > > it is a fancy name, I got usual to explicit getAccessible name, but maybe > > > it's ok > > > > > > You mean the _ function? I can change it to getAccessible. > > Yeah. I don't have strong opinion on it. Don't people use $() for that? I > think I saw it somewhere in the tests. That suggests getting a DOM node from an id. I'll do getAcc()... > > > > > > > btw, events.js and head.js define both waitForEvent functions, which is > > > confusing a bit, it'd be good to have one shared version to avoid possible > > > conflicts > > > > Ah, didn't see that till now. The one in head.js is only used in the tests > > directly in the 'browser' dir (not the 'states' or 'e10s' subdirs), I think > > that should be revisited in another bug to remove one of those > > implementations. > > new bug for that then? > Yep! Bug 1367829. > > > > - // a: no traversed state > > > > - testStates("link_traversed", 0, 0, STATE_TRAVERSED); > > > > > > should we keep these in mochitests and move event queue part only to the > > > browser tests? > > > > I would like to see us rely less on eventQueue and use async/await where > > appropriate, it makes it easier to read tests. > > I think there are a lot of new features in JS that make writing and reading > > async tests simpler, and we should take advantage of them. For more > > complicated stuff, there may be need for eventQueue (or something similar), > > but for basic automation we don't need it. > > I'm good with this, however it doesn't mean the transition from mochitest to > browser tests in general, correct? And those particular tests I was talking > about are not about eventing. I'm not sure about that, we could look into having this supported in mochitests. I first saw this style in browser tests. > > > > > > or probably keep two separate versions (mochitest and browser > > > ones) of this test? > > > > Because non-e10s mode is going away, some mochitests (like this one) will > > break permanently. I think the browser ones should replace them, and we > > should delete the mochitest ones. Then there will be no confusion as to > > which test is canonical. > > I meant testStates() part, we have many mochitest in states folder, and if > we move some of them into browsers tests leaving others in mochitests, then > it's confusing. I think it would be confusing to have the identical (or similar) code in two places. We could move the common stuff down another directory to accessible/tests. For now, I'm happy with how Yura started doing this with common.js, where we pull it from the mochitests dir. > > > > at least until we have a certain plan about mochitest. > > > otherwise it may appear confusing that some tests are in mochitests and some > > > in browser tests, and it's not clear what is the guideline for adding new > > > tests. > > > > Yeah, I think we need clarity as to what we need to exercise and check for: > > 1. xpc API of content in same process? > > I would say that most tests should fall into this category. They don't > depend on the specificity of e10s a11y implementation and as a consequence > the logic under the hood is simpler, which means it's easier to locate and > fix the problem(regression). > > > 2. xpc API of child content in parent process? > > we should have some of them too, or - it'd be ideal - find a way how to run > same tests in 1 and 2 categories. Yup, ideally we would do both. 1 is similar to unit testing and closer to our implementation. 2 is what the user will actually experience (if the ipc layer breaks its bad). > > > .. and what form of test is good for that. Right now the browser tests do a > > pretty good job of that because they run both in e10s mode and non-e10s > > mode. The mochitests are only run in non-e10s mode. > > is my understanding correct that mochitests are still good for #1 category, > i.e. mochitest can be run in e10s mode in general, and all we need is to fix > bunch of failing tests? Correct, they can be run in e10s, but you would only be covering category #1. Right now, when you run a11y-mochitests (either locally or in CI), the test runner forces e10s off. Many tests don't handle the e10s case well, and often break. You can try that yourself with this patch: https://hg.mozilla.org/try/rev/0a8b99b8de59a58e27dbe7c8df31370be1efaa97 See bug 1337887 for more context. > > > I'm assuming non-e10s is > > going away altogether, which leaves us with the above dilemma. > > > > As for style, I am biased towards how our browser tests are written. They > > are more modern, readable, and easier for newcomers to understand. So, if it > > were up to me, I would say all new tests should be written as browser tests, > > and we should slowly migrate mochitests to browser tests. You may have a > > different opinion :) > > except eventQueue thing, are there other non-modern stuff specific to our > mochitests? I don't know if it's not possible in mochitests, but I like how yura set up most tests to be a js file with a small markup snippet for the test. Much less boilerplate. Also, the browser tests allow you to have both content and parent process javascript arranged in a linear fashion that is easy to read. For example, you can run a script in content to change an attribute on an element, and wait for the change event in the parent process: https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/e10s/browser_events_statechange.js#51-60 Such a test will work for both e10s and non-e10s. With e10s it will exercise the ipc layer, mochitests won't.

(In reply to Eitan Isaacson [:eeejay] from comment #9) > > Yup, ideally we would do both. 1 is similar to unit testing and closer to > our implementation. > 2 is what the user will actually experience (if the ipc layer breaks its > bad). To be clear, the browser tests do just this. Locally, you can do --disable-e10s and the results should be identical to e10s. In CI, browser tests are run both with and without e10s (this may change soon).

It seems we rolled down a bit from from the patch review to generic topics. Here's some summary of my thinking (please comment your concerns): * mochitests is a workable solution for in-tab testing * mochitests are good for unit testing (no extra ipc layers are involved) * mochitests allows to deal with static HTML which is different from DOM manipulations (for example, no mutation events) * a11y mochitests infrastructure is somewhat obsolete but can be fixed by promises and async functions - new JS features * browser tests may work well for both in-tab vs inter-process modes, but do not allow static HTML. * category #1 (in-tab testing) is a real unit testing (no ipc level involved and thus easier for regressions finding) * category #1 teststests themselves are simpler (no inter-process communications) * category #2 (in-chrome testing) technically makes us closer to AT API, but it keeps us further from AOM testing. Also it depends on a11y e10s architecture: if one day we'll make something closer to AOM in desktop APIs, then we'll need to rewrite the tests again. * it is expensive to convert all mochitest to browsers tests, and it seems there's no great advantage So I would keep mochitests for unit testing. I would use browser tests where mochitest don't work. Getting back to review, I would be happier (until we sorted out our testing infrastructure strategy) if you moved eventQueue part only to browser tests, leaving testStates() part in mochitests.

Comment on attachment 8870897 [details] [diff] [review] Convert states/test_link.html to browser test. r?surkov Review of attachment 8870897 [details] [diff] [review]: ----------------------------------------------------------------- if head.js spei ::: accessible/tests/mochitest/states/a11y.ini @@ -24,5 @@ > [test_editablebody.html] > [test_expandable.xul] > [test_frames.html] > [test_inputs.html] > -[test_link.html] as I commented, I would keep it

Yeah, we should find a better forum for this discussion :) (In reply to alexander :surkov from comment #11) > It seems we rolled down a bit from from the patch review to generic topics. > Here's some summary of my thinking (please comment your concerns): > * mochitests is a workable solution for in-tab testing True. This is possible in browser tests as well. We would need to load in some utility functions into the framescript to make it easier. > * mochitests are good for unit testing (no extra ipc layers are involved) If you run a browser test with --disable-e10s, it won't pass through the IPC layer either. > * mochitests allows to deal with static HTML which is different from DOM > manipulations (for example, no mutation events) True, but so do browser tests. The html snippets are passed as data uris. > * a11y mochitests infrastructure is somewhat obsolete but can be fixed by > promises and async functions - new JS features > True. Browser tests already have a set of modules that do basic browser automation so we don't have to worry about it. In mochitests, there is no easy way to manipulate an e10s-enabled browser (and we do that a lot). > * browser tests may work well for both in-tab vs inter-process modes, but do > not allow static HTML. They do. We either load snippets as data uris, or pass full html documents if that is needed. > > * category #1 (in-tab testing) is a real unit testing (no ipc level involved > and thus easier for regressions finding) But it also is farther removed from issues users will run into. > * category #1 teststests themselves are simpler (no inter-process > communications) Not sure I agree here. The ipc stuff does not get in the way of the test. > * category #2 (in-chrome testing) technically makes us closer to AT API, but > it keeps us further from AOM testing. Also it depends on a11y e10s > architecture: if one day we'll make something closer to AOM in desktop APIs, > then we'll need to rewrite the tests again. > How does this affect AOM testing? Not sure I understand. It doesn't depend on e10s at all. Not sure I understand this point. > * it is expensive to convert all mochitest to browsers tests, and it seems > there's no great advantage I would propose a gradual approach: 1. We port mochitests to browser tests only when necessary (eg. breaks in e10s mode). 2. All new tests should be written as browser tests

Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/679bf926eb5f Move common js stuff down in a11y broswer tests. r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad875285fec Move failing part of states/test_link.html to browser test. r=surkov

Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/100442388071 Fix eslint errors on a CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/679bf926eb5f https://hg.mozilla.org/mozilla-central/rev/1ad875285fec https://hg.mozilla.org/mozilla-central/rev/100442388071