Added combined test for Syncable File System to write file and then get that usage gets updated. (issue 11368160)

[calv...@chromium.org]

Reviewers: benwells, kinuko,

Message:
Thanks to Kinuko for helping me resolve the issue I had with making this
combined test. I think it should pass now without being flakey. Please take
a
look.

Description:
Added combined test for Syncable File System to write file and then get that
usage gets updated.

BUG=153501


Please review this at http://codereview.chromium.org/11368160/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
A
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
A
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js


Index:
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
diff --git
a/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
b/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
index
497e4db2ae3db30c46219090a116bdf05835699a..64793ed1bf72dd6e856ef657f76d687566fb7d4d
100644
---
a/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
+++
b/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
@@ -55,4 +55,9 @@ IN_PROC_BROWSER_TEST_F(SyncFileSystemApiTest,
RequestFileSystem) {
<< message_;
}

+IN_PROC_BROWSER_TEST_F(SyncFileSystemApiTest, WriteFileThenGetUsage) {
+
ASSERT_TRUE(RunPlatformAppTest("sync_file_system/write_file_then_get_usage"))
+ << message_;
+}
+
} // namespace chrome
Index:
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
diff --git
a/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
b/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
new file mode 100644
index
0000000000000000000000000000000000000000..43e969ca4e5f66349b4ba04d8b71c1bad6c730b0
--- /dev/null
+++
b/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
@@ -0,0 +1,12 @@
+{
+ "name": "chrome.syncFileSystem.writeFileThenGetUsage",
+ "version": "0.1",
+ "manifest_version": 2,
+ "description": "Compound test to get file system, write a file with some
data and check usage gets updated accurately.",
+ "permissions": ["syncFileSystem"],
+ "app": {
+ "background": {
+ "scripts": ["test.js"]
+ }
+ }
+}
Index:
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
diff --git
a/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
b/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
new file mode 100644
index
0000000000000000000000000000000000000000..dbc440b90b875876d2e45274c1f881007a34843c
--- /dev/null
+++
b/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
@@ -0,0 +1,74 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var fileEntry;
+var usageBeforeWrite;
+var usageAfterWrite;
+var testData = "12345";
+
+var testStep = [
+ function () {
+ chrome.syncFileSystem.requestFileSystem('drive', testStep.shift());
+ },
+ // Create empty file
+ function(fileSystem) {
+ fileSystem.root.getFile('Test.txt', {create: true}, testStep.shift(),
+ errorHandler);
+ },
+ function(fe) {
+ fileEntry = fe;
+ testStep.shift()();
+ },
+ // Record usage before write
+ function() {
+ chrome.syncFileSystem.getUsageAndQuota('drive', testStep.shift());
+ },
+ function(storageInfo) {
+ usageBeforeWrite = storageInfo.usage_bytes;
+ testStep.shift()();
+ },
+ // Write a known number of bytes
+ function() {
+ fileEntry.createWriter(testStep.shift(), errorHandler);
+ },
+ function (fileWriter) {
+ fileWriter.onwriteend = function(e) {
+ testStep.shift()();
+ };
+
+ fileWriter.onerror = function(e) {
+ chrome.test.fail('Write failed: ' + e.toString());
+ };
+
+ fileWriter.seek(fileWriter.length);
+ var blob = new Blob([testData], {type: "text/plain"});
+ fileWriter.write(blob);
+ },
+ // Check the meta data for updated usage
+ function() {
+ fileEntry.getMetadata(testStep.shift(), errorHandler);
+ },
+ function (metadata) {
+ chrome.test.assertTrue(metadata.size > 0);
+ testStep.shift()();
+ },
+ // Check global usage was updated
+ function() {
+ chrome.syncFileSystem.getUsageAndQuota('drive', testStep.shift());
+ },
+ function(storageInfo) {
+ usageAfterWrite = storageInfo.usage_bytes;
+ chrome.test.assertEq(testData.length, usageAfterWrite -
usageBeforeWrite);
+ chrome.test.succeed();
+ }
+];
+
+function errorHandler() {
+ chrome.test.fail();
+}
+
+chrome.test.runTests([
+ chrome.test.callbackPass(testStep[0])
+]);
+

[kin...@chromium.org]

lgtm


http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
(right):

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json#newcode9
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json:9:
"scripts": ["test.js"]
nit: It looks it originally comes from the file I've written
(request_file_system/manifest.json) but the indentation looks off?

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode14
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:14:
// Create empty file
nit: please end comments with '.' (here and below)

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode33
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:33:
fileEntry.createWriter(testStep.shift(), errorHandler);
nit: it might be intentional (if so it's ok) but we could also combine
this step and the one above

// Records the usage and creates a writer.
usageBeforeWrite = storageInfo.usage_bytes;
fileEntry.createWriter(...);

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode53
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:53:
chrome.test.assertTrue(metadata.size > 0);
...or assertEq with testData.length?
(metadata.size should only include the file content size so this should
match)

http://codereview.chromium.org/11368160/

[calv...@chromium.org]

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json
(right):

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json#newcode9
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/manifest.json:9:
"scripts": ["test.js"]

On 2012/11/09 07:24:49, kinuko wrote:
> nit: It looks it originally comes from the file I've written
> (request_file_system/manifest.json) but the indentation looks off?

Ok I changed all 4 manifest.json files under sync_file_system to use 2
spaces instead of 4 on the line with "scripts:"

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode14
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:14:
// Create empty file

On 2012/11/09 07:24:49, kinuko wrote:
> nit: please end comments with '.' (here and below)

Done.

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode33
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:33:
fileEntry.createWriter(testStep.shift(), errorHandler);

On 2012/11/09 07:24:49, kinuko wrote:
> nit: it might be intentional (if so it's ok) but we could also combine
this step
> and the one above

> // Records the usage and creates a writer.
> usageBeforeWrite = storageInfo.usage_bytes;
> fileEntry.createWriter(...);

This was intentional so that the comment // Write a known number of
bytes. appears before fileEntry.createWriter(); I thought this made the
test easier to read rather than trying to combine steps.

http://codereview.chromium.org/11368160/diff/2001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode53
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:53:
chrome.test.assertTrue(metadata.size > 0);

On 2012/11/09 07:24:49, kinuko wrote:
> ...or assertEq with testData.length?
> (metadata.size should only include the file content size so this
should match)

Done.

http://codereview.chromium.org/11368160/

[kin...@chromium.org]

lgtm


http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode19
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:19:
function(fe) {
nit: we often use a variable name 'entry' for {File,Directory}Entry.
Maybe more readable than 'fe'

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode39
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:39:

nit: since we have no empty lines between test steps I'd prefer not
having them either here (and line 43)

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode53
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:53:
chrome.test.assertEq(testData.length, metadata.size );
nit: extra space at the end (before ')')

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode61
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:61:
usageAfterWrite = storageInfo.usage_bytes;
nit: This variable doesn't seem really necessary in global namespace

http://codereview.chromium.org/11368160/

[calv...@chromium.org]

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode19
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:19:
function(fe) {

On 2012/11/09 09:11:49, kinuko wrote:
> nit: we often use a variable name 'entry' for {File,Directory}Entry.
Maybe more
> readable than 'fe'

Done.

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode39
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:39:

On 2012/11/09 09:11:49, kinuko wrote:
> nit: since we have no empty lines between test steps I'd prefer not
having them
> either here (and line 43)

Done.

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode53
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:53:
chrome.test.assertEq(testData.length, metadata.size );

On 2012/11/09 09:11:49, kinuko wrote:
> nit: extra space at the end (before ')')

Done.

http://codereview.chromium.org/11368160/diff/6001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode61
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:61:
usageAfterWrite = storageInfo.usage_bytes;

On 2012/11/09 09:11:49, kinuko wrote:
> nit: This variable doesn't seem really necessary in global namespace

Done.

http://codereview.chromium.org/11368160/

[benw...@chromium.org]

http://codereview.chromium.org/11368160/diff/13001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/13001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode69
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:69:
chrome.test.callbackPass(testStep[0])
Why are you using chrome.test.callbackPass here, this doesn't seem to be
wrapping a callback.

I have found tests can exit early if you use callbackPass and then use
other callbacks which do not go through chrome.test. Are you sure all of
your test steps are being run?

http://codereview.chromium.org/11368160/

[calv...@chromium.org]

http://codereview.chromium.org/11368160/diff/13001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
File
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js
(right):

http://codereview.chromium.org/11368160/diff/13001/chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js#newcode69
chrome/test/data/extensions/api_test/sync_file_system/write_file_then_get_usage/test.js:69:
chrome.test.callbackPass(testStep[0])

On 2012/11/10 10:22:56, benwells wrote:
> Why are you using chrome.test.callbackPass here, this doesn't seem to
be
> wrapping a callback.

> I have found tests can exit early if you use callbackPass and then use
other
> callbacks which do not go through chrome.test. Are you sure all of
your test
> steps are being run?

Sorry Ben. I'm kinda confused. When I was playing with this last week I
noticed that some of my tests were coming back as passing even if an
assertion failed using chrome.test.assertTrue or assertEq. Even if I
purposely put in chrome.test.fail(); lines in some steps the failure
message would get printed to console but the test would still say
passing.

After some confusion, I tried adding in the chrome.test.callbackPass and
that seemed to fix it. However, this morning when I tried to reproduce
the strange behavior I saw before I couldn't replicate it and now just
using testStep[0] on this line works again.

I'm honestly no sure what happened here so either I'm nuts or my machine
was in a weird state before. Anyway, it seems to be ok now so I've
removed the chrome.test.callbackPass() here now.

http://codereview.chromium.org/11368160/

[calv...@chromium.org]

Aww, I understand what I did wrong now. In my manual testing I kick off the
test
using testStep[0](); When I converted my manual test to the automated
version I
wrote:

chrome.test.runTests([
testStep[0]()
]);

instead of (left the brackets after testStep[0])
chrome.test.runTests([
testStep[0]
]);

It just happens that if you forgot to remove the last set of brackets the
test
still runs but never reports back failure.

http://codereview.chromium.org/11368160/

[benw...@chromium.org]

lgtm



http://codereview.chromium.org/11368160/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calv...@chromium.org/11368160/2004


https://chromiumcodereview.appspot.com/11368160/

[commi...@chromium.org]

Retried try job too often for step(s) browser_tests

https://chromiumcodereview.appspot.com/11368160/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/calv...@chromium.org/11368160/8006


https://chromiumcodereview.appspot.com/11368160/

[commi...@chromium.org]

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/calv...@chromium.org/11368160/5009


https://chromiumcodereview.appspot.com/11368160/

[commi...@chromium.org]

[commi...@chromium.org]

Change committed as 167392

https://chromiumcodereview.appspot.com/11368160/