Closed Bug 1258931 Opened 6 years ago Closed 6 years ago
--disable-accessibility makes it impossible to run reftests
STR: 1. Add "ac_add_options --disable-accessibility" to .mozconfig 2. rebuild 3. ./mach reftest layout/reftests/css-grid/reftest.list ACTUAL RESULTS reftests are not run EXPECTED RESULTS reftests are run This used to work until very recently. ADDITIONAL INFORMATION In the console output I see these error messages: 1458701394331 Marionette DEBUG Marionette enabled via command-line flag 1458701394754 Marionette ERROR Error on starting server: TypeError: Ci.nsIAccessibleStates is undefined TypeError: Ci.nsIAccessibleStates is undefined
Do you know if it's a regression from https://hg.mozilla.org/mozilla-central/rev/8f862d5425a3 ?
It's not, this is the changeset that broke it: https://hg.mozilla.org/mozilla-central/rev/06baa0fa499b
I'm not quite sure what the accessibility feature is, but certainly Marionette uses it at https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/accessibility.js#L41 to define states for accessibility checks. What would cause Ci.nsIAccessibleStates to be undefined? I can't see this being a regression from https://hg.mozilla.org/mozilla-central/rev/8f862d5425a3 as it used nsIAccessibleStates before that, but I'm not sure Marionette has ever been tested with --disable-accessibility. If it is a problem that Marionette uses nsIAccessibleStates we shold ni? :yzen who has done work on this.
I guess :ahal's patch tries to import Marionette in order to be able to install add-ons at runtime, which is needed in the world of signed add-ons. When --disable-accessibility is specified, Ci.nsIAccessibleStates becomes undefined and the import of Marionette fails because it uses the enum in testing/marionette/accessibility.js.
Yep. This means mochitests are also broken with --disable-accessibility (and bug 1251782 was filed for those). Reading comments over there it looks like it was tentatively WONTFIX'ed. So there are two options: 1) Make marionette work with --disable-accessibility (this might be hard, I'm not sure) 2) Provide a better error message and call it a day I guess it depends on why people want to test with --disable-accessibility in the first place, and how important that use case is. Mats, could you help us figure out your use case here? Why are you running with --disable-accessibility? (Not saying that doing so is wrong or bad, just want to understand)
Flags: needinfo?(ahalberstadt) → needinfo?(mats)
Mats Can you explain why you need to build with --disable-accessibility? Marionette does use accessibility features in it so it is required. With --disable-accessibility we move away from what the end product is built like. Before I see about handling this case I need to understand why you're using this build flag
(In reply to Andrew Halberstadt [:ahal] from comment #5) > 1) Make marionette work with --disable-accessibility (this might be hard, > I'm not sure) It’s not impossible, but I’d argue it’s a bad idea for the same reasons that #ifdefs often are harmful. A need to run the tests on custom builds imposes unnecessary maintenance burden. This is especially true if --disable-accessibility isn’t something we actually ship to users.
I and many other Gecko developers are using --disable-accessibility to reduce the time it takes to cycle through "write code -> recompile -> run tests". It's important for productivity that this cycle time is as low as possible. Turning off as many features as you possible can helps with that. The accessibility code base is a rather large piece that isn't necessary for what I'm working on (layout). http://mxr.mozilla.org/mozilla-central/source/accessible/ A secondary benefit is that it enforces layer boundaries between areas of code, so that we don't accidentally introduce layout code that depends on accessibility code for example. Granted, this part doesn't depend on reftests working.
I believe this is fixed with bug 1312816
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I've verified it works now. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.