Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524553002:
[Extensions] Create ChromeTestExtensionLoader (Closed)
Description[Extensions] Create ChromeTestExtensionLoader
Create a ChromeTestExtensionLoader to load extensions during tests.
This class is designed to be used across browser tests and unit tests,
making it a (mostly) solution to needing to re-code the same
implementation in various different test classes, as well as an
alternative to the double-inheritance we sometimes see in order to get
extension-loading helper methods in addition to some other test
infrastructure.
The main class includes options to modify the behavior according to most
common flags set by existing (unit|browser) tests, such as error
handling, packing, creation flags, permissions, etc.
Additionally, hook the class up to the existing ExtensionBrowserTest
(browser test) and ExtensionServiceTestWithInstall (unit test) as a
proof of concept that it works in both unit and browser tests. Going
forward, we should continue to a) expand the test loader and b) replace
current duplicative code.
BUG=667587
Committed: https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050
Cr-Commit-Position: refs/heads/master@{#434692}
Patch Set 1 : . #
Total comments: 14
Patch Set 2 : lazyboy's #Patch Set 3 : compile #
Total comments: 3
Messages
Total messages: 60 (47 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Create extensionloader BUG= ========== to ========== [Extensions] Create ChromeTestExtensionLoader Create a ChromeTestExtensionLoader to load extensions during tests. This class is designed to be used across browser tests and unit tests, making it a (mostly) solution to needing to re-code the same implementation in various different test classes, as well as an alternative to the double-inheritance we sometimes see in order to get extension-loading helper methods in addition to some other test infrastructure. The main class includes options to modify the behavior according to most common flags set by existing (unit|browser) tests, such as error handling, packing, creation flags, permissions, etc. Additionally, hook the class up to the existing ExtensionBrowserTest (browser test) and ExtensionServiceTestWithInstall (unit test) as a proof of concept that it works in both unit and browser tests. Going forward, we should continue to a) expand the test loader and b) replace current duplicative code. BUG=667587 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy@, mind taking a look? There's more that can use this, but I wanted to keep this cl fairly targeted to start, and then expand. But it still includes both unit and browser tests to ensure it satisfies our criteria.
https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:68: extension_service_->ReloadExtension(extension_id_); Add a note on why reload is necessary. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:87: const Extension& extension) { Remove |extension| param since it isn't really waiting for a single extension load to complete, rather it's waiting for *all* https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:101: ADD_FAILURE() << "Crx path exists: " << crx_path.value(); This probably means same ChromeTestExtensionLoader is being used to create more than one extension? Maybe log that in the message to give hint to consumers of this class? https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:129: DCHECK(base::PathExists(crx_path)); CHECK is better? https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:144: // Do something with ^^ Should this become a TODO? https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.h (right): https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.h:27: class ExtensionSystem; nit: sort https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.h:83: // crx. Note that the created crx is tied to this object's lifetime. nit: s/this object's lifetime/lifetime of |this|
https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:68: extension_service_->ReloadExtension(extension_id_); On 2016/11/23 21:27:56, lazyboy wrote: > Add a note on why reload is necessary. Done. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:87: const Extension& extension) { On 2016/11/23 21:27:56, lazyboy wrote: > Remove |extension| param since it isn't really waiting for a single extension > load to complete, rather it's waiting for *all* Done. I was thinking this might be expanded a bit to do more extension-specific logic, but we can wait 'til that happens. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:101: ADD_FAILURE() << "Crx path exists: " << crx_path.value(); On 2016/11/23 21:27:57, lazyboy wrote: > This probably means same ChromeTestExtensionLoader is being used to create more > than one extension? Maybe log that in the message to give hint to consumers of > this class? Done. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:129: DCHECK(base::PathExists(crx_path)); On 2016/11/23 21:27:56, lazyboy wrote: > CHECK is better? Practically, I'm not sure how much of a difference it makes, since release bots build with dchecks, but changed to be safe. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:144: // Do something with ^^ On 2016/11/23 21:27:56, lazyboy wrote: > Should this become a TODO? Whoops! Done. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.h (right): https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.h:27: class ExtensionSystem; On 2016/11/23 21:27:57, lazyboy wrote: > nit: sort Done. https://codereview.chromium.org/2524553002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.h:83: // crx. Note that the created crx is tied to this object's lifetime. On 2016/11/23 21:27:57, lazyboy wrote: > nit: > s/this object's lifetime/lifetime of |this| Done.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, one more q, didn't notice this before. https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:71: registry_observer.WaitForExtensionLoaded(); Here and below when we call WaitForExtensionLoaded(): Should we inspect the returned Extension*'s id and compare it with extension_id_? Or, (as the previous code used to) check with ExtensionService that |extension_id_| is indeed still in the enabled extensions: i.e. similar to ExtensionBrowserTest::LoadExtensionWithInstallParam(): extension = service->GetExtensionById(extension_id_, false); CHECK(extension) << extension_id_ << " not found after reloading."; Just wanted to make sure you didn't unintentionally left them out.
https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:71: registry_observer.WaitForExtensionLoaded(); On 2016/11/24 00:30:22, lazyboy wrote: > Here and below when we call WaitForExtensionLoaded(): > Should we inspect the returned Extension*'s id and compare it with > extension_id_? > > Or, (as the previous code used to) check with ExtensionService that > |extension_id_| is indeed still in the enabled extensions: > > i.e. similar to ExtensionBrowserTest::LoadExtensionWithInstallParam(): > extension = service->GetExtensionById(extension_id_, false); > CHECK(extension) << extension_id_ << " not found after reloading."; > > Just wanted to make sure you didn't unintentionally left them out. TestExtensionRegistryObserver will wait for an extension with the given id (passed to it in the ctor) before returning, and a loaded extension will always be enabled AFAIK. So I don't *think* there's no way to get to this point without having loaded an extension with the proper id into the enabled set of extensions.
lgtm https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2524553002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_test_extension_loader.cc:71: registry_observer.WaitForExtensionLoaded(); On 2016/11/24 00:35:30, Devlin wrote: > On 2016/11/24 00:30:22, lazyboy wrote: > > Here and below when we call WaitForExtensionLoaded(): > > Should we inspect the returned Extension*'s id and compare it with > > extension_id_? > > > > Or, (as the previous code used to) check with ExtensionService that > > |extension_id_| is indeed still in the enabled extensions: > > > > i.e. similar to ExtensionBrowserTest::LoadExtensionWithInstallParam(): > > extension = service->GetExtensionById(extension_id_, false); > > CHECK(extension) << extension_id_ << " not found after reloading."; > > > > Just wanted to make sure you didn't unintentionally left them out. > > TestExtensionRegistryObserver will wait for an extension with the given id > (passed to it in the ctor) before returning, and a loaded extension will always > be enabled AFAIK. So I don't *think* there's no way to get to this point > without having loaded an extension with the proper id into the enabled set of > extensions. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480352810121980,
"parent_rev": "1e34e8e21bede5eb19077695ca5032981aad3db9", "commit_rev":
"545d17dafaa475f08e4c3841ea8acfcde7cecf89"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Create ChromeTestExtensionLoader Create a ChromeTestExtensionLoader to load extensions during tests. This class is designed to be used across browser tests and unit tests, making it a (mostly) solution to needing to re-code the same implementation in various different test classes, as well as an alternative to the double-inheritance we sometimes see in order to get extension-loading helper methods in addition to some other test infrastructure. The main class includes options to modify the behavior according to most common flags set by existing (unit|browser) tests, such as error handling, packing, creation flags, permissions, etc. Additionally, hook the class up to the existing ExtensionBrowserTest (browser test) and ExtensionServiceTestWithInstall (unit test) as a proof of concept that it works in both unit and browser tests. Going forward, we should continue to a) expand the test loader and b) replace current duplicative code. BUG=667587 ========== to ========== [Extensions] Create ChromeTestExtensionLoader Create a ChromeTestExtensionLoader to load extensions during tests. This class is designed to be used across browser tests and unit tests, making it a (mostly) solution to needing to re-code the same implementation in various different test classes, as well as an alternative to the double-inheritance we sometimes see in order to get extension-loading helper methods in addition to some other test infrastructure. The main class includes options to modify the behavior according to most common flags set by existing (unit|browser) tests, such as error handling, packing, creation flags, permissions, etc. Additionally, hook the class up to the existing ExtensionBrowserTest (browser test) and ExtensionServiceTestWithInstall (unit test) as a proof of concept that it works in both unit and browser tests. Going forward, we should continue to a) expand the test loader and b) replace current duplicative code. BUG=667587 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Create ChromeTestExtensionLoader Create a ChromeTestExtensionLoader to load extensions during tests. This class is designed to be used across browser tests and unit tests, making it a (mostly) solution to needing to re-code the same implementation in various different test classes, as well as an alternative to the double-inheritance we sometimes see in order to get extension-loading helper methods in addition to some other test infrastructure. The main class includes options to modify the behavior according to most common flags set by existing (unit|browser) tests, such as error handling, packing, creation flags, permissions, etc. Additionally, hook the class up to the existing ExtensionBrowserTest (browser test) and ExtensionServiceTestWithInstall (unit test) as a proof of concept that it works in both unit and browser tests. Going forward, we should continue to a) expand the test loader and b) replace current duplicative code. BUG=667587 ========== to ========== [Extensions] Create ChromeTestExtensionLoader Create a ChromeTestExtensionLoader to load extensions during tests. This class is designed to be used across browser tests and unit tests, making it a (mostly) solution to needing to re-code the same implementation in various different test classes, as well as an alternative to the double-inheritance we sometimes see in order to get extension-loading helper methods in addition to some other test infrastructure. The main class includes options to modify the behavior according to most common flags set by existing (unit|browser) tests, such as error handling, packing, creation flags, permissions, etc. Additionally, hook the class up to the existing ExtensionBrowserTest (browser test) and ExtensionServiceTestWithInstall (unit test) as a proof of concept that it works in both unit and browser tests. Going forward, we should continue to a) expand the test loader and b) replace current duplicative code. BUG=667587 Committed: https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050 Cr-Commit-Position: refs/heads/master@{#434692} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050 Cr-Commit-Position: refs/heads/master@{#434692} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
