Closed Bug 1258931 Opened 6 years ago Closed 6 years ago

--disable-accessibility makes it impossible to run reftests

Categories

(Testing :: Reftest, defect)

defect
Not set
major

Tracking

(firefox48 affected)

VERIFIED FIXED
Tracking Status
firefox48 --- affected

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

Details

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 ?
Flags: needinfo?(mats)
It's not, this is the changeset that broke it:
https://hg.mozilla.org/mozilla-central/rev/06baa0fa499b
Blocks: 1245092
Flags: needinfo?(mats)
Flags: needinfo?(ahalberstadt)
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
Keywords: regression
See Also: → 1251782
(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.
Flags: needinfo?(mats)
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.