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.

[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

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

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

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

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

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.

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?

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

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.

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.

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

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

(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

: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.

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

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.

[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

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.

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.