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

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
4 years ago
Last year

People

(Reporter: yzen, Unassigned)

Tracking

({leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 4 obsolete attachments)

46 bytes, text/x-github-pull-request
gaye
: review+
Details | Review
46 bytes, text/x-github-pull-request
gaye
: review+
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
gaye
: review+
Details | Review
46 bytes, text/x-github-pull-request
gaye
: review+
Details | Review
46 bytes, text/x-github-pull-request
yzen
: review+
Details | Review
46 bytes, text/x-github-pull-request
yzen
: review+
Details | Review
46 bytes, text/x-github-pull-request
aus
: review+
Details | Review
Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Status: NEW → ASSIGNED
Reporter

Comment 2

4 years ago
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.
Attachment #8591897 - Flags: review?(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
Flags: needinfo?(jlorenzo)
Flags: needinfo?(gmealer)
Attachment #8591897 - Flags: review?(bebe)
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.
Flags: needinfo?(jlorenzo)
Flags: needinfo?(gmealer)
Attachment #8591897 - Flags: review?(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.
Attachment #8591897 - Flags: review?(etienne) → review?(kgrandon)
Reporter

Comment 6

4 years ago
(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 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?
Attachment #8591897 - Flags: review?(kgrandon)
Reporter

Comment 8

4 years ago
(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 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.
Attachment #8591897 - Flags: review?(gaye)
Reporter

Updated

4 years ago
Keywords: leave-open
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).
Flags: needinfo?(yzenevich)
Reporter

Comment 11

4 years ago
(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.
Flags: needinfo?(yzenevich)
Reporter

Comment 12

4 years ago
Posted file Marionette-js-runner pull request (obsolete) —
I will follow up with Gaia PR that updates to the new runner API.
Attachment #8606499 - Flags: review?(gaye)
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?
Reporter

Updated

4 years ago
Attachment #8606499 - Attachment is obsolete: true
Attachment #8606499 - Flags: review?(gaye)
Reporter

Updated

4 years ago
Attachment #8607691 - Flags: review?(gaye)
Reporter

Comment 15

4 years ago
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
Flags: needinfo?(gaye)
(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!
Flags: needinfo?(gaye)
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?
Attachment #8607691 - Flags: review?(gaye) → review+
Reporter

Updated

4 years ago
Attachment #8591897 - Attachment is obsolete: true
Attachment #8591897 - Flags: review?(gaye)
Reporter

Comment 19

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

This is the second part with actual enabled a11y checks.
Attachment #8608756 - Flags: review?(gaye)
r=self bumping gaia's marionette-js-runner version to 1.1.0 which includes yzen's changes to marionette.client()
Attachment #8609312 - Flags: review+
Comment on attachment 8609322 [details] [review]
[gaia] gaye:yzen-bug-1154035 > mozilla-b2g:master

r=gaye
Attachment #8609322 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/db93a2264a4ed0092340ccd6892237c6b32cb96b part 2 which turns on some accessibility checks landed on master. Nice work yzen!
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Reporter

Comment 26

4 years ago
Sorry, I just want to keep it open as I am going to start fixing/enabling more Gij tests.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #8608756 - Flags: review?(gaye) → review+
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
Reporter

Comment 28

4 years ago
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
Reporter

Comment 30

4 years ago
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.
Flags: needinfo?(gaye)
Reporter

Comment 33

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

Carrying over r+ from Gareth.
Attachment #8612982 - Flags: review+
Reporter

Comment 35

4 years ago
Resolved in IRC, removing ni?
Flags: needinfo?(gaye)
Reporter

Updated

4 years ago
Depends on: 1174314
Reporter

Comment 36

4 years ago
Posted file Github pull request. (obsolete) —
All remaning passing Gij with a11y checks enabled.
Attachment #8623776 - Flags: review?(gaye)
Reporter

Comment 37

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

Comment 38

4 years ago
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.
Flags: needinfo?(gaye)
Comment on attachment 8623776 [details] [review]
Github pull request.

LGTM! Nice work :)
Flags: needinfo?(gaye)
Attachment #8623776 - Flags: review?(gaye) → review+
Reporter

Comment 41

4 years ago
Carrying over gaye's r+, some tests seem to be failing for vertical homescreen so disabling them.
Attachment #8627854 - Flags: review+
Reporter

Comment 44

4 years ago
(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.
Reporter

Comment 45

4 years ago
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+
Attachment #8623776 - Attachment is obsolete: true
Attachment #8627854 - Attachment is obsolete: true
Attachment #8628058 - Flags: review+
Hey Yura, can you send a mail to dev-gaia (or fxos) about this? I'd like to understand the consequences :) Thanks !
Reporter

Comment 48

4 years ago
Marking ni to do it asap
Flags: needinfo?(yzenevich)
Reporter

Comment 51

4 years ago
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.
Attachment #8694240 - Flags: review?(aus)
Comment on attachment 8694240 [details] [review]
[gaia] yzen:bug-1154035 > mozilla-b2g:master

Yes! Awesome! =)
Attachment #8694240 - Flags: review?(aus) → review+
Reporter

Updated

3 years ago
Assignee: yzenevich → nobody
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 4 years agoLast year
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.