Adds API tests for listRemovableStorageDevices

Adds API tests for listRemovableStorageDevices

BUG=370658
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270030

[Drew Haven, 2014-05-06 23:15:52]

Antony,

This is my first attempt at an API test.  Could you look it over and help me
figure out if I did everything roughly correct?  I'm going to write a lot more
tests, but I figured it's best to start with one and make sure I'm on the right
track.

-- Drew

[asargent_no_longer_on_chrome, 2014-05-07 21:31:40]

Good start! Just a few things to fix.

https://codereview.chromium.org/266363006/diff/50001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/removable_storage_provider.cc
(right):

https://codereview.chromium.org/266363006/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/removable_storage_provider.cc:12:
static scoped_refptr<StorageDeviceList> g_test_device_list;
Hmm, I'm not sure that scoped_refptr is static initialization safe. You should
probably wrap this with base::LazyInstance.

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-...

https://codereview.chromium.org/266363006/diff/50001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js
(right):

https://codereview.chromium.org/266363006/diff/50001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js:45:
chrome.test.runTests([testDeviceList])
FYI, I think your test is in danger of completing before your callback ever gets
a chance to fire. You either need to *not* use chrome.test.runTests and only
manually call chrome.test.succeed/chrome.test.fail once, *or* use
chrome.test.runTests along with the helpers to make sure it doesn't early
succeed (it waits for known outstanding callbacks to fire before moving on to
success for each test, but you have to tell it that a callback is outstanding).

The chrome.test framework is a little creaky and could use better documentation.
You can see the source code at
chrome/renderer/resources/extensions/test_custom_bindings.js

The general idea for most tests is:


chrome.test.runTests([

  function myFirstTest() {
    // We use chrome.test.callback here to let the test 
    // framework know that there is an outstanding callback
    // and it should not move on to the next test until the
    // callback has fired. If no assertion fails inside the
    // callback, it will consider the test a success and
    // move on to the next one.
    chrome.foo.someMethodWithACallback(
      42, chrome.test.callback(function(some_args) {
      chrome.test.assertEq(some_args.whatever, 19);
    });
  },


  function mySecondTest() {
    // This demonstrates telling the test framework that
    // we are calling a function with a callback and expect
    // that chrome.runtime.lastError will be set, with a
    // particular value for the error message in
    // chrome.runtime.lastError.message.
    chrome.foo.someMethodWithACallback(
      "whatever", chrome.test.callbackFail("invalid input"));
  }

[Drew Haven, 2014-05-08 00:14:33]

Thanks for the input, Antony, particularly the long comment about using
chrome.test.callback.  It was very useful.

https://codereview.chromium.org/266363006/diff/50001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/removable_storage_provider.cc
(right):

https://codereview.chromium.org/266363006/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/removable_storage_provider.cc:12:
static scoped_refptr<StorageDeviceList> g_test_device_list;
On 2014/05/07 21:31:40, Antony Sargent wrote:
> Hmm, I'm not sure that scoped_refptr is static initialization safe. You should
> probably wrap this with base::LazyInstance.
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-...

Shouldn't hurt in any case.  Good catch.  Sometimes smart pointers aren't just
pointers. :)

https://codereview.chromium.org/266363006/diff/50001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js
(right):

https://codereview.chromium.org/266363006/diff/50001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js:45:
chrome.test.runTests([testDeviceList])
On 2014/05/07 21:31:40, Antony Sargent wrote:
> FYI, I think your test is in danger of completing before your callback ever
gets
> a chance to fire. You either need to *not* use chrome.test.runTests and only
> manually call chrome.test.succeed/chrome.test.fail once, *or* use
> chrome.test.runTests along with the helpers to make sure it doesn't early
> succeed (it waits for known outstanding callbacks to fire before moving on to
> success for each test, but you have to tell it that a callback is
outstanding).
> 
> The chrome.test framework is a little creaky and could use better
documentation.
> You can see the source code at
> chrome/renderer/resources/extensions/test_custom_bindings.js
> 
> The general idea for most tests is:
> 
> 
> chrome.test.runTests([
> 
>   function myFirstTest() {
>     // We use chrome.test.callback here to let the test 
>     // framework know that there is an outstanding callback
>     // and it should not move on to the next test until the
>     // callback has fired. If no assertion fails inside the
>     // callback, it will consider the test a success and
>     // move on to the next one.
>     chrome.foo.someMethodWithACallback(
>       42, chrome.test.callback(function(some_args) {
>       chrome.test.assertEq(some_args.whatever, 19);
>     });
>   },
> 
> 
>   function mySecondTest() {
>     // This demonstrates telling the test framework that
>     // we are calling a function with a callback and expect
>     // that chrome.runtime.lastError will be set, with a
>     // particular value for the error message in
>     // chrome.runtime.lastError.message.
>     chrome.foo.someMethodWithACallback(
>       "whatever", chrome.test.callbackFail("invalid input"));
>   }
> 
> 
> 

Yeah, I had trouble understanding the framework and looking for documentation. 
My next test involves spawning a window and not just a background page so it can
use the fileSystem API and I'm having trouble getting it all to work.

I went with the chrome.test.callback in this case because I feel like if we have
a test framework I should learn to use it.

[asargent_no_longer_on_chrome, 2014-05-08 16:18:48]

lgtm w/ one last nit

https://codereview.chromium.org/266363006/diff/70001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js
(right):

https://codereview.chromium.org/266363006/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js:38:
chrome.test.succeed();
nit: (Sorry if I didn't explain this the first time around) If you're going to
use chrome.test.callback, then you don't need/want to manually call
chrome.test.succeed (and usually not chrome.test.fail either). The general idea
is that runTests will call all the functions in the array passed to it, and for
each one it will wait until there are no outstanding callbacks, and as long as
no assert* statements have failed, it will consider that test case a success and
then move on to the next case. Underneath the covers, the framework will call
chrome.test.succeed/chrome.test.fail for you. 

In contrast, using chrome.test.succeed and chrome.test.fail let you manage this
process manually - the overall GTEST will just start up and wait for your script
to either call succeed or fail. If you don't do either of those by the GTEST
timeout value, then your test fails due to GTEST timeout.

[Drew Haven, 2014-05-08 21:21:31]

https://codereview.chromium.org/266363006/diff/70001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js
(right):

https://codereview.chromium.org/266363006/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/image_writer_private/list_devices/test.js:38:
chrome.test.succeed();
On 2014/05/08 16:18:48, Antony Sargent wrote:
> nit: (Sorry if I didn't explain this the first time around) If you're going to
> use chrome.test.callback, then you don't need/want to manually call
> chrome.test.succeed (and usually not chrome.test.fail either). The general
idea
> is that runTests will call all the functions in the array passed to it, and
for
> each one it will wait until there are no outstanding callbacks, and as long as
> no assert* statements have failed, it will consider that test case a success
and
> then move on to the next case. Underneath the covers, the framework will call
> chrome.test.succeed/chrome.test.fail for you. 
> 
> In contrast, using chrome.test.succeed and chrome.test.fail let you manage
this
> process manually - the overall GTEST will just start up and wait for your
script
> to either call succeed or fail. If you don't do either of those by the GTEST
> timeout value, then your test fails due to GTEST timeout. 
> 
> 

Aaaah, I see.  I swapped it to use no explicit pass-fail.  Now the use of
callbackPass/callbackFail make a bit more sense.

[Drew Haven, 2014-05-08 22:09:38]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-08 22:16:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/266363006/90001

[commit-bot: I haz the power, 2014-05-09 02:32:51]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 03:45:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 03:45:42]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Drew Haven, 2014-05-09 23:48:49]

The CQ bit was checked by haven@chromium.org

[Drew Haven, 2014-05-09 23:51:10]

The CQ bit was unchecked by haven@chromium.org

[Drew Haven, 2014-05-10 00:20:27]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-10 00:23:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/266363006/110001

[commit-bot: I haz the power, 2014-05-10 03:32:16]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 05:04:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 05:04:14]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Drew Haven, 2014-05-12 19:05:52]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-12 19:07:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/266363006/110001

[commit-bot: I haz the power, 2014-05-12 22:40:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-12 23:14:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 23:14:32]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Drew Haven, 2014-05-13 01:47:26]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-13 01:48:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/266363006/150001

[commit-bot: I haz the power, 2014-05-13 05:35:37]

Change committed as 270030