1154035 - [Accessibility] Enable accessibility checks for marionette integration tests (GIJ).

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

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

There are only a few tests that are passing ATM, they need to be enabled asap. Next step will be enabling tests one by one, that might require fixes to the apps themselves, not just tests.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1154035 > mozilla-b2g:master

Comment 2

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

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Hi Florin, let me know if this is OK for you to review, or perhaps you know someone else who I can redirect to.

Comment 3

[Florin Strugariu [:Bebe]]

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Not sure that I'm the right person to review you pull yzen
Let's ask Geo or Johan

Comment 4

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Sorry, we don't own these tests :s

Asking Etienne for review.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Forwarding to Kevin who might know if there's a more "central" place to set this up.

Comment 6

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

(In reply to Etienne Segonzac (:etienne) from comment #5)
> Comment on attachment 8591897 [details] [review]
> [gaia] yzen:bug-1154035 > mozilla-b2g:master
> 
> Forwarding to Kevin who might know if there's a more "central" place to set
> this up.

FYI the central place would be running the tests with that a11y checks capability enabled for all tests (at the time of calling make with --capabilities capabilities.json arg). However only a few tests pass at the moment and it will be a very delicate and incremental task to make the rest of them pass.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

I don't think we need a central spot for this then, but I'm really not a fan of the syntax. Normally I would R- this, but I don't have much context here.

Particularly, I don't like the use of `undefined, {Object}`, when the first param to marionette.client is already an object. How come we don't pass raisesAccessibilityExceptions into the first object? Do you know where/when this was implemented in the client, and do we have other tests doing this?

Comment 8

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

(In reply to Kevin Grandon :kgrandon from comment #7)
> Comment on attachment 8591897 [details] [review]
> [gaia] yzen:bug-1154035 > mozilla-b2g:master
> 
> I don't think we need a central spot for this then, but I'm really not a fan
> of the syntax. Normally I would R- this, but I don't have much context here.
> 
> Particularly, I don't like the use of `undefined, {Object}`, when the first
> param to marionette.client is already an object. How come we don't pass
> raisesAccessibilityExceptions into the first object? Do you know where/when
> this was implemented in the client, and do we have other tests doing this?

Well the first argument is the host profile and is not related to marionette client. There already was an optional second argument, so when implementing capabilities support with js-runner/js-client, we added a 3rd, marionette client specific argument for that. It would not necessary if all tests ran with the check, but at this point it's not possible because of the check failures elsewhere

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8591897 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

I see, thanks for the info. I'm personally not a fan of the constructor, and would opt to change it into a single options object, or maybe using a smarter splat object to receive arguments.

I'm forwarding this review to Gareth/James as owners of the testing framework - if they're happy with it, then I'm sure it's fine to land. Thanks.

Comment 10

[Gareth Aye [:gaye] (back from PTO)]

I think that I'm with :kgrandon here that I would prefer marionette.client() to take a single options object like:

```js
marionette.client({
  driver: Marionette.Drivers.Tcp,  
  profile: {
  },
  desiredCapabilities: {
    raisesAccessibilityExceptions: true    
  }
});
```

What do you think about doing that and then converting existing tests that do profile configuration to look like marionette.client({ profile: { ... } }) instead of marionette.client({ ... })? We could also make it so that both invocation styles work for the time-being but deprecate the one where the profile config is the first argument.

I also think that a blacklist is better than a whitelist here if we care about future tests paying attention to accessibility errors (perhaps ignoreAccessibilityExceptions).

Comment 11

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

(In reply to Gareth Aye [:gaye] from comment #10)
> I think that I'm with :kgrandon here that I would prefer marionette.client()
> to take a single options object like:
> 
> ```js
> marionette.client({
>   driver: Marionette.Drivers.Tcp,  
>   profile: {
>   },
>   desiredCapabilities: {
>     raisesAccessibilityExceptions: true    
>   }
> });
> ```
> 
> What do you think about doing that and then converting existing tests that
> do profile configuration to look like marionette.client({ profile: { ... }
> }) instead of marionette.client({ ... })? We could also make it so that both
> invocation styles work for the time-being but deprecate the one where the
> profile config is the first argument.

No objections to that, I'll take a look at that.

> 
> I also think that a blacklist is better than a whitelist here if we care
> about future tests paying attention to accessibility errors (perhaps
> ignoreAccessibilityExceptions).

I agree, but marionette is used across numerous project outside of mozilla and this approach will not be acceptable there.

Comment 12

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

Attachment: Marionette-js-runner pull request

I will follow up with Gaia PR that updates to the new runner API.

Comment 13

[Gareth Aye [:gaye] (back from PTO)]

Hey yzen. I don't know if you saw my recent post to dev-gaia but marionette-js-runner development is moving here https://github.com/mozilla-b2g/gaia/tree/master/tests/jsmarionette/runner/marionette-js-runner. Would you mind moving your pr over?

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1154035-1 > mozilla-b2g:master

Comment 15

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

Hi Gareth, A couple of questions:

* I made a PR with changes to both marionette-js-runner and Gij tests in Gaia repo, would you want me to make 2 separate ones for that?
* I noticed that package version in gaia/tree/master/tests/jsmarionette/runner/marionette-js-runner is 1.0.16 whilst the gaia/package.json has 1.0.17, I'm trying to find where it's being published from?
* I also noticed that my Gij tests fail on treeherder, but pass if I update the node_modules/marionette-js-runner to the new API. Does it mean that the process it to:
** make changes to gaia/tree/master/tests/jsmarionette/runner/marionette-js-runner
** bump up the version of marionette-js-runner based on the changes and publish to npm
** update gaia/package.json

Comment 16

[Gareth Aye [:gaye] (back from PTO)]

(In reply to Yura Zenevich [:yzen] from comment #15)
> Hi Gareth, A couple of questions:
> 
> * I made a PR with changes to both marionette-js-runner and Gij tests in
> Gaia repo, would you want me to make 2 separate ones for that?

One pr is fine!

> * I noticed that package version in
> gaia/tree/master/tests/jsmarionette/runner/marionette-js-runner is 1.0.16
> whilst the gaia/package.json has 1.0.17, I'm trying to find where it's being
> published from?

Yeah somewhat unfortunately a patch got published from mozilla-b2g/marionette-js-runner after I moved it into gaia but before I closed it for development. That one got copied over here https://github.com/mozilla-b2g/gaia/commit/3c144782491f0ec90c3536699da65191ff5faa6a. I think they didn't copy the package version bit though.

> * I also noticed that my Gij tests fail on treeherder, but pass if I update
> the node_modules/marionette-js-runner to the new API. Does it mean that the
> process it to:
> ** make changes to
> gaia/tree/master/tests/jsmarionette/runner/marionette-js-runner
> ** bump up the version of marionette-js-runner based on the changes and
> publish to npm
> ** update gaia/package.json

Yeah I think that's what we'll have to do for now. I'm hoping to find time in the next couple weeks to do https://bugzilla.mozilla.org/show_bug.cgi?id=1165488 which will make it so that gaia uses the in-tree marionete-js-runner instead of one from npm.

Thanks for your patience!

Comment 17

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8607691 [details] [review]
[gaia] yzen:bug-1154035-1 > mozilla-b2g:master

LGTM. I'll try to find time to land later today. I think you might have some conflicts with master though?

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1154035-2 > mozilla-b2g:master

Comment 19

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

Comment on attachment 8608756 [details] [review]
[gaia] yzen:bug-1154035-2 > mozilla-b2g:master

This is the second part with actual enabled a11y checks.

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] gaye:marionette-js-runner@1.1.0 > mozilla-b2g:master

Comment 21

[Gareth Aye [:gaye] (back from PTO)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/30196

r=self bumping gaia's marionette-js-runner version to 1.1.0 which includes yzen's changes to marionette.client()

Comment 22

[Gareth Aye [:gaye] (back from PTO)]

yzen patch part 1 https://github.com/mozilla-b2g/gaia/commit/f38268ae9921ab7af381b6ad45d37634a199edfe and https://github.com/mozilla-b2g/gaia/commit/f4486f7c7785b66e8dbfc775677d1b6386718e96 marionette-js-runner@1.1.0 landed on gaia master

Comment 23

[GitHub Autolander Bot]

Attachment: [gaia] gaye:yzen-bug-1154035 > mozilla-b2g:master

Comment 24

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8609322 [details] [review]
[gaia] gaye:yzen-bug-1154035 > mozilla-b2g:master

r=gaye

Comment 25

[Gareth Aye [:gaye] (back from PTO)]

https://github.com/mozilla-b2g/gaia/commit/db93a2264a4ed0092340ccd6892237c6b32cb96b part 2 which turns on some accessibility checks landed on master. Nice work yzen!

Comment 26

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

Sorry, I just want to keep it open as I am going to start fixing/enabling more Gij tests.

Comment 27

[Gareth Aye [:gaye] (back from PTO)]

So I went ahead and backed out the second patch which turned on the accessibility checks because we saw a big uptick in the number of intermittently failing tests on ci

https://github.com/mozilla-b2g/gaia/commit/361fb984ad68d1b5aa69702bca9cc7ea2f2d710e

Comment 28

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

The following tests seemed to have issues:

apps/system/test/marionette/statusbar_icon_prioritization_test.js
apps/system/test/marionette/software_home_notification_banner_test.js 
apps/system/test/marionette/software_home_app_crash_dialog_layout_test.js
apps/system/test/marionette/software_home_permission_prompt_test.js
apps/system/test/marionette/software_home_button_homescreen_test.js

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1154035 > mozilla-b2g:master

Comment 30

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

Hey Gareth, 

Perhaps, if you have a moment you could give me some pointers as to what the issue with the intermittents might be for the test suites I played with. I tried re-running them (https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=5541d6ff7895a3d7625f0c5241daab4066de5dc1) and the failure rate is noticeable smaller but the logs aren't super helpful in figuring out what might be going wrong.

Comment 31

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

another test job: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=48f28a567307a2fe8b5cb9b12c4b590cdf8bb387

Comment 32

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dc2fee9e08c

Comment 33

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

Comment on attachment 8612982 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Carrying over r+ from Gareth.

Comment 34

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

https://github.com/mozilla-b2g/gaia/commit/45ad4f643960a7424e3c7b65b1da7c9b7f5cdc77

Comment 35

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

Resolved in IRC, removing ni?

Comment 36

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

Attachment: Github pull request.

All remaning passing Gij with a11y checks enabled.

Comment 37

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

Figured out the set of tests that are good with a11y checks: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=19c64a74b47188f75a1134673c95a40f2244bb99

Comment 38

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

Hi Gareth, would you have a sec to take a look at the changes I made, these tests seem to work great with a11y checks on.

Comment 39

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8623776 [details] [review]
Github pull request.

LGTM! Nice work :)

Comment 40

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

https://github.com/mozilla-b2g/gaia/commit/eeb7820fc40314274cfaa819696756b444bee136

Comment 41

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

Attachment: Same as https://github.com/mozilla-b2g/gaia/pull/30629 with a couple of tests disabled

Carrying over gaye's r+, some tests seem to be failing for vertical homescreen so disabling them.

Comment 42

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

https://github.com/mozilla-b2g/gaia/commit/356bb26c248e178057cd556c8dd3b988ddb4e8d8

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Backed out for various Gij failures on b2g-inbound. Chunking issues maybe?
https://treeherder.mozilla.org/logviewer.html#?job_id=2230777&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=2230701&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=2230648&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=2230482&repo=b2g-inbound

https://github.com/mozilla-b2g/gaia/commit/b060888b424d64a376b4a787b39102d65e0aaf3e

Comment 44

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> Backed out for various Gij failures on b2g-inbound. Chunking issues maybe?
> https://treeherder.mozilla.org/logviewer.html#?job_id=2230777&repo=b2g-
> inbound
> https://treeherder.mozilla.org/logviewer.html#?job_id=2230701&repo=b2g-
> inbound
> https://treeherder.mozilla.org/logviewer.html#?job_id=2230648&repo=b2g-
> inbound
> https://treeherder.mozilla.org/logviewer.html#?job_id=2230482&repo=b2g-
> inbound
> 
> https://github.com/mozilla-b2g/gaia/commit/
> b060888b424d64a376b4a787b39102d65e0aaf3e

This was backed out and re-landed with failing tests reverted to the original ones. I ll wait until treehereder is green again and re-land.

Comment 45

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

Attachment: Github pull request v3.

Exactly the same as https://github.com/mozilla-b2g/gaia/pull/30759, was backed out after it was landed because of the previous version that was already backed out.
Carrying over r+

Comment 46

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

v3: https://github.com/mozilla-b2g/gaia/commit/bb16111ddbdfc2ae85daa471d46164384c07785b

Comment 47

[Julien Wajsberg [:julienw]]

Hey Yura, can you send a mail to dev-gaia (or fxos) about this? I'd like to understand the consequences :) Thanks !

Comment 48

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

Marking ni to do it asap

Comment 49

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

https://groups.google.com/forum/#!topic/mozilla.dev.gaia/LRNWvz4baqU

Comment 50

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1154035 > mozilla-b2g:master

Comment 51

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

Comment on attachment 8694240 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Making the a11y checks capability true by default. So the test author would have to explicitly set it to false to not check a11y.

Comment 52

[Aus Lacroix [:aus]]

Comment on attachment 8694240 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Yes! Awesome! =)

Comment 53

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

Thanks

merged: https://github.com/mozilla-b2g/gaia/commit/909ca0127bbe373442fa9a5646c821157c8b3c79

Comment 54

[BMO Automation]

Firefox OS is not being worked on