1084412 - searchTimeout doesn't work when using findElements

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andrew Halberstadt [:ahal]]

Attachment: test case

I noticed that searchTimeout doesn't work when using marionette.find_elements(). This is because the findElements() function [1] returns "[]" if no elements were found. But since !![] == true, this if statement [2] always evaluates to true, meaning we return the empty list immediately rather than waiting for searchTimeout.

Test case that fails attached.

Comment 1

[Andrew Halberstadt [:ahal]]

[1] http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-elements.js#443
[2] http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-elements.js#296

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Fix findElements so it honours searchTimeout

The included test fails without this patch and works with it.

Comment 3

[David Burns :automatedtester]

Comment on attachment 8506996 [details] [diff] [review]
Fix findElements so it honours searchTimeout

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

::: testing/marionette/client/marionette/tests/unit/test_findelement.py
@@ +130,5 @@
>          self.assertRaises(NoSuchElementException, self.marionette.find_element, By.ID, "newDiv")
>          self.assertTrue(True, self.marionette.set_search_timeout(8000))
>          self.assertEqual(HTMLElement, type(self.marionette.find_element(By.ID, "newDiv")))
>  
> +    def test_timeout_elements(self):

I know you copied this from the singular version but the assumptions are wrong...

Can you raise a bug to have these checked since you should set the search timeout whenever and carry on as normal

Comment 4

[Andrew Halberstadt [:ahal]]

I don't see what you mean, could you clarify what assumption is wrong?

Comment 5

[David Burns :automatedtester]

This is how I would expect the test to be written

> test_html = self.marionette.absolute_url("test.html")
> self.marionette.navigate(test_html)
> self.assertEqual(len(self.marionette.find_elements(By.ID, "newDiv")), 0)
> self.marionette.set_search_timeout(8000)
> button = self.marionette.find_element("id", "createDivButton")
> button.click()
> self.assertEqual(len(self.marionette.find_elements(By.ID, "newDiv")), 1)

It feels like we could fail on `> self.assertEqual(len(self.marionette.find_elements(By.ID, "newDiv")), 0)` if we are on a slow machine.

I dont see this as affecting the landing of your patch. I just think these tests need to be audited

Comment 6

[Andrew Halberstadt [:ahal]]

The 'test.html' page that gets loaded does a 2 second delay before creating the element the tests are looking for:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/www/test.html#15

Though I guess there's still technically a race condition, so I don't mind filing a follow up.

Comment 7

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e2d9aff637

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56e2d9aff637

Comment 9

[Kevin Grandon :kgrandon]

Andrew, David - This is causing multiple JSMarionette test suite failures, and our tree is closed because of it. Can it be backed out until we adjust our tests to work with this?

Comment 10

[Kevin Grandon :kgrandon]

checkin-needed: Can I request a backout here so we can reopen gaia trees?

Comment 11

[Kevin Grandon :kgrandon]

Removing checkin-needed for the backout. It seems Tim has found the problem in bug 1087038, so I suppose we should try to go with a proper fix for now.

Comment 12

[Andrew Halberstadt [:ahal]]

Ah, apologies. This was my first marionette server patch and I neglected to look at the gaia trees. I'll remember to do so next time.

Comment 13

[Andrew Halberstadt [:ahal]]

I backed this out since bug 1087038 still isn't fixed:
https://hg.mozilla.org/mozilla-central/rev/305d24174ace

Comment 14

[Andrew Halberstadt [:ahal]]

Should I land this directly on m-i/b-i as well? Or should we merge it there.. not sure how this works.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> Should I land this directly on m-i/b-i as well? Or should we merge it
> there.. not sure how this works.

The backout patch was merged from m-c to m-i earlier:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6066a2a0766f

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

:ahal, you might want to try reland your patch (with mine squash on it), after testing Gij with this try syntax:

try: -b o -p linux64_gecko -u all -t none

Note that the tests are hidden.

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Fix findElements so it honours searchTimeout

I stepped away from this for a bit until bug 1080764 was finished. This patch is the same as before except with Tim's patch from bug 1087038 qfolded on top. Since both patches were individually r+'ed by AutomatedTester, carrying r+ forward.

From reading bug 1087038, it looks like maybe Tim's patch was all that was needed after all and that this got mistakenly backed out.

Here's a try run to verify:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca9443b50d7a

Comment 18

[Andrew Halberstadt [:ahal]]

This is still breaking Gij even with Tim's patch. I looked at one of the errors for the clock tests..

This call to waitFor [1] is timing out. That suggests that the "$(selector).tap()" call above isn't working. I poked around and $(selector) is indeed using findElements [2], so my change is definitely exposing this.

This suggests two things to me:
1) The call to findElements isn't working, but no errors are thrown/logged. Seems like mquery.js should raise if no elements were found.
2) marionette-js-client is somehow depending on the broken behaviour of marionette server's findElements and needs to be updated, or there's still a subtle error in this patch.

I can't figure this out just by inspecting, I'll need to set up a gaia-integration environment to test further.

Comment 19

[Andrew Halberstadt [:ahal]]

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/test/marionette/lib/actions.js#L66
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/test/marionette/lib/mquery.js#L59

Comment 20

[Andrew Halberstadt [:ahal]]

I filed bug 1091783 to look into the Gij test failures this patch causes. I figured out why the clock tests were failing and uploaded a patch there. Now it's just a matter of looking into the other chunks and coordinating the landing.

Comment 21

[Andrew Halberstadt [:ahal]]

Now that bug 1091783 is fixed, here's a new try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f3f05dadc96c&searchQuery=js-integration

The failed '1' looks like infrastructure issues, while the failed '3' I believe is a known intermittent (at least I saw a similar job with the same failure). So I think this is good to land. Just waiting on inbound being open.

Comment 22

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b168f06c71f9

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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