Test cleanup: Using MockSpecialStoragePolicy instead of local subclasses of SpecialStoragePolicy.

Test cleanup: Using MockSpecialStoragePolicy instead of local subclasses of SpecialStoragePolicy.


BUG=NONE
TEST=Existing tests pass: FileSystemDirURLRequestJobTest.*, FileSystemURLRequestJobTest.*, FileSystemContextTest.*, DatabaseTrackerTest.*, QuotaFileUtilTest.*, FileSystemPathManagerTest.*, SandboxMountPointProviderMigrationTest.*


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

[marja, 2011-08-12 08:55:18]

Hi Michael,

Here's the small test cleanup we discussed yesterday. Please review.

[michaeln, 2011-08-12 18:53:52]

thnx for following up on this!

http://codereview.chromium.org/7633016/diff/3001/webkit/database/database_tra...
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/7633016/diff/3001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:608: static
scoped_refptr<quota::MockSpecialStoragePolicy> special_storage_policy_;
i would expect valgrind to complain about leaking this object. i think you can
remove the global and create a new one per test

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:51: return true;
The old impl claimed all origins were unlimited (and were also fileHandlers)...
the new impl will return false all. I guess the tests don't depend on the 'true'
return values? Is that right?

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_t...
File webkit/fileapi/file_system_test_helper.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_t...
webkit/fileapi/file_system_test_helper.cc:32: return unlimited_quota_;
same question here, in some cases the test logic is explicit about a true return
value... now it will always get a false return value... seems like this changes
an aspect of the tests that may make a difference?

maybe the 'mock' impl could use a mode that return a canned value regardless of
the origin being asked about? that way we could use it in these tests w/o
changing the values seen when conductiing those tests?

[marja, 2011-08-16 10:54:25]

Hi,

Thanks for comments.

http://codereview.chromium.org/7633016/diff/3001/webkit/database/database_tra...
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/7633016/diff/3001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:608: static
scoped_refptr<quota::MockSpecialStoragePolicy> special_storage_policy_;
On 2011/08/12 18:53:52, michaeln wrote:
> i would expect valgrind to complain about leaking this object. i think you can
> remove the global and create a new one per test

Hmm, why would valgrind complain: the dtor of the scoped_refptr is ran before
the program exits.

Anyway, probably it's more elegant to remove the global as you suggested, so I
did that.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:51: return true;
On 2011/08/12 18:53:52, michaeln wrote:
> The old impl claimed all origins were unlimited (and were also
fileHandlers)...
> the new impl will return false all. I guess the tests don't depend on the
'true'
> return values? Is that right?

IsStorageUnlimited and IsFileHandler in this file were never called when
executing FileSystemDirURLRequestJobTest.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager_unittest.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager_unittest.cc:214: virtual bool
IsStorageUnlimited(const GURL& origin) {
IsStorageUnlimited & IsFileHandler here were never called when executing
FileSystemPathManagerTest, either.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_t...
File webkit/fileapi/file_system_test_helper.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_t...
webkit/fileapi/file_system_test_helper.cc:32: return unlimited_quota_;
On 2011/08/12 18:53:52, michaeln wrote:
> same question here, in some cases the test logic is explicit about a true
return
> value... now it will always get a false return value... seems like this
changes
> an aspect of the tests that may make a difference?
> 
> maybe the 'mock' impl could use a mode that return a canned value regardless
of
> the origin being asked about? that way we could use it in these tests w/o
> changing the values seen when conductiing those tests?

Done.

I thought it's FileSystemTestOriginHelper is never used with unlimited quota,
but I had overlooked FileWriterDelegateUnlimitedTest in unit_tests. I added the
"all unlimited" mode to the MockSpecialStoragePolicy, and made
FileSystemTestOriginHelper to set it.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_t...
webkit/fileapi/file_system_test_helper.cc:35: virtual bool IsFileHandler(const
std::string& extension_id) {
And this was never called in unit_tests or test_shell_tests, so the "false"
return value of MockSpecialStoragePolicy shouldn't be a problem.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_url_request_job_unittest.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_url_request_job_unittest.cc:69: virtual bool
IsStorageUnlimited(const GURL& origin) {
IsStorageUnlimited & IsFileHandler in this file were never called when executing
FileSystemDirURLRequestJobTest, either.

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (left):

http://codereview.chromium.org/7633016/diff/3001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:166: new
TestSpecialStoragePolicy(true /* unlimited quota */),
I changed this to use the "all unlimited" mode of MockSpecialStoragePolicy, too.

[michaeln, 2011-08-16 16:36:01]

LGTM, thanx!

[commit-bot: I haz the power, 2011-08-17 09:23:05]

Change committed as 97119