1388062 - Refactor browser aria owns tests

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

The aria-owns tests are growing, I want to refactor this test with a few design goals in mind:

1. Isolate each case to a separate test that does not rely on the state from the previous test.
2. Make the tests highly readable.
3. Make failures easily locatable.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Refactor browser aria-owns tests. r?surkov r?yzen

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8894510 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov r?yzen

Review of attachment 8894510 [details] [diff] [review]:
-----------------------------------------------------------------

looks good , some questions below:

::: accessible/tests/browser/tree/browser_aria_owns.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +let NO_MOVE = { unexpected: [[EVENT_REORDER, "container"]] };

Are you 100% sure that when no expected events are passed we have a chance to capture any unexpected events? It looks like the promise that waits for expected is resolved right away and the unexpected events observer would be stopped right away?

::: accessible/tests/browser/tree/head.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +/* exported addAriaOwnsTest, contentSpawnAriaOwns, testChildrenIds */

These 2 functions (addAriaOwnsTest, contentSpawnAriaOwns) don't seem to belong in the head, as they are really specific to the aria owns test. Are they going to be used in other ones too?

@@ +37,5 @@
> + * the aria-owns relocations queue, so it gaurentees we won't have more reorder
> + * events after that one from the test's task. This allows us to check
> + * for unexpected moves without using a brittle timeout.
> + */
> +async function contentSpawnAriaOwns(browser, waitFor, func) {

contentSpawnAriaOwnsReorder?

@@ +50,5 @@
> +
> +  await ContentTask.spawn(browser, null, func);
> +
> +  if (waitForArgs.unexpected.length) {
> +    await ContentTask.spawn(browser, null, function() {

perhaps it's safer to queue this mutation after the func is completed for sure? I'm slightly concerned that we do it 2 separate ContentTask.spawn calls. Should we do this instead?

await ContentTask.spawn(browser, null, func).then(() => 
  waitForArgs.unexpected.length &&
  ContentTask.spawn(browser, null, function() {...}));

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8894510 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov r?yzen

Review of attachment 8894510 [details] [diff] [review]:
-----------------------------------------------------------------

I'm good with the approach if Yura feels that's the right way to do

::: accessible/tests/browser/tree/browser_aria_owns.js
@@ +7,5 @@
> +let NO_MOVE = { unexpected: [[EVENT_REORDER, "container"]] };
> +let MOVE = { expected: [[EVENT_REORDER, "container"]] };
> +
> +// Set last ordinal child as aria-owned, should produce no reorder.
> +addAriaOwnsTest(`<ul id="container"><li id="a">Test</li></ul>`,

nit: I would keep a markup on a new line for better redability

::: accessible/tests/browser/tree/head.js
@@ +56,5 @@
> +      if (container.hasAttribute("aria-owns")) {
> +        container.removeAttribute("aria-owns");
> +      } else {
> +        container.setAttribute("aria-owns", "_ao1");
> +      }

the approach feels hacky, because it makes the testing more complicated than it could be, I mean, since you trigger aria-owns mutations under the hood, then these operations are always part of any test, and you cannot simply test the simpler things. It's harder to debug too. I think I would prefer if we waited a timeout for unexpected events case rather than triggering other ones.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #2)
> Comment on attachment 8894510 [details] [diff] [review]
> Refactor browser aria-owns tests. r?surkov r?yzen
> 
> Review of attachment 8894510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good , some questions below:
> 
> ::: accessible/tests/browser/tree/browser_aria_owns.js
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +let NO_MOVE = { unexpected: [[EVENT_REORDER, "container"]] };
> 
> Are you 100% sure that when no expected events are passed we have a chance
> to capture any unexpected events? It looks like the promise that waits for
> expected is resolved right away and the unexpected events observer would be
> stopped right away?

Right. If there are no expected events, it would resolve immediately without catching potential unexpected events.

That is why we check for this in head.js/contentSpawnAriaOwns. If we have unexpected events, we tweak the aria-owns attribute  on #_ao-container in order to have a bookend event to wait for.

> 
> ::: accessible/tests/browser/tree/head.js
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  "use strict";
> >  
> > +/* exported addAriaOwnsTest, contentSpawnAriaOwns, testChildrenIds */
> 
> These 2 functions (addAriaOwnsTest, contentSpawnAriaOwns) don't seem to
> belong in the head, as they are really specific to the aria owns test. Are
> they going to be used in other ones too?

Thought it would be useful for more tests in the future. I could put it back in the test for now.

> 
> @@ +37,5 @@
> > + * the aria-owns relocations queue, so it gaurentees we won't have more reorder
> > + * events after that one from the test's task. This allows us to check
> > + * for unexpected moves without using a brittle timeout.
> > + */
> > +async function contentSpawnAriaOwns(browser, waitFor, func) {
> 
> contentSpawnAriaOwnsReorder?

Sounds good.

> 
> @@ +50,5 @@
> > +
> > +  await ContentTask.spawn(browser, null, func);
> > +
> > +  if (waitForArgs.unexpected.length) {
> > +    await ContentTask.spawn(browser, null, function() {
> 
> perhaps it's safer to queue this mutation after the func is completed for
> sure? I'm slightly concerned that we do it 2 separate ContentTask.spawn
> calls. Should we do this instead?
> 
> await ContentTask.spawn(browser, null, func).then(() => 
>   waitForArgs.unexpected.length &&
>   ContentTask.spawn(browser, null, function() {...}));

That is what 'await' does. It won't return until the task promise resolves.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 8894510 [details] [diff] [review]
> Refactor browser aria-owns tests. r?surkov r?yzen
> 
> Review of attachment 8894510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm good with the approach if Yura feels that's the right way to do
> 
> ::: accessible/tests/browser/tree/browser_aria_owns.js
> @@ +7,5 @@
> > +let NO_MOVE = { unexpected: [[EVENT_REORDER, "container"]] };
> > +let MOVE = { expected: [[EVENT_REORDER, "container"]] };
> > +
> > +// Set last ordinal child as aria-owned, should produce no reorder.
> > +addAriaOwnsTest(`<ul id="container"><li id="a">Test</li></ul>`,
> 
> nit: I would keep a markup on a new line for better redability
> 
> ::: accessible/tests/browser/tree/head.js
> @@ +56,5 @@
> > +      if (container.hasAttribute("aria-owns")) {
> > +        container.removeAttribute("aria-owns");
> > +      } else {
> > +        container.setAttribute("aria-owns", "_ao1");
> > +      }
> 
> the approach feels hacky, because it makes the testing more complicated than
> it could be, I mean, since you trigger aria-owns mutations under the hood,
> then these operations are always part of any test, and you cannot simply
> test the simpler things. It's harder to debug too. I think I would prefer if
> we waited a timeout for unexpected events case rather than triggering other
> ones.

It does add a layer of complexity to the test, but only in tests where no moves are expected.

Arbitrary timeouts are frowned upon in tests[1]. Furthermore, they add up quickly and lengthen test runs which translates to real infrastructure money spent. So, I think they should be avoided here.

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Avoiding_intermittent_oranges#Using_magical_timeouts_to_cause_delays

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> > 
> > @@ +50,5 @@
> > > +
> > > +  await ContentTask.spawn(browser, null, func);
> > > +
> > > +  if (waitForArgs.unexpected.length) {
> > > +    await ContentTask.spawn(browser, null, function() {
> > 
> > perhaps it's safer to queue this mutation after the func is completed for
> > sure? I'm slightly concerned that we do it 2 separate ContentTask.spawn
> > calls. Should we do this instead?
> > 
> > await ContentTask.spawn(browser, null, func).then(() => 
> >   waitForArgs.unexpected.length &&
> >   ContentTask.spawn(browser, null, function() {...}));
> 
> That is what 'await' does. It won't return until the task promise resolves.

duh, you're right :)

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

> > the approach feels hacky, because it makes the testing more complicated than
> > it could be, I mean, since you trigger aria-owns mutations under the hood,
> > then these operations are always part of any test, and you cannot simply
> > test the simpler things. It's harder to debug too. I think I would prefer if
> > we waited a timeout for unexpected events case rather than triggering other
> > ones.
> 
> It does add a layer of complexity to the test, but only in tests where no
> moves are expected.

true, but still, a layer of complexity for some tests

> Arbitrary timeouts are frowned upon in tests[1].

I wouldn't say these are arbitrary timeouts, it's rather a predefined timeout - one for all - just making sure that something doesn't happen.

> Furthermore, they add up
> quickly and lengthen test runs which translates to real infrastructure money
> spent. So, I think they should be avoided here.

If we save money in trade for the tests complexity, then we probably do something wrong.

Ehsan, I know you were looking into the tests timeouts issue in the past, you might have some ideas on the topic. How do people from other modules approach to design a test, where they need to ensure that something doesn't happen?

Comment 8

[(no longer active)]

It depends, it's usually best to try to avoid using timeouts where possible.  Sometimes there are ways to trigger the operation that may result in the outcome that you would like to test to make sure it won't happen and then somehow inspect the state of the system to ensure that the async result isn't queued.  There are also other places where this isn't an option, and instead we manually synthesize time passing (for example, see nsIDOMWindowUtils.advanceTimeAndRefresh()).  Using timeouts is usually acceptable as a last resort, since the timeout value may be insufficient to actually ensure that the asynchronous outcome doesn't happen, and these timeouts delay the end-to-end results of running tests.  They also do have an impact on the cost of running tests on the infra however I personally find the cost argument the least convincing reason against using timeouts of all.  :-)

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8894510 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov r?yzen

Review of attachment 8894510 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/browser/events.js
@@ +111,5 @@
>          }
>  
>          if (matchEvent(event, matchCriteria)) {
>            Logger.log(`Correct event type: ${eventTypeToString(eventType)}`);
> +          if (!(event.accessibleDocument instanceof nsIAccessibleDocument)) {

So we need to update our tests if we are making this change. Currently there are some tests that depend on this ok check because it's the only assertion that we make in the test for example - https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/e10s/browser_events_show.js

and actual failure https://treeherder.mozilla.org/logviewer.html#?job_id=122364575&repo=try&lineNumber=1917

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

See comment 9

Comment 11

[Eitan Isaacson [:eeejay]]

Gotcha. I'll update tests for that in the next round. Ehsan's suggestion of advanceTimeAndRefresh is interesting, I may try to emply that here too.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Refactor browser aria-owns tests. r?surkov r?yzen

This version removed the "queue flush" hack I had, and uses advanceTimeAndRefresh to explicitly flush our mutations queue in a document so we can know for certain that an unexpected mutation event doesn't occur.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 8897635 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov r?yzen

Review of attachment 8897635 [details] [diff] [review]:
-----------------------------------------------------------------

I'd say 0 should be enough if we were in a content process, 100 guarantee us nothing for interprocess communications. Indeed all we need is to trigger a drvier refresh, which will flush our events queue, but since we do the inprocess communications, then the solution has to be more complex: I don't know how we may ensure that all events from content were delivered to a parent, and whether a refresh driver is also in charge for that.

Another solution is to move the test completely into a content process, what makes advanceTimeAndRefresh(0) approach sufficient and elegant.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8897635 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov r?yzen

Review of attachment 8897635 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, couple of questions:

::: accessible/tests/browser/events.js
@@ +111,5 @@
>          }
>  
>          if (matchEvent(event, matchCriteria)) {
>            Logger.log(`Correct event type: ${eventTypeToString(eventType)}`);
> +          if (!(event.accessibleDocument instanceof nsIAccessibleDocument)) {

I think my earlier comment here still stands. There are tests that just wait for event (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/e10s/browser_events_show.js) and thus will fail because they now have 0 asserts.

::: accessible/tests/browser/tree/browser_aria_owns.js
@@ +15,5 @@
> +    testChildrenIds(containerAcc, ["a"]);
> +
> +    await contentSpawnMutation(browser, NO_MOVE, function() {
> +      // aria-own ordinal child in place, should be a no-op.
> +      document.getElementById("container").setAttribute("aria-owns", "a");

this works without content.document... (same in other places down the file)?

@@ +102,5 @@
> +
> +    let waitfor = { expected: [
> +      [EVENT_REORDER, "container"],
> +      [EVENT_REORDER,
> +        evt => getAccessibleDOMNodeID(evt.accessible.parent) == "select"]] };

would a check like [EVENT_REORDER, selectAcc.firstChild] work here? I think it would be slightly clearer? We could even take selectAcc.firstChild as a variable with a meaningful name.

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: Refactor browser aria-owns tests. r?surkov

I removed the event.accessibleDocument from events.js and put it in all the e10s/browser_events_*.js tests.

Comment 16

[alexander :surkov (:asurkov)]

Eitan, could you answer comment #13 please? I'd need to figure out whether you will adjust your patch to address the comment or it's preferable to have a follow up on this.

Comment 17

[Eitan Isaacson [:eeejay]]

Sorry, I missed this comment earlier.

(In reply to alexander :surkov from comment #13)
> Comment on attachment 8897635 [details] [diff] [review]
> Refactor browser aria-owns tests. r?surkov r?yzen
> 
> Review of attachment 8897635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd say 0 should be enough if we were in a content process, 100 guarantee us
> nothing for interprocess communications. Indeed all we need is to trigger a
> drvier refresh, which will flush our events queue, but since we do the
> inprocess communications, then the solution has to be more complex: I don't
> know how we may ensure that all events from content were delivered to a
> parent, and whether a refresh driver is also in charge for that.

Do you mean 0 or 100 milliseconds in the advance queue function? 100 Is just an arbitrary positive number. I thought it would be safer than 0 for other tick observers that rely on it. It has no use in our own a11y observer.

In process or out of process shouldn't matter. A spawned content function is composed of two messages, the first one contains a toString() version of the function, and it is eval()ed in the handler in content. After the function returns, the content sends a "done" message back to the chrome process.

When we call advanceTimeAndRefresh from the content function it dispatches any resulting mutation events immediately. The event IPC messages should always arrive and get dispatched before the spawn "done" message is received.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #17)

> When we call advanceTimeAndRefresh from the content function it dispatches
> any resulting mutation events immediately. The event IPC messages should
> always arrive and get dispatched before the spawn "done" message is received.

are you sure we are guaranteed that all a11y events sent from content process to parent process are delivered before swan is returned? If so then it's great and we can go with 0 timeout :)

Comment 19

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #18)
> (In reply to Eitan Isaacson [:eeejay] from comment #17)
> 
> > When we call advanceTimeAndRefresh from the content function it dispatches
> > any resulting mutation events immediately. The event IPC messages should
> > always arrive and get dispatched before the spawn "done" message is received.
> 
> are you sure we are guaranteed that all a11y events sent from content
> process to parent process are delivered before swan is returned? If so then
> it's great and we can go with 0 timeout :)

Pretty confident of this. Yes. What timeout?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #19)

> > are you sure we are guaranteed that all a11y events sent from content
> > process to parent process are delivered before swan is returned? If so then
> > it's great and we can go with 0 timeout :)
> 
> Pretty confident of this. Yes. What timeout?

not sure I caught your question. I meant that 0 ms timeout should work.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 8897916 [details] [diff] [review]
Refactor browser aria-owns tests. r?surkov

Review of attachment 8897916 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/browser/e10s/browser_events_caretmove.js
@@ +16,5 @@
>    let caretMovedEvent = event.QueryInterface(nsIAccessibleCaretMoveEvent);
>    is(caretMovedEvent.caretOffset, 5,
>      "Correct caret offset.");
> +  ok(caretMovedEvent.accessibleDocument instanceof nsIAccessibleDocument,
> +    "Accessible document not present.");

not sure if this one is important to keep, I think Yura's point was to add this check to tests that don't have any checks without this one.

::: accessible/tests/browser/e10s/browser_events_hide.js
@@ +29,5 @@
>        "Correct target next sibling.");
>      is(getAccessibleDOMNodeID(hideEvent.targetPrevSibling), "previous",
>        "Correct target previous sibling.");
> +    ok(hideEvent.accessibleDocument instanceof nsIAccessibleDocument,
> +      "Accessible document not present.");

same here

::: accessible/tests/browser/e10s/browser_events_statechange.js
@@ +15,5 @@
>    is(scEvent.isExtraState, isExtraState,
>      "Correct extra state bit of the statechange event.");
>    is(scEvent.isEnabled, isEnabled, "Correct state of statechange event state");
> +  ok(scEvent.accessibleDocument instanceof nsIAccessibleDocument,
> +    "Accessible document not present.");

and here

::: accessible/tests/browser/tree/head.js
@@ +36,5 @@
> +  let unexpectedListener = new UnexpectedEvents(waitFor.unexpected || []);
> +
> +  function tick() {
> +    content.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils).advanceTimeAndRefresh(100);

Could you please add a comment for that 100 number and get Ehsan thumb up on it? I don't really follow that advanceTime technique, I see the existing tests ranges it from 0 to thousands. This method apparently is more complex than we need, and thus feels complicated with me.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8827ea5e0a50
Refactor browser aria-owns tests. r=surkov r=yzen

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8827ea5e0a50