Treeherder Bug Filer Reporter
	Description • 7 years ago
treeherder

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=79124039&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64-debug/1487695049/autoland_yosemite_r7-debug_test-marionette-bm108-tests1-macosx-build293.txt.gz

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago

The newly introduced Marionette headless tests on Linux caused this failure to appear for nearly each commit now. Maybe there is something display related which we have to get fixed for the test, or something is broken in Marionette. Btw, it's the only test in headless mode which fails.

I will come up with a skip patch, so that we temporarily disable this test on Linux. But it would be good if someone with Linux could have a look at this.

Andreas, could you have a look? Looks like Yura is still out for a while.

	Pulsebot
	Comment 9 • 7 years ago

Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af622640b17f
"Intermittent test_accessibility.py TestAccessibility.test_element_visible_but_not_visible_to_accessbility | AssertionError: ElementNotAccessibleException not raised" []. r=me

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #7)
> The newly introduced Marionette headless tests on Linux caused this failure
> to appear for nearly each commit now. Maybe there is something display
> related which we have to get fixed for the test, or something is broken in
> Marionette. Btw, it's the only test in headless mode which fails.

Why are we running the Mn job with MOZ_HEADLESS enabled?  I would not consider that production ready.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 7 years ago

Why not? Everything else passes. The MnH jobs are totally green on inbound. Also David gave his r+ to everything.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Why not? Everything else passes. The MnH jobs are totally green on inbound.
> Also David gave his r+ to everything.

That it doesn’t work with accessibility features is an indication it’s not production ready.

I am also somewhat skeptical that cutting out the widget backends and replacing them with no-op alternatives, which is what MOZ_HEADLESS does, is a good idea for our user interaction tests.  By indirection, a bug caused by i.e. GTK should be considered a real bug and Mn is currently the only harness that would catch that.

In any case, I don’t have time or the competence to look into accessibility.  The accessibility tests does not make for great reading, but you can get a pinpoint of which element type is failing the displayed check by looking at the predefined lists near the top of test_accessibility.py.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago
bugherder

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 14 • 7 years ago

David, we would also need your input on comment 12. Do we want to keep MnH tests running or disable them alltogether. I don't have the necessary background for that.

	David Burns :automatedtester
	Comment 15 • 7 years ago

I gave r+ to having them run but my understanding is that they would be tier2 or tier3 until they were stable enough. bdahl was going to be working with the relative teams.

	David Burns :automatedtester
	Comment 16 • 7 years ago

:bdahl, did I misunderstand this work?

	Brendan Dahl [:bdahl] Assignee
	Comment 17 • 7 years ago

I'll take a look at the test. I haven't seen this fail on try runs or locally yet though.

(In reply to David Burns :automatedtester from comment #15)
> I gave r+ to having them run but my understanding is that they would be
> tier2 or tier3 until they were stable enough. bdahl was going to be working
> with the relative teams.

I misunderstood that things default to tier 1. I'll bump the tier down until it's a little more stable.

(In reply to Andreas Tolfsen ‹:ato› from comment #12)
> That it doesn’t work with accessibility features is an indication it’s not
> production ready.

Being in nightly does not imply production ready, but perhaps you mean by being tier1?

> I am also somewhat skeptical that cutting out the widget backends and
> replacing them with no-op alternatives, which is what MOZ_HEADLESS does, is
> a good idea for our user interaction tests.  By indirection, a bug caused by
> i.e. GTK should be considered a real bug and Mn is currently the only
> harness that would catch that.

We don't plan to replace Mn running, headless will be an alternative.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago

(In reply to Brendan Dahl [:bdahl] from comment #17)
> > I am also somewhat skeptical that cutting out the widget backends and
> > replacing them with no-op alternatives, which is what MOZ_HEADLESS does, is
> > a good idea for our user interaction tests.  By indirection, a bug caused by
> > i.e. GTK should be considered a real bug and Mn is currently the only
> > harness that would catch that.
> 
> We don't plan to replace Mn running, headless will be an alternative.

Just to be absolutely clear, the ability to run Firefox headlessly is marvelous.

I’m sorry I misunderstood that this was a separate job from Mn.  It absolutely makes sense to introduce an Mn-headless job to run in parallel.

	Brendan Dahl [:bdahl] Assignee
	Comment 19 • 7 years ago

I'm able to reproduce the bug consistently in a local optimized build by running the the failing test in a loop. From there it was pretty easy to track down:

GeckoDriver.isElementDisplayed calls
listener.isElementDisplayed() calls
interactions.isElementDisplayed(el, strict = true /* true because capabilities.get("moz:accessibilityChecks") is set */) calls
accessibility.getAccessible(element, mustHaveAccessible = false /* the default value */)

mustHaveAccessible defaults to false and it is not set to true by isElementDisplayed, so this causes us to only call accessibility.service.getAccessibleFor once which appears to be wrong in this case. It seems if moz:accessibilityChecks is true we should set mustHaveAccessible to true. The test works correctly if I pass on the `strict` value from isElementDisplayed to getAccessible, but unfortunately test_element_is_visible_to_accessibility then fails because it does not have accessible object (this seems like a bug). I'll keep looking to now see why test_element_is_visible_to_accessibility doesn't have an accessible object.


http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/testing/marionette/accessibility.js#142

	Brendan Dahl [:bdahl] Assignee
	Comment 20 • 7 years ago

test_element_is_visible_to_accessibility looks for the accessible object of the element no_accessible_but_displayed (which obviously doesn't have one), so we can't mustHaveAccessible=true for that test. Seems we may need a mode to wait for an accessible object but not require it.

	Andreas Tolfsen ❲:ato❳
	Comment 21 • 7 years ago

FWIW accessibility testing is not an integral part of providing WebDriver support.  It is part of testing the internal accessibility features of Firefox.  I suggest we loop in someone who knows something about accessibility in this discussion.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #21)
> FWIW accessibility testing is not an integral part of providing WebDriver
> support.  It is part of testing the internal accessibility features of
> Firefox.  I suggest we loop in someone who knows something about
> accessibility in this discussion.

Hi David, is there anyone from the accessibility team who could help us with this topic from above? Thanks.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 24 • 7 years ago

Let's find out. NI'ing Eitan.

	Eitan Isaacson [:eeejay]
	Comment 25 • 7 years ago

(In reply to Brendan Dahl [:bdahl] from comment #20)
> test_element_is_visible_to_accessibility looks for the accessible object of
> the element no_accessible_but_displayed (which obviously doesn't have one),
> so we can't mustHaveAccessible=true for that test. Seems we may need a mode
> to wait for an accessible object but not require it.

This test passes when an exception is thrown. For an exception to be thrown we need mustHaveAccessible=true.

I tried reproducing this locally (linux64 optimized build) with no luck. I think having this skipped for now is not too bad.

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 7 years ago

Did you run it with MOZ_HEADLESS=1?

	Eitan Isaacson [:eeejay]
	Comment 27 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #26)
> Did you run it with MOZ_HEADLESS=1?

I reverted the skip patch above and used the --headless mach option. Is the nev variable different?

	Brendan Dahl [:bdahl] Assignee
	Comment 28 • 7 years ago

> Is the nev variable different?
They're basically the same, --headless is preferred.


FWIW, I had started looking more into fixing this before the comment saying it's not an integral part of web driver. I really think accessibility.getAccessible() should be re-written to not use polling to check if the accessible object is ready. Instead it should look at the state of the document[1] and if it's not eCompletelyLoaded then add a listener for one of the AccEvents and wait for that to call getAccessibleFor.

[1] = http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/accessible/generic/DocAccessible.h#141



For reproducing I was running:
    def test_element_visible_but_not_visible_to_accessbility(self):
        for x in range(0, 1000):
            self.setup_accessibility()
            for id in self.displayed_but_a11y_hidden_elementIDs:
                element = self.marionette.find_element(By.ID, id)
                errored = False
                try:
                    element.is_displayed()
                except:
                    errored = True
                if not errored:
                    raise Exception('DIE!')
            print "PASSED " + str(x)

	Eitan Isaacson [:eeejay]
	Comment 29 • 7 years ago

(In reply to Brendan Dahl [:bdahl] from comment #28)
> > Is the nev variable different?
> They're basically the same, --headless is preferred.
> 
> 
> FWIW, I had started looking more into fixing this before the comment saying
> it's not an integral part of web driver. I really think
> accessibility.getAccessible() should be re-written to not use polling to
> check if the accessible object is ready. Instead it should look at the state
> of the document[1] and if it's not eCompletelyLoaded then add a listener for
> one of the AccEvents and wait for that to call getAccessibleFor.
> 

It's been a while since I visited this domain. I think the issue was that the accessibles are created opportunistically with no notification, so we had to poll. When you want to assert that there is *no* accessible it gets awkward, so I would love to see the polling go away.

Looks like you are looking into this, so that is great!

	Andreas Tolfsen ❲:ato❳
	Comment 30 • 7 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #27)
> (In reply to Andreas Tolfsen ‹:ato› from comment #26)
> > Did you run it with MOZ_HEADLESS=1?
> 
> I reverted the skip patch above and used the --headless mach option. Is the
> nev variable different?

Using the flag probably sets MOZ_HEADLESS to the environment used when launching Firefox, so it should be equivalent.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 33 • 7 years ago

this picked up recently, I am not clear on what was reverted and what headless is, :eeejay, can you look into this again and see if there is something else that can be done to get this resolved?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 34 • 7 years ago

this week the rate is much higher than last week- I would like to see some work on this next week- I will follow up on Tuesday with a patch to disable this if we don't have any progress on debugging/fixing.

	Eitan Isaacson [:eeejay]
	Comment 37 • 7 years ago

Brendan, were you looking into reworking this?

	Brendan Dahl [:bdahl] Assignee
	Comment 39 • 7 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #37)
> Brendan, were you looking into reworking this?

I had some time today to dig into it again. I've pushed what I believe is a fix to try, but I'll need to re-trigger to make sure everything is all right.

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

	Brendan Dahl [:bdahl] Assignee
	Comment 41 • 7 years ago

Occasionally marionette tries to get the accessibility element while the
elements are still being built. This causes getAccessibleFor to return
null when there actually should be an accessibility element available.
Instead, if the document is busy, wait until it finishes to get the
accessibility element.

This is similar to the approach that mochitests use.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 42 • 7 years ago

Comment on attachment 8877712 [details] [diff] [review]
Wait for accessibility element if accessibility doc is busy.

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

Thank you for the patch Brendan. Here just some small notes...

::: testing/marionette/accessibility.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +const {Services} = Cu.import('resource://gre/modules/Services.jsm');

Please use this line without `const {Services} =`, and add an empty line above.

@@ +123,5 @@
>     * @param {boolean=} mustHaveAccessible
>     *     Flag indicating that the element must have an accessible object.
>     *     Defaults to not require this.
>     *
> +   *

nit: looks like an unnecessary change.

@@ +179,5 @@
>            }
> +        }
> +      };
> +      Services.obs.addObserver(eventObserver, "accessible-event");
> +    }).catch((e) => this.error(

The brackets around `e` are not necessary.

	Brendan Dahl [:bdahl] Assignee
	Comment 43 • 7 years ago

Changing the reviewer and addressing the junk I left behind above.
One other note, with the new approach where we know if there's an accessible or not
it seems like it would be good to get rid of the 'mustHaveAccessible' argument and
handle that in callers instead, but this should probably be done in another bug.

	Pulsebot
	Comment 44 • 7 years ago

Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d220383ec899
Wait for accessibility element if accessibility doc is busy. r=automatedtester

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 46 • 7 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e953b73a75c3

	BMO Automation
	Comment 49 • 1 year ago

Moving bug to Testing::Marionette Client and Harness component per bug 1815831.