Files.app: Test local delete and directory delete

Files.app: Test local delete and directory delete

BUG=241113
TEST=browser_tests --gtest_filter="*FileManager*"

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

[mtomasz, 2013-05-28 11:07:17]

https://codereview.chromium.org/15919006/diff/3003/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:494:
IN_PROC_BROWSER_TEST_P(FileManagerBrowserLocalTest, TestKeyboardDelete) {
How about drive? Removing on drive is much more complex and bug prone, so I
think we should test it.

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:363:
checkIfNoErrorsOccured(chrome.test.succeed);
This will not work. We have to wrap it with a callbackPass or move the
chrome.test.succeed to a separate step.

[hashimoto, 2013-05-29 06:26:26]

https://codereview.chromium.org/15919006/diff/3003/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:494:
IN_PROC_BROWSER_TEST_P(FileManagerBrowserLocalTest, TestKeyboardDelete) {
On 2013/05/28 11:07:17, mtomasz wrote:
> How about drive? Removing on drive is much more complex and bug prone, so I
> think we should test it.

FileManagerBrowserDriveTest has TestKeyboardDelete already.
And the test case "keyboardDeleteDownloads" has been already there but not being
executed for an uncertain reason.
I'm just reactivating the test case.

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:363:
checkIfNoErrorsOccured(chrome.test.succeed);
On 2013/05/28 11:07:17, mtomasz wrote:
> This will not work. We have to wrap it with a callbackPass or move the
> chrome.test.succeed to a separate step.

Done.
Should we fix other tests doing the same thing in this file?

BTW, do we need to use callbackPass in this file at all?
As far as I looked at
chrome/renderer/resources/extensions/test_custom_bindings.js, callbackPass is
necessary only when chrome.test.runTests is used and we don't use it.
Perhaps, we should use chrome.test.runTests instead of manually dispatching test
case name with ExtensionTestMessageListener? (the only obstacle to do this seems
to be DoTestFileDisplay which uses an additional ExtensionTestMessageListener to
add a new file.)

[mtomasz, 2013-05-29 06:40:11]

lgtm

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:363:
checkIfNoErrorsOccured(chrome.test.succeed);
On 2013/05/29 06:26:26, hashimoto wrote:
> On 2013/05/28 11:07:17, mtomasz wrote:
> > This will not work. We have to wrap it with a callbackPass or move the
> > chrome.test.succeed to a separate step.
> 
> Done.
> Should we fix other tests doing the same thing in this file?

I'm going to do it here: crbug.com/244327 today together with other cleanup.

> 
> BTW, do we need to use callbackPass in this file at all?
> As far as I looked at
> chrome/renderer/resources/extensions/test_custom_bindings.js, callbackPass is
> necessary only when chrome.test.runTests is used and we don't use it.
> Perhaps, we should use chrome.test.runTests instead of manually dispatching
test
> case name with ExtensionTestMessageListener? (the only obstacle to do this
seems
> to be DoTestFileDisplay which uses an additional ExtensionTestMessageListener
to
> add a new file.)

We do use runTests, see background.js in file_manager_browsertest. The
testRunner function just queries for a test name. I guess this is to make js
tests with c++ test cases synchronized.

If we don't use callbackPass, then the test case will finish prematurely. AFAIR
we do not need to call chrome.test.succeed since it is called by the last queued
callbackPass, but I may be wrong here.

[hashimoto, 2013-05-29 06:53:16]

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js
(right):

https://codereview.chromium.org/15919006/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:363:
checkIfNoErrorsOccured(chrome.test.succeed);
On 2013/05/29 06:40:11, mtomasz wrote:
> On 2013/05/29 06:26:26, hashimoto wrote:
> > On 2013/05/28 11:07:17, mtomasz wrote:
> > > This will not work. We have to wrap it with a callbackPass or move the
> > > chrome.test.succeed to a separate step.
> > 
> > Done.
> > Should we fix other tests doing the same thing in this file?
> 
> I'm going to do it here: crbug.com/244327 today together with other cleanup.
> 
> > 
> > BTW, do we need to use callbackPass in this file at all?
> > As far as I looked at
> > chrome/renderer/resources/extensions/test_custom_bindings.js, callbackPass
is
> > necessary only when chrome.test.runTests is used and we don't use it.
> > Perhaps, we should use chrome.test.runTests instead of manually dispatching
> test
> > case name with ExtensionTestMessageListener? (the only obstacle to do this
> seems
> > to be DoTestFileDisplay which uses an additional
ExtensionTestMessageListener
> to
> > add a new file.)
> 
> We do use runTests, see background.js in file_manager_browsertest. The
> testRunner function just queries for a test name. I guess this is to make js
> tests with c++ test cases synchronized.
Ah, I see.
> 
> If we don't use callbackPass, then the test case will finish prematurely.
AFAIR
> we do not need to call chrome.test.succeed since it is called by the last
queued
> callbackPass, but I may be wrong here.

[commit-bot: I haz the power, 2013-05-29 06:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/15919006/16001

[commit-bot: I haz the power, 2013-05-29 07:20:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-29 07:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/15919006/16001

[commit-bot: I haz the power, 2013-05-29 11:38:51]

Change committed as 202843