Add API tests to the experimental Speech Input API.

Add API tests to the experimental Speech Input API.


BUG=97388
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106916

[Leandro Graciá Gil, 2011-10-17 14:04:30]

Aaron, could you please take a look to the
chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc file?

Jay, could you please do the same with the test files under
chrome/test/data/extensions/api_test/speech_input/?

Satish: feel free to take a general look to the code and tests.

Thanks.

[Jay Civelli, 2011-10-20 16:17:05]

I could review but I am probably not the best person for this.
Adding asargent.

[Aaron Boodman, 2011-10-21 00:52:47]

Please consider rewriting this using the new ExtensionTest framework: 

http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/brows...

If you can do so, please make it a unit test, not a browser test. The framework
should work both ways.

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
File chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc
(right):

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc:105:
bool recording_devices_available_;
Please always initialize primitive values. In this case, in the constructor.

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc:120:
recording_devices_available_ = true;
Just do this in the contructor.

[Leandro Graciá Gil, 2011-10-21 10:53:58]

It looks like I'm already using the new testing engine: ExtensionApiTest
inherits from ExtensionBrowserTest which inherits from InProcessBrowserTest. I'm
using ExtensionApiTest because it provides the RunExtensionTest method.

I have no problem moving this test to the unit tests, however all other
extension API tests are browser tests and the class hierarchy has BrowserTest
explicitly twice in the class names. Are you sure this should be changed to unit
tests?

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
File chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc
(right):

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc:105:
bool recording_devices_available_;
On 2011/10/21 00:52:47, Aaron Boodman wrote:
> Please always initialize primitive values. In this case, in the constructor.

Done.

http://codereview.chromium.org/8321003/diff/1/chrome/browser/extensions/speec...
chrome/browser/extensions/speech_input/extension_speech_input_apitest.cc:120:
recording_devices_available_ = true;
On 2011/10/21 00:52:47, Aaron Boodman wrote:
> Just do this in the contructor.

Done.

[Aaron Boodman, 2011-10-21 16:35:37]

On Fri, Oct 21, 2011 at 3:53 AM,  <leandrogracia@chromium.org> wrote:
> It looks like I'm already using the new testing engine: ExtensionApiTest
> inherits from ExtensionBrowserTest which inherits from InProcessBrowserTest.
> I'm
> using ExtensionApiTest because it provides the RunExtensionTest method.

ExtensionTest, not ExtensionAPITest. ExtensionTest is the new newness.
ExtensionAPITest is the old newness.

We have historically had a lot of problems with flakiness of
ExtensionAPITest because they are essentially integration tests that
test the entire system. They are also hard to debug when things go
wrong.

The new ExtensionTest framework only tests the browser-process side of
extension APIs. It excludes the renderer side, which is usually
generated and is well-covered by other tests.

> I have no problem moving this test to the unit tests, however all other
> extension API tests are browser tests and the class hierarchy has
> BrowserTest
> explicitly twice in the class names. Are you sure this should be changed to
> unit
> tests?

I'm trying to encourage everyone to give it a shot for new tests. Just
like any new approach, there may be some pain, but I think in the long
run having better isolated tests will be better for everyone.

Take a look at extension_function_test_utils.h. You can use it with
either a BrowserTest (like extension_tabs_test.cc does), or with a
plain unit test (if your dependencies are low). If you can get it
working with not a lot of hassle, I would prefer it. If you find that
it doesn't work easily, you can leave the current API tests. Just let
me know what doesn't work so that I know what to improve first.

Thanks,

- a

[Aaron Boodman, 2011-10-21 16:55:34]

Nevermind, I see now you need to test events and we don't have good support for
that yet.

[Aaron Boodman, 2011-10-21 16:57:18]

lgtm

[satish1, 2011-10-21 21:53:49]

LGTM