1459677 - Rework jsat content and text tests

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

Currently, they are implemented as a very long list of actions and expected results. This has several issues:

Problem:
1. Since we iterate over the list, we don't get an appropriate backtrace. This makes it very hard to pinpoint a failure.
2. The expected results we defined are abstracted for both b2g and Android
3. Only the b2g output contents were actually checked, in Android we just looked for event types.
4. The event queue management is complex and fragile, we still see some intermitents.

Solution:
1. Use JS promises and async syntax to simplify the test and make it more readable. This also offers proper stack traces.
2. Test for concrete Android events, and don't hide them in utility functions.
3. Test contents of Android events.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Promisify, simplify, and unabstract jsat content tests. r?yzen

Comment 2

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

(In reply to Eitan Isaacson [:eeejay] from comment #0)
> Currently, they are implemented as a very long list of actions and expected
> results. This has several issues:
> 
> Problem:
> 1. Since we iterate over the list, we don't get an appropriate backtrace.
> This makes it very hard to pinpoint a failure.
> 2. The expected results we defined are abstracted for both b2g and Android
> 3. Only the b2g output contents were actually checked, in Android we just
> looked for event types.
> 4. The event queue management is complex and fragile, we still see some
> intermitents.
> 
> Solution:
> 1. Use JS promises and async syntax to simplify the test and make it more
> readable. This also offers proper stack traces.
> 2. Test for concrete Android events, and don't hide them in utility
> functions.
> 3. Test contents of Android events.

Even before looking at the patch - \o/

Comment 3

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

Comment on attachment 8973760 [details] [diff] [review]
Promisify, simplify, and unabstract jsat content tests. r?yzen

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

Thanks for this work, I added comments, mostly nits and questions.

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +170,5 @@
>      });
>    }
>  };
>  
> +class AccessFuContentTest {

AccessFuContentTestRunner? and also in places where you have afct it was hard to figure out what it was until i found the declaration. Maybe then that name could be changed to runner or something similar.

@@ +188,4 @@
>  
>    finish() {
>      Logger.logLevel = Logger.INFO;
> +    this.listenersMap = new Map();

perhaps this.listenersMap.clear() would be more meaningful?

@@ +197,3 @@
>  
> +  sendMessage(message) {
> +    this.mms[0].sendAsyncMessage(message.name, message.json);

this.mms[0] could be a getter:
get messageManager {
  return this.mms[0];
}

@@ +198,5 @@
> +  sendMessage(message) {
> +    this.mms[0].sendAsyncMessage(message.name, message.json);
> +  }
> +
> +  androidEvent(eventType) {

I see what you're doing here. I was wondering, if you think we could alternatively use a toolkit/modules/EventEmitter.jsm to do the whole event listener storing bits. I realize that you are building queues here but maybe we can utilize its promise returning methods like once to queue things. No worries if not.

@@ +221,1 @@
>          var elem = content.document.querySelector(aMessage.json.selector);

Here and everywhere else, we should probably use "data" instead of deprecated "json"

@@ +231,5 @@
>  
> +    return new Promise(resolve => {
> +      aMessageManager.addMessageListener("AccessFu:Present", this);
> +      aMessageManager.addMessageListener("AccessFu:Ready", function() {
> +        aMessageManager.addMessageListener("AccessFu:ContentStarted", resolve);

This is a bit hard to read. Could we change this function to be async and then have these set up in order after the frame scripts are loaded. (The only one that will remain before is the ContentSarted listener that we also want to remove when resolve is called?).

@@ +252,4 @@
>        return;
>      }
>  
> +    for (let evt of aMessage.json) {

we should use "data" instead of deprecated "json"

@@ +283,2 @@
>  
> +  moveNext(aRule, ...aExpectedEvents) {

For all of these (moveNext, moveLast, ...) can we have a info(...) block so we know what step we are in (might help with figuring out failures down the road).

@@ +303,5 @@
>  
> +  async clearCursor() {
> +    return new Promise(resolve => {
> +      let _listener = (msg) => {
> +        this.mms.forEach(

this is a little confusing. Are we interested in mm[0] when we want to resolve? if so could you use messageManager getter I suggested above.

@@ +307,5 @@
> +        this.mms.forEach(
> +          mm => mm.removeMessageListener("AccessFu:CursorCleared", _listener));
> +        resolve();
> +      };
> +      this.mms.forEach(

Same here? If you are waiting for resolve in all mms then we should have individual promises for all of them, otherwise I'm worried about the intermittents.

::: accessible/tests/mochitest/jsat/test_content_integration.html
@@ +159,5 @@
> +      evt = await afct.moveLast("Simple",
> +        AndroidEvents.VIEW_ACCESSIBILITY_FOCUSED);
> +      afct.eventTextMatches(evt, ["much range", "4", "slider"]);
> +
> +      await afct.clearCursor();

I also wonder (perhaps not in this place in particular) but blocks where we clear cursor and start new sequences could be moved into their separate tests? Do we really benefit from having them all in the same file?

::: accessible/tests/mochitest/jsat/test_content_text.html
@@ +34,5 @@
> +        "These are my awards, Mother. From Army. " +
> +        "The seal is for marksmanship, and the gorilla is for sand racing."]);
> +
> +      // "These"
> +      evt = await afct.moveNextByGranularity(MovementGranularity.WORD,

these blocks look like a good function that you pass from and to indexes

@@ +102,5 @@
> +
> +      await afct.clearCursor();
> +    }
> +
> +    async function testEditableTextNavigation(doc, afct) {

Do you think we can split all of these up (the top level test functions) into their separate test files? It would help with keeping things more independent from each other.

@@ +126,5 @@
> +         "text area"]);
> +      is(evt[2].fromIndex, 0, "Caret at start (fromIndex)");
> +      is(evt[2].toIndex, 0, "Caret at start (toIndex)");
> +
> +      evt = await afct.activateCurrent(10,

This block and the ones below seem to be a good candidate to be taken out as a function.

@@ +252,5 @@
> +      afct.eventTextMatches(evt[1], ["Text content test document", "entry"]);
> +      is(evt[2].fromIndex, 0, "Caret at start (fromIndex)");
> +      is(evt[2].toIndex, 0, "Caret at start (toIndex)");
> +
> +      evt = await afct.typeKey("B",

Same here

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #3)
> Comment on attachment 8973760 [details] [diff] [review]
> Promisify, simplify, and unabstract jsat content tests. r?yzen
> 
> Review of attachment 8973760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for this work, I added comments, mostly nits and questions.
> 
> ::: accessible/tests/mochitest/jsat/jsatcommon.js
> @@ +170,5 @@
> >      });
> >    }
> >  };
> >  
> > +class AccessFuContentTest {
> 
> AccessFuContentTestRunner? and also in places where you have afct it was
> hard to figure out what it was until i found the declaration. Maybe then
> that name could be changed to runner or something similar.

Good idea, renamed to AccessFuContentTestRunner/runner.

> 
> @@ +188,4 @@
> >  
> >    finish() {
> >      Logger.logLevel = Logger.INFO;
> > +    this.listenersMap = new Map();
> 
> perhaps this.listenersMap.clear() would be more meaningful?

Oops, i was looking for that..

> 
> @@ +197,3 @@
> >  
> > +  sendMessage(message) {
> > +    this.mms[0].sendAsyncMessage(message.name, message.json);
> 
> this.mms[0] could be a getter:
> get messageManager {
>   return this.mms[0];
> }

Since we only use it once, I'm inclined to keep it as-is. I'll put a comment though to explain why mms[0] is so special.

> 
> @@ +198,5 @@
> > +  sendMessage(message) {
> > +    this.mms[0].sendAsyncMessage(message.name, message.json);
> > +  }
> > +
> > +  androidEvent(eventType) {
> 
> I see what you're doing here. I was wondering, if you think we could
> alternatively use a toolkit/modules/EventEmitter.jsm to do the whole event
> listener storing bits. I realize that you are building queues here but maybe
> we can utilize its promise returning methods like once to queue things. No
> worries if not.

The implementations are strikingly similar! The difference is that we only emit an event on the first listener we encounter. This is important so that each event only resolves one promise (eg. if we expect two consecutive CLICK events, the first event would resolve both promises in the EventEmitter implementation).

> 
> @@ +221,1 @@
> >          var elem = content.document.querySelector(aMessage.json.selector);
> 
> Here and everywhere else, we should probably use "data" instead of
> deprecated "json"

Wow, I didn't realize that's possible.. fixed.

> 
> @@ +231,5 @@
> >  
> > +    return new Promise(resolve => {
> > +      aMessageManager.addMessageListener("AccessFu:Present", this);
> > +      aMessageManager.addMessageListener("AccessFu:Ready", function() {
> > +        aMessageManager.addMessageListener("AccessFu:ContentStarted", resolve);
> 
> This is a bit hard to read. Could we change this function to be async and
> then have these set up in order after the frame scripts are loaded. (The
> only one that will remain before is the ContentSarted listener that we also
> want to remove when resolve is called?).

I'm not sure I understand your suggestion fully. I unfolded it to be an async function so I hope it's easier to read.

> 
> @@ +252,4 @@
> >        return;
> >      }
> >  
> > +    for (let evt of aMessage.json) {
> 
> we should use "data" instead of deprecated "json"

Done.

> 
> @@ +283,2 @@
> >  
> > +  moveNext(aRule, ...aExpectedEvents) {
> 
> For all of these (moveNext, moveLast, ...) can we have a info(...) block so
> we know what step we are in (might help with figuring out failures down the
> road).

Are you suggesting adding a string argument to the move* functions? We call them over 60 times, I don't know if I can come up with a unique name for each move. What is nice about this setup is that we get a backtrace to the specific line in the test file that failed, so we don't need to rely on strings. Having a static string (e.g. info("moveNext")) would be redundant because of the backtrace.

> 
> @@ +303,5 @@
> >  
> > +  async clearCursor() {
> > +    return new Promise(resolve => {
> > +      let _listener = (msg) => {
> > +        this.mms.forEach(
> 
> this is a little confusing. Are we interested in mm[0] when we want to
> resolve? if so could you use messageManager getter I suggested above.

No. Only the distant "leaf" frame will send a "CursorCleared", so we will only recieve it once, but we need to register a listener on each mm.

> 
> @@ +307,5 @@
> > +        this.mms.forEach(
> > +          mm => mm.removeMessageListener("AccessFu:CursorCleared", _listener));
> > +        resolve();
> > +      };
> > +      this.mms.forEach(
> 
> Same here? If you are waiting for resolve in all mms then we should have
> individual promises for all of them, otherwise I'm worried about the
> intermittents.

We should only get a since "CursorCleared" message from an undetermined mm.

> 
> ::: accessible/tests/mochitest/jsat/test_content_integration.html
> @@ +159,5 @@
> > +      evt = await afct.moveLast("Simple",
> > +        AndroidEvents.VIEW_ACCESSIBILITY_FOCUSED);
> > +      afct.eventTextMatches(evt, ["much range", "4", "slider"]);
> > +
> > +      await afct.clearCursor();
> 
> I also wonder (perhaps not in this place in particular) but blocks where we
> clear cursor and start new sequences could be moved into their separate
> tests? Do we really benefit from having them all in the same file?

I'll take up your suggestion with the text tests because there is a good separation of functionality there. It will be a bit more work to unravel the content integration test and break it up. Might be a worthwhile spinoff bug.

> 
> ::: accessible/tests/mochitest/jsat/test_content_text.html
> @@ +34,5 @@
> > +        "These are my awards, Mother. From Army. " +
> > +        "The seal is for marksmanship, and the gorilla is for sand racing."]);
> > +
> > +      // "These"
> > +      evt = await afct.moveNextByGranularity(MovementGranularity.WORD,
> 
> these blocks look like a good function that you pass from and to indexes

Good idea. I'm explicitly only making function for the checking bits. I want to keep all the expected android events inline.

> 
> @@ +102,5 @@
> > +
> > +      await afct.clearCursor();
> > +    }
> > +
> > +    async function testEditableTextNavigation(doc, afct) {
> 
> Do you think we can split all of these up (the top level test functions)
> into their separate test files? It would help with keeping things more
> independent from each other.

Yup, I'll do that.

> 
> @@ +126,5 @@
> > +         "text area"]);
> > +      is(evt[2].fromIndex, 0, "Caret at start (fromIndex)");
> > +      is(evt[2].toIndex, 0, "Caret at start (toIndex)");
> > +
> > +      evt = await afct.activateCurrent(10,
> 
> This block and the ones below seem to be a good candidate to be taken out as
> a function.

As I said above, I think the checking bits are worth streamlining, don't want to move actual actions and expected event types.

> 
> @@ +252,5 @@
> > +      afct.eventTextMatches(evt[1], ["Text content test document", "entry"]);
> > +      is(evt[2].fromIndex, 0, "Caret at start (fromIndex)");
> > +      is(evt[2].toIndex, 0, "Caret at start (toIndex)");
> > +
> > +      evt = await afct.typeKey("B",
> 
> Same here

Ditto.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Promisify, simplify, and unabstract jsat content tests. r?yzen

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d30e3acaf57e45f5a450ef06ecf460ceffa386

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Promisify, simplify, and unabstract jsat content tests.

Try loves it! except for some eslint issues..

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7b6fcf919a
Promisify, simplify, and unabstract jsat content tests. r=yzen

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/be7b6fcf919a