Files.app: Reland r261616: Add a test to rename a file.

Files.app: Reland r261616: Add a test to rename a file.

Previous patch was reverted because of flakiness.
The main reason of the flakiness is a race like followings:

1. After renaming, Files.app commit the new file name previsionally.
2. Test regards as the rename was done and starts the next rename.
3. Previous rename was actually done in a file system and the file list
   is updated. It cancels the second rename.

This patches add a marker for previsionally commitment, and let the test wait
until the marker is removed.

Previously reverted CL: crrev.com/221673002

BUG=345062
TEST=run the test.

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

[hirono, 2014-04-09 03:13:09]

PTAL the CL? Thanks!

[mtomasz, 2014-04-09 03:38:54]

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:343:
* Sends fake key down event.
nit: fake -> a fake

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:345:
* @param {string} query Query for target element.
nit: target -> the target

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:137:
}).then(chrome.test.callbackPass(function() {
Why this change?

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:187:
* @param {string} windowId ID of window.
nit: window -> the window

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:207:
// Wait until rename completes.
I think this is racy. The class 'provisional' may not be added before
waitForElementLost is called.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:228:
expectedEntryRows[i][0] = 'New File Name.txt';
This works, but it is not readable. Can we move it to #225, under the if? We
could read it then:

'For every item in expected entry rows, if the name is hello.txt, then change it
to 'new file name.txt' and stop.'

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:230:
// Open window.
nit: window -> a window

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:249:
// The name does not changed.
does not changed -> did not change OR has not changed OR is not changed

[mtomasz, 2014-04-09 03:42:14]

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:207:
// Wait until rename completes.
On 2014/04/09 03:38:54, mtomasz wrote:
> I think this is racy. The class 'provisional' may not be added before
> waitForElementLost is called.

To fix this race, we could eg. wait for the newName, and then for 'provisional'
class removed. Then we would have a guarantee that 'provisional' was set, and
later removed (since it is set together with the new name). WDYT?

[hirono, 2014-04-09 20:01:12]

Thanks!

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:343:
* Sends fake key down event.
On 2014/04/09 03:38:54, mtomasz wrote:
> nit: fake -> a fake

Done.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:345:
* @param {string} query Query for target element.
On 2014/04/09 03:38:54, mtomasz wrote:
> nit: target -> the target

Done.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:137:
}).then(chrome.test.callbackPass(function() {
On 2014/04/09 03:38:54, mtomasz wrote:
> Why this change?

Because chrome.test.succeed generates a log.
We already have succeed log end of tests. So it generates succeed log doubly.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:187:
* @param {string} windowId ID of window.
On 2014/04/09 03:38:54, mtomasz wrote:
> nit: window -> the window

Done.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:207:
// Wait until rename completes.
On 2014/04/09 03:42:15, mtomasz wrote:
> On 2014/04/09 03:38:54, mtomasz wrote:
> > I think this is racy. The class 'provisional' may not be added before
> > waitForElementLost is called.
> 
> To fix this race, we could eg. wait for the newName, and then for
'provisional'
> class removed. Then we would have a guarantee that 'provisional' was set, and
> later removed (since it is set together with the new name). WDYT?

This is race. But we cannot fix it by waiting newName.

1. Rename to newName.
2. It is an invalid name and the name is reverted.
3. Waiting newName. But it will not be seen.

I updated the lifetime of the marker so that we can ensure that renaming
completes.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:228:
expectedEntryRows[i][0] = 'New File Name.txt';
On 2014/04/09 03:38:54, mtomasz wrote:
> This works, but it is not readable. Can we move it to #225, under the if? We
> could read it then:
> 
> 'For every item in expected entry rows, if the name is hello.txt, then change
it
> to 'new file name.txt' and stop.'

Done.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:230:
// Open window.
On 2014/04/09 03:38:54, mtomasz wrote:
> nit: window -> a window

Done.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:249:
// The name does not changed.
On 2014/04/09 03:38:54, mtomasz wrote:
> does not changed -> did not change OR has not changed OR is not changed

Done.

[mtomasz, 2014-04-09 21:15:19]

lgtm with one nit!

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:137:
}).then(chrome.test.callbackPass(function() {
On 2014/04/09 20:01:13, hirono wrote:
> On 2014/04/09 03:38:54, mtomasz wrote:
> > Why this change?
> 
> Because chrome.test.succeed generates a log.
> We already have succeed log end of tests. So it generates succeed log doubly.

Please add a comment exactly saying that. Also, please comment that the
callbacPass is necessary to avoid prematurely finishing tests. Currently it
looks very misterious and is difficult to understand.

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:207:
// Wait until rename completes.
On 2014/04/09 20:01:13, hirono wrote:
> On 2014/04/09 03:42:15, mtomasz wrote:
> > On 2014/04/09 03:38:54, mtomasz wrote:
> > > I think this is racy. The class 'provisional' may not be added before
> > > waitForElementLost is called.
> > 
> > To fix this race, we could eg. wait for the newName, and then for
> 'provisional'
> > class removed. Then we would have a guarantee that 'provisional' was set,
and
> > later removed (since it is set together with the new name). WDYT?
> 
> This is race. But we cannot fix it by waiting newName.
> 
> 1. Rename to newName.
> 2. It is an invalid name and the name is reverted.
> 3. Waiting newName. But it will not be seen.
> 
> I updated the lifetime of the marker so that we can ensure that renaming
> completes.

Yes, that would cause a test timeout, so the test would be slower in case on
failures.

[hirono, 2014-04-09 21:37:05]

Thanks!

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js
(right):

https://codereview.chromium.org/230073002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/keyboard_operations.js:137:
}).then(chrome.test.callbackPass(function() {
On 2014/04/09 21:15:19, mtomasz wrote:
> On 2014/04/09 20:01:13, hirono wrote:
> > On 2014/04/09 03:38:54, mtomasz wrote:
> > > Why this change?
> > 
> > Because chrome.test.succeed generates a log.
> > We already have succeed log end of tests. So it generates succeed log
doubly.
> 
> Please add a comment exactly saying that. Also, please comment that the
> callbacPass is necessary to avoid prematurely finishing tests. Currently it
> looks very misterious and is difficult to understand.

Done.

[hirono, 2014-04-09 21:37:09]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-04-09 21:38:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/230073002/60001

[commit-bot: I haz the power, 2014-04-09 22:43:35]

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

[commit-bot: I haz the power, 2014-04-09 22:43:35]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[hirono, 2014-04-10 00:04:50]

The CQ bit was checked by hirono@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/230073002/80001

[commit-bot: I haz the power, 2014-04-10 04:43:28]

Change committed as 262912