Adding browsertest to verify that updating forced-installed extensions is possible.

Adding browsertest to verify that updating forced-installed extensions is possible.

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

[csorba, 2013-05-27 18:36:23]

Please check (only if there are many green tests :P )

[Joao da Silva, 2013-05-28 08:27:31]

The test is failing on the bots.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
File chrome/browser/policy/cloud/test_request_interceptor.cc (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.cc:61: LOG(ERROR) <<
file_path.value();
Remove this.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.cc:65: file_path);
it: indent (+4 spaces)

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
File chrome/browser/policy/cloud/test_request_interceptor.h (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.h:12: #include
"base/files/file_path.h"
FilePath can be forward declared:

namespace base {
class FilePath;
}

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.h:61: static JobCallback
MockJob(const base::FilePath& file_path);
This JobCallback will serve the contents of |file_path|. So a better name is
"FileJob" or "SendFileJob", and the comment should say that it just sends the
contents of the file back.

The URLRequestMockHTTPJob is an implementation detail, and doesn't have to be
mentioned here.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:162:
FILE_PATH_LITERAL("good_v1_update_manifest.xml");
Use consistent names for the files: "good_v1_*" and "good_v2_*"

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1350:
PathService::Get(chrome::DIR_TEST_DATA, &test_path);
ASSERT_TRUE( ... )

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1352:
scoped_ptr<TestRequestInterceptor> interceptor;
No need to have this in a scoped_ptr, just use a value directly:

TestRequestInterceptor interceptor("update.extension");

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1369:
service->GetExtensionById(kGoodCrxId, true)->version();
nit: indent

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1371:
EXPECT_FALSE(new_version->IsOlderThan(old_version_number));
This would pass even if new_version == old_version_number. It should be

base::Version old_version(old_version_number);
ASSERT_TRUE(old_version.IsValid());
EXPECT_EQ(1, new_version->CompareTo(old_version));

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1372: }
Verify that the expected callback jobs were used before ending the test:

EXPECT_EQ(0u, interceptor.GetPendingSize());

[csorba, 2013-05-28 11:49:58]

Please check after the corrections.

[Joao da Silva, 2013-05-28 11:59:34]

lgtm

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
File chrome/browser/policy/cloud/test_request_interceptor.h (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.h:12: #include
"base/files/file_path.h"
On 2013/05/28 08:27:31, Joao da Silva wrote:
> FilePath can be forward declared:
> 
> namespace base {
> class FilePath;
> }

Done.

https://codereview.chromium.org/16102004/diff/10001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/test_request_interceptor.h (right):

https://codereview.chromium.org/16102004/diff/10001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/test_request_interceptor.h:63: // Returns a
JobCallback that will sends the contents of |file_path|.
nit: sends -> send

[csorba, 2013-05-28 12:00:39]

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
File chrome/browser/policy/cloud/test_request_interceptor.cc (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.cc:61: LOG(ERROR) <<
file_path.value();
On 2013/05/28 08:27:31, Joao da Silva wrote:
> Remove this.

Done.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.cc:65: file_path);
On 2013/05/28 08:27:31, Joao da Silva wrote:
> it: indent (+4 spaces)

Done.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
File chrome/browser/policy/cloud/test_request_interceptor.h (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.h:12: #include
"base/files/file_path.h"
On 2013/05/28 08:27:31, Joao da Silva wrote:
> FilePath can be forward declared:
> 
> namespace base {
> class FilePath;
> }

Done.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/cloud/t...
chrome/browser/policy/cloud/test_request_interceptor.h:61: static JobCallback
MockJob(const base::FilePath& file_path);
On 2013/05/28 08:27:31, Joao da Silva wrote:
> This JobCallback will serve the contents of |file_path|. So a better name is
> "FileJob" or "SendFileJob", and the comment should say that it just sends the
> contents of the file back.
> 
> The URLRequestMockHTTPJob is an implementation detail, and doesn't have to be
> mentioned here.

Done. (Choosed FileJob)

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:162:
FILE_PATH_LITERAL("good_v1_update_manifest.xml");
On 2013/05/28 08:27:31, Joao da Silva wrote:
> Use consistent names for the files: "good_v1_*" and "good_v2_*"

I tried to introduce as few new crx files as possible, so I use good2.crx
together with a slightly modified version of good.crx (good_v1.crx) which
contains an "update_url".

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1350:
PathService::Get(chrome::DIR_TEST_DATA, &test_path);
On 2013/05/28 08:27:31, Joao da Silva wrote:
> ASSERT_TRUE( ... )

Done.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1369:
service->GetExtensionById(kGoodCrxId, true)->version();
On 2013/05/28 08:27:31, Joao da Silva wrote:
> nit: indent

Done.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1371:
EXPECT_FALSE(new_version->IsOlderThan(old_version_number));
On 2013/05/28 08:27:31, Joao da Silva wrote:
> This would pass even if new_version == old_version_number. It should be
> 
> base::Version old_version(old_version_number);
> ASSERT_TRUE(old_version.IsValid());
> EXPECT_EQ(1, new_version->CompareTo(old_version));

Done + ASSERT_TRUE(new_version->IsValid()) added.

https://codereview.chromium.org/16102004/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_browsertest.cc:1372: }
On 2013/05/28 08:27:31, Joao da Silva wrote:
> Verify that the expected callback jobs were used before ending the test:
> 
> EXPECT_EQ(0u, interceptor.GetPendingSize());

Done.

[csorba, 2013-05-28 12:07:25]

https://codereview.chromium.org/16102004/diff/10001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/test_request_interceptor.h (right):

https://codereview.chromium.org/16102004/diff/10001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/test_request_interceptor.h:63: // Returns a
JobCallback that will sends the contents of |file_path|.
On 2013/05/28 11:59:35, Joao da Silva wrote:
> nit: sends -> send

Done.

[commit-bot: I haz the power, 2013-05-28 18:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csorba@chromium.org/16102004/17001

[commit-bot: I haz the power, 2013-05-28 19:33:38]

Change committed as 202624