Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
436903002:
Add tests for Extension installation/update for supervised users. (Closed)
|
Created:
6 years, 4 months ago by Marc Treib Modified:
6 years, 4 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, asargent_no_longer_on_chrome, Yoyo Zhou, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd tests for Extension installation/update for supervised users.
BUG=390520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287515
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 26
Patch Set 3 : review comments #Messages
Total messages: 7 (0 generated)
PTAL!
thanks for adding these! https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:780: void PackCRXAndUpdateExtension(const std::string& id, nit: Crx seems more common in this file, not CRX https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:784: base::FilePath crx_path; nit: initialize this directly on line 787? https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6134: const Extension* extensions[2] = { nit: the [2] should be unnecessary https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6136: InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN) it would be nicer if instead of using InstallCRX directly you could create the sync operations which install the supervised extensions. you already do that below for uninstall. same for other tests where appropriate. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6143: EXPECT_TRUE(service()->GetExtensionById(extensions[1]->id(), false)); use registry()->enabled_extensions()->Contains(extensions[1]->id()); https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6164: ASSERT_TRUE(service()->GetExtensionById(extension->id(), false)); registry()... https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6176: extension = service()->GetExtensionById(id, false); registry()... https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6200: ASSERT_TRUE(service()->GetExtensionById(extension->id(), false)); registry() https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6212: EXPECT_FALSE(service()->GetExtensionById(id, false)); registry() x2 https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6214: EXPECT_TRUE(extension); should be an ASSERT_TRUE really since the line below expects to see it. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6233: const Extension* extensions[2] = { shouldn't need the [2] here either https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6245: for (int i = 0; i < 2; i++) { arraysize(extensions)
https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:780: void PackCRXAndUpdateExtension(const std::string& id, On 2014/08/01 17:09:15, kalman wrote: > nit: Crx seems more common in this file, not CRX Hm not really: all the PackCRX, InstallCRX etc functions use all-caps, so overall it's pretty inconsistent. But if you want, I can replace all "CRX" with "Crx" (or the other way around)? https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:784: base::FilePath crx_path; On 2014/08/01 17:09:15, kalman wrote: > nit: initialize this directly on line 787? Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6134: const Extension* extensions[2] = { On 2014/08/01 17:09:16, kalman wrote: > nit: the [2] should be unnecessary Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6136: InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN) On 2014/08/01 17:09:15, kalman wrote: > it would be nicer if instead of using InstallCRX directly you could create the > sync operations which install the supervised extensions. you already do that > below for uninstall. > > same for other tests where appropriate. Can I still make it use a local crx file then? Usually it would try to download the extension from the web store. No other test in this file uses sync changes to install extensions, only to uninstall or to update settings. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6143: EXPECT_TRUE(service()->GetExtensionById(extensions[1]->id(), false)); On 2014/08/01 17:09:15, kalman wrote: > use registry()->enabled_extensions()->Contains(extensions[1]->id()); Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6164: ASSERT_TRUE(service()->GetExtensionById(extension->id(), false)); On 2014/08/01 17:09:15, kalman wrote: > registry()... Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6176: extension = service()->GetExtensionById(id, false); On 2014/08/01 17:09:15, kalman wrote: > registry()... Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6200: ASSERT_TRUE(service()->GetExtensionById(extension->id(), false)); On 2014/08/01 17:09:15, kalman wrote: > registry() Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6212: EXPECT_FALSE(service()->GetExtensionById(id, false)); On 2014/08/01 17:09:15, kalman wrote: > registry() x2 Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6214: EXPECT_TRUE(extension); On 2014/08/01 17:09:15, kalman wrote: > should be an ASSERT_TRUE really since the line below expects to see it. Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6233: const Extension* extensions[2] = { On 2014/08/01 17:09:15, kalman wrote: > shouldn't need the [2] here either Done. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6245: for (int i = 0; i < 2; i++) { On 2014/08/01 17:09:16, kalman wrote: > arraysize(extensions) Done.
lgtm. CC some people about the sync test issue if any of you are curious. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:780: void PackCRXAndUpdateExtension(const std::string& id, On 2014/08/04 08:38:12, Marc Treib wrote: > On 2014/08/01 17:09:15, kalman wrote: > > nit: Crx seems more common in this file, not CRX > > Hm not really: all the PackCRX, InstallCRX etc functions use all-caps, so > overall it's pretty inconsistent. But if you want, I can replace all "CRX" with > "Crx" (or the other way around)? eh, good point. don't worry about it. https://codereview.chromium.org/436903002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_service_unittest.cc:6136: InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN) On 2014/08/04 08:38:12, Marc Treib wrote: > On 2014/08/01 17:09:15, kalman wrote: > > it would be nicer if instead of using InstallCRX directly you could create the > > sync operations which install the supervised extensions. you already do that > > below for uninstall. > > > > same for other tests where appropriate. > > Can I still make it use a local crx file then? Usually it would try to download > the extension from the web store. > No other test in this file uses sync changes to install extensions, only to > uninstall or to update settings. Yeah it doesn't look like we really test basic sync installation anywhere. There are a bunch of tests which touch the sides but nothing which truly tests the flow. Bummer :( Maybe one day... one day... we'll be able to properly test this stuff. Oh well.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/436903002/40001
Message was sent while issue was closed.
Change committed as 287515 |
