New issue
Advanced search Search tips

Issue 667587 link

Starred by 2 users
Status: Fixed
Owner:
rdevlin....@chromium.org
Closed: Dec 21
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Need common extension loading test util for use across unit tests, browser tests

Project Member Reported by rdevlin....@chromium.org, Nov 22 2016 
We have a bunch of ways to load extensions in tests.  Too many.  It makes it hard to reason about and nearly impossible to remember which variation you want to use, and also hard to make changes to them.  We should instead (to the extent possible) have a single canonical way of loading extensions that can be used across unit tests and browser tests.  Additionally, we should have it able to be used as a separate object (composition over inheritance, yay!) to prevent future double-inheritance like [1].

Converting ExtensionBrowserTest and ExtensionServiceTestBase to use whatever the common implementation is would be a good start.

[1] https://chromium.googlesource.com/chromium/src/+/50bf87a5fa8874cd37a334e402c6931f55641335/chrome/browser/prerender/prerender_browsertest.cc#2642
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 28 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11f01686d8b1d4eb46f2bf6a76fecc012b518050

commit 11f01686d8b1d4eb46f2bf6a76fecc012b518050
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Mon Nov 28 18:15:52 2016

[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 

Review-Url: https://codereview.chromium.org/2524553002
Cr-Commit-Position: refs/heads/master@{#434692}

[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/BUILD.gn
[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/chrome_extension_test_notification_observer.cc
[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/chrome_extension_test_notification_observer.h
[add] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/chrome_test_extension_loader.cc
[add] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/chrome_test_extension_loader.h
[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/browser/extensions/extension_service_test_with_install.cc
[modify] https://crrev.com/11f01686d8b1d4eb46f2bf6a76fecc012b518050/chrome/test/BUILD.gn
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 5 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55f2b623d3cb915156f24f7efdd09e3b64bd43e4

commit 55f2b623d3cb915156f24f7efdd09e3b64bd43e4
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Mon Dec 05 21:25:53 2016

[Extensions WebUI] Clean up ExtensionSettingsUIBrowserTest

Clean up ExtensionSettingsUIBrowserTest. Most notably, use the utility
class ChromeTestExtensionLoader() rather than re-implementing the load
extension functionality. Also remove some unneeded members, includes,
complexity, etc.

BUG= 667587 

Review-Url: https://codereview.chromium.org/2536903002
Cr-Commit-Position: refs/heads/master@{#436411}

[modify] https://crrev.com/55f2b623d3cb915156f24f7efdd09e3b64bd43e4/chrome/browser/extensions/chrome_test_extension_loader.cc
[modify] https://crrev.com/55f2b623d3cb915156f24f7efdd09e3b64bd43e4/chrome/browser/ui/webui/extensions/extension_settings_browsertest.cc
[modify] https://crrev.com/55f2b623d3cb915156f24f7efdd09e3b64bd43e4/chrome/browser/ui/webui/extensions/extension_settings_browsertest.h
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d811f38dc4a3cff661f750825679bc1ef7e6072

commit 6d811f38dc4a3cff661f750825679bc1ef7e6072
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Sat Oct 06 02:58:35 2018

[Extensions Cleanup] Use ChromeTestExtensionLoader in developerPrivate tests

ChromeTestExtensionLoader is designed to load extensions from disk for
testing purposes, and abstracts out a lot of the nasty process. Use it
in developer_private_unittest.cc to simplify the code.

As a drive-by, also get rid of some other unnecessary includes.

Bug:  667587 
Change-Id: If47abca4faca6231bb2fc95ce38fbed5edf677e2
Reviewed-on: https://chromium-review.googlesource.com/c/1265086
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597401}
[modify] https://crrev.com/6d811f38dc4a3cff661f750825679bc1ef7e6072/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc

Comment 4 by rdevlin....@chromium.org, Dec 21

Status: Fixed (was: Assigned)
The loading utility exists and is used in many places; closing this out.
►

Sign in to add a comment