1108576 - Experiment with methods to speed up the keyboard's send() method

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Zac C (:zac)]

Some have observed that the keyboard app object's send() method is painfully slow.

A couple of things were discussed during the work week: 
1. Remove the wait(0.1)

2. Use send_keys for all but the last character and then use an action to send the final character (which would trigger the right events that Gaia needs).

Comment 1

[Zac C (:zac)]

I tried #1 and it makes almost 0 effect because the wait time is almost entirely in the rocketbar's search results being rendered. Although there could be some improvement when typing outside of this scope.

#2 cannot be done easily because the current keyboard app's send() method has no knowledge of the <input> element it's interacting with. but there may be ways around it and I'll experiment a bit further.

Comment 2

[Zac C (:zac)]

Experimenting here:
https://github.com/mozilla-b2g/gaia/pull/26656

This could invalidate some of the existing keyboard tests so I need to check over them before this is merged.

http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/412/console

Comment 3

[Zac C (:zac)]

Two tests failed on treeherder.

Trying again on both CI.

http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/413/console

Comment 4

[Zac C (:zac)]

Attachment: github pr

Method 2 as described in comment #0.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

FWIW the proposed changes make me uneasy. We are emulating the user and the user has no option but to use the on-screen keyboard. If there are delays introduced by the test tool or framework then we should do whatever we can to reduce those. If this is a general keyboard performance issue then it should be dealt with in the application under test.

Another option would be to reduce the amount we use the keyboard. For the browser tests we could use shorter URLs, and for any other tests we should easily be able to reduce the length of the strings we're typing. Has anybody profiled using the keyboard via gaiatest? It would be interesting to see where the time is spent.

Comment 6

[Martijn Wargers (dead)]

Comment on attachment 8533226 [details] [review]
github pr

The patch seems to work fine here.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Dave Hunt (:davehunt) from comment #5)
> FWIW the proposed changes make me uneasy. We are emulating the user and the
> user has no option but to use the on-screen keyboard. If there are delays
> introduced by the test tool or framework then we should do whatever we can
> to reduce those. If this is a general keyboard performance issue then it
> should be dealt with in the application under test.

Dave,

Happy to have an extended conversation about this offline, but I asked for this change (or more accurately, indicated I'd make it myself in the page object/test level if need be).

The short answer is that the strategy for reliable, fast UI testing is two-pronged:

In setup/cleanup, use the absolute fastest, most foolproof way possible. Generally, use user actions or even the UI in general *as little as possible* because they/it are slow and error-prone. This includes any navigation or other incidental functions on the way to the specific change and subsequent verification being tested.

In the specific thing being changed that causes a state change to be verified (i.e., the test), use user-actions for all the reasons you mention. I will say that (in a classic OS suite) it's typical to use clipboard paste instead of keystrokes, even here, unless you know that the area in question is per-stroke sensitive, like a password validator or the Rocketbar.

There's really not a ton of value in getting "incidental" testing on the way to the tests you're crafting. If anything, they tend to ruin your isolation and blow huge portions of the test suite away when minor bugs block setup for other tests.

I have no idea what the Selenium standards are here, but this is the plan that made the suites I've crafted in the past halfway robust and still execute quickly. I do agree with optimizing the keystrokes as much as possible, but it's not going to be enough. We really need these tests to execute significantly faster, both for sanity when desk-debugging and for runtime efficiency.

Like I said, hit me up offline. I don't think an extended philosophical discussion is going to be awesome in a bug.

One comment re: Zac's stuff, I need to take a closer look but I'm not sure it's great to have the Keyboard object use anything but the keyboard. Otherwise, that might be surprising behavior.

Even realizing how many things would need to be touched, this might be more a case where I'd probably take any field setters in a page object and modify those to use injection instead, and add a type_field() type method that uses the keyboard for use in the tests (or an arg for by_keyboard=True, perhaps).

I haven't looked at the patch closely yet, though, so take that with a grain of salt.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

To clarify, a simple example might be that if you have a login box at the beginning of all your flows, the tests that specifically test login type, but the tests that are just logging in to get to the rest inject and hit the button (or use a hook to quickly log in without UI). 

That would be true whether or not going through login was formally considered setup--point is it's not the thing being tested most of the time, and keyboard handling got tested in a specific section.

So that means we need to keep typing around, still add capability for injection, and be conscious of how we're actually using a bit of UI in the tests to do either the fast way or the user way.

For another example, look at the tests I loosely specced in bug 1052267 -- #2 would need to inject for sheer speed purposes to iterate through a bunch of values, but that makes it important to add test #3, which actually does type to make sure that works too.

Comment 9

[Zac C (:zac)]

In my testing I found that it's the rocketbar's search that's taking the majority of the time. The keyboard could type faster.

We're essentially just working around the lack of dev action in https://bugzilla.mozilla.org/show_bug.cgi?id=1067472.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Rocketbar was definitely the worst offender. Other fields aren't as bad. 

I do want to try this locally in part to see what the effective speedup was. To Dave's point, it's a cost-benefit call, and if the speedup isn't much then we should arguably default to typing but then do injection selectively when needed. Usually it's a pretty decent boost to skip the keyboard for the default cases though.

Comment 11

[Zac C (:zac)]

For a --type=b2g+smoketest run I found that it saved around 5 minutes overall, which was about 7.5%. Most of the gain is in few tests; test_browser_lan for example goes from ~159 seconds to ~81s. If you're impartial to the test coverage then the speed gain is significant.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm sure browser tests can just use Marionette's navigate [1] method whilst in the content frame as an alternative. If the keyboard had considerably more test coverage I might agree that the incidental coverage was unnecessary, but as it's such a critical feature I've always been keen to exploit the extra coverage. For Selenium, the goal is to send keys in much the same way a user would (emulating key strokes). This is the case in WebDriver, whereas in the original Selenium implementation the value was directly modified.

For Rocketbar, can't we disable the search executed after each keypress? I suspect this is causing Marionette to wait for the document to be ready after each tap, or does this also slow down the speed you can manually use the keyboard?

[1] http://marionette-client.readthedocs.org/en/latest/reference.html#marionette.Marionette.navigate

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Dave Hunt (:davehunt) from comment #12)
> I'm sure browser tests can just use Marionette's navigate [1] method whilst
> in the content frame as an alternative. If the keyboard had considerably
> more test coverage I might agree that the incidental coverage was
> unnecessary, but as it's such a critical feature I've always been keen to
> exploit the extra coverage. For Selenium, the goal is to send keys in much

What kind of coverage would we need on the keyboard to make such a strategy more palatable for you? I should be clear that we want as much coverage of differentiated functionality as possible, but if there's no reason to think that an area does special key-by-key processing, doing so anyway comes at a cost.

> the same way a user would (emulating key strokes). This is the case in
> WebDriver, whereas in the original Selenium implementation the value was
> directly modified.

What's the speed like on that compared to ours? Like I said, I've seen paste used as a compromise between the two strategies, but Webdriver may not have access to that since that's a system-external thing. It's also destructive to the global keyboard, which matters more when testing a single app.

Anyway, to some extent this is philosophical. We both agree key-by-key gives you more coverage, and I'm sure we agree there's a speed hit. My thing is that if the errors aren't likely, or aren't actionable, or aren't isolated, losing the occasional time it would be useful is a bad trade for the every time it's slower.

As for the patch:

I'm honestly not sure how best to do this but I'm leaning r- for the reasons above re: using the keyboard app object to inject text would surprise me. I kind of expect anything with the Keyboard object to use the keyboard.

I still feel like this is probably something better encapsulated at the page object level (perhaps with the addition of a central helper method to handle the l10n concerns Keyboard seems to handle, if those are still relevant).

The only thing that gives me pause is that the comment/doc on send() says it's for when you don't care about the Keyboard.

Beyond that, this feels too global to me without more exploration, as much as I do want to see us improve this. The point of having both methodologies available is to use whichever one is better to the task at hand. But as blind decisions go, blindly switching them all over to injection is riskier than blindly having them all use the keyboard and I'm concerned about false passes.

Since I do have a bug/test at hand that really does need to inject text to make the runtime sane, it might make sense to briefly freezer this (since the code is excellent), let me try something more context-sensitive for that particular test, and then figure out if we can easily and safely migrate whatever comes out of that more widely.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Throwing this out there: when I was brainstorming with Zac in Portland, once suggestion was a "conservative mode."

This would work by still using two ways of entering text. Things that absolutely need to type out on the keyboard because it's integral to the test would use Keyboard.send() as it exists prior to this patch. Things that don't need to type out on the keyboard would use a setter on the test's target app object, which in turn defers to a global helper to directly set the text.

So that's the same as I suggested above.

What's different is that if you ran the tests with ALWAYS_TYPE=1 or something along those lines, the global helper defers to Keyboard.send() instead, and it works like it does now.

Normally we'd run without ALWAYS_TYPE for speed, but we could use it either periodically or ad hoc as needed to double-check ourselves or debug.

Compare to the option to restart or not on the current test runner.

Difference

Comment 15

[Zac C (:zac)]

Dave I don't think navigate could be used because the Haida/Rocketbar UI flow is that it launches a new browser window with the URL or search term you've input. You'd have to launch an empty window somehow and then navigate it and I'm not sure this is possible. I'm not aware of any way to get an empty browser window.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #15)
> Dave I don't think navigate could be used because the Haida/Rocketbar UI
> flow is that it launches a new browser window with the URL or search term
> you've input. You'd have to launch an empty window somehow and then navigate
> it and I'm not sure this is possible. I'm not aware of any way to get an
> empty browser window.

Ah, right - I was thinking of the old browser app.

Comment 17

[Viorela Ioia [:viorela]]

As I said in the PR, I agree with the changes in Keyboard.send() method, to use send_keys() for all but the last character in tests that are not focused on testing the keyboard behavior.
It is definitely reducing the duration of a lot of test runs.

As for the keyboard tests, I believe we should update them to actually tap on each character, due to the fact that the purpose of those tests is to check the keyboard behavior.

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8533226 [details] [review]
github pr

After chewing on it, my opinion stays the same: changing the Keyboard.send() to do this violates "least surprise." I support the idea, but think we shouldn't modify the Keyboard object to achieve it.

Comment 19

[BMO Automation]

Firefox OS is not being worked on