Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2076583002:
Add WebAPK's tests in ChildProcessLauncherTest. (Closed)
DescriptionAdd WebAPK's tests in ChildProcessLauncherTest.
Instrument tests are added in ChildProcessLauncherTest to connect to
child processes in a WebAPK:
- Move ChildProcessLauncherTest from //content to //chrome, because
the WebAPKs instruments tests require to pre-install a WebAPK.
- A refactoring is done to build a test WebAPK. This is because
WebAPK needs to load its host browser's code to create a renderer,
we hard code the host browser package name in the test WebAPK
be the package of chrome_public_apk.
BUG=609122
Committed: https://crrev.com/4b7e057321503b5f833156ee2b26ba85e0384ddd
Cr-Commit-Position: refs/heads/master@{#402046}
Patch Set 1 #
Total comments: 10
Patch Set 2 : pkotwicz@'s comments. #Patch Set 3 : Move ChildProcessLaucherTest back to content. #
Total comments: 3
Patch Set 4 : Move more functions to the helper class. #
Total comments: 3
Patch Set 5 : Move all tests to //content. #
Total comments: 2
Patch Set 6 : Use "external apk" to replace "webapk". #Patch Set 7 : Fix test failures. #
Messages
Total messages: 51 (21 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. This CL includes: - Adds instrument tests in ChildProcessLauncherTest to connect to child processes in a WebAPK. - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. BUG=609122 ==========
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. This CL includes: - Adds instrument tests in ChildProcessLauncherTest to connect to child processes in a WebAPK. - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a testing WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the testing WebAPK be the package of chrome_public_apk. BUG=609122 ==========
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a testing WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the testing WebAPK be the package of chrome_public_apk. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a testing WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the testing WebAPK be the package of chrome_public_apk. BUG=609122 ==========
Patchset #1 (id:20001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, this CL depends on CL: https://codereview.chromium.org/2049843004/ Could you please take a look? Thanks!
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, becuase the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a testing WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the testing WebAPK be the package of chrome_public_apk. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, because the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a test WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the test WebAPK be the package of chrome_public_apk. BUG=609122 ==========
Just a few comments about the BUILD.gn file https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:28: "org.chromium.webapk.$webapk_manifest_package_origin.test" Don't you need to hard code test_shell_apk_manifest_package? Won't your test fail for someone who specified a custom webapk_manifest_package_origin https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:49: Nit: remove the new line https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:56: Nit: remove the new line https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:104: # chrome_public_test_apk, and its runtime host package is "org.chromium.chrome". How about: "The |test_webapk| target is used for WebAPK's instrumentation tests in the target chrome_public_test_apk. |test_webapk|'s package name and runtime host are set to be compatible with the instrumentation tests and are not customizable." I think that mentioning that test_webapk's package name and runtime host are not customizable is important. That's the reason why there are two APKs: webapk and test_webapk https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:108: apk_name = "WebApk.$webapk_manifest_package_origin.test" Can you hard code apk_name? TestWebApk maybe?
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:28: "org.chromium.webapk.$webapk_manifest_package_origin.test" On 2016/06/20 19:14:43, pkotwicz wrote: > Don't you need to hard code test_shell_apk_manifest_package? Won't your test > fail for someone who specified a custom webapk_manifest_package_origin Good point. Updated. https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:49: On 2016/06/20 19:14:43, pkotwicz wrote: > Nit: remove the new line Done. https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:56: On 2016/06/20 19:14:43, pkotwicz wrote: > Nit: remove the new line Done. https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:104: # chrome_public_test_apk, and its runtime host package is "org.chromium.chrome". On 2016/06/20 19:14:43, pkotwicz wrote: > How about: "The |test_webapk| target is used for WebAPK's instrumentation tests > in the target chrome_public_test_apk. |test_webapk|'s package name and runtime > host are set to be compatible with the instrumentation tests and are not > customizable." > > I think that mentioning that test_webapk's package name and runtime host are not > customizable is important. That's the reason why there are two APKs: webapk and > test_webapk Sounds good to me. https://codereview.chromium.org/2076583002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:108: apk_name = "WebApk.$webapk_manifest_package_origin.test" On 2016/06/20 19:14:43, pkotwicz wrote: > Can you hard code apk_name? TestWebApk maybe? Done.
LGTM!
hanxi@chromium.org changed reviewers: + mariakhomenko@chromium.org
Hi Maria, This CL depends on CL (https://codereview.chromium.org/2049843004/). Could you please take a look? Thanks!
On 2016/06/20 22:25:55, Xi Han wrote: > Hi Maria, > This CL depends on CL (https://codereview.chromium.org/2049843004/). > Could you please take a look? Thanks! Before I look at details, it seems odd to move the test out of content/. Could you subclass the test instead and have just the webapp test cases in chrome/ and leave the existing ones in content? That way maybe you won't need to expose methods to be public either.
On 2016/06/20 22:50:31, Maria wrote: > On 2016/06/20 22:25:55, Xi Han wrote: > > Hi Maria, > > This CL depends on CL (https://codereview.chromium.org/2049843004/). > > Could you please take a look? Thanks! > > Before I look at details, it seems odd to move the test out of content/. Could > you subclass the test instead and have just the webapp test cases in chrome/ and > leave the existing ones in content? That way maybe you won't need to expose > methods to be public either. Agree, I tried, but chrome browser tests don't have dependency on content tests. Is it okay to introduce such dependency?
Good question. I am not actually sure. Adding John here for an opinion of how to structure this correctly.
On 2016/06/20 23:02:54, Maria wrote: > Good question. I am not actually sure. Adding John here for an opinion of how to > structure this correctly. The existing test definitely shouldn't move out of content/. Instead, you should: - add your two tests to a new test class in //chrome - pull whatever you're using in that test class that's currently in ChildProcessLauncherTest (startConnection and allocatedChromeSandboxedConnectionCount, afaict) into a helper class in //content/public/test/android/:content_java_test_support. You'll probably have to add either an Instrumentation or a Context parameter to those methods. - make both the existing ChildProcessLauncherTest as well as your new test class in //chrome use that helper
It might be easier to create a test app - external_child_process_service_apk and add it to content_shell_test_apk's additional_apks. external_child_process_service_apk would have a stub service which implements IChildProcessService.
On 2016/06/21 13:04:59, pkotwicz wrote: > It might be easier to create a test app - external_child_process_service_apk and > add it to content_shell_test_apk's additional_apks. > external_child_process_service_apk would have a stub service which implements > IChildProcessService. Thank you John and Peter for the advice. I will try John's first, and it doesn't seem too many changes involved.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Hi Maria, I moved the ChildProcessLauncherTest back to content, and introduce subclass ChromeChildProcessLauncherTest. PTAL, thanks! https://codereview.chromium.org/2076583002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2076583002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:252: CriteriaHelper.pollInstrumentationThread( Tests failed if the "CriteriaHelper.pollInstrumentationThread(..)" block was moved to the helper class, so I keep the entire startConnection() in the (Chrome)ChildProcessLauncherTest.
https://codereview.chromium.org/2076583002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2076583002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:252: CriteriaHelper.pollInstrumentationThread( On 2016/06/21 18:26:58, Xi Han wrote: > Tests failed if the "CriteriaHelper.pollInstrumentationThread(..)" block was > moved to the helper class, so I keep the entire startConnection() in the > (Chrome)ChildProcessLauncherTest. Did ChildProcessLauncherTest failed or ChromeChildProcessLauncherTest failed? I find this very odd and I'd like to understand why that would be the case... Were you passing the context into the helper?
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Hi Maria, the failures are resolved and more functions are moved to the helper class. PTAL, thanks! https://codereview.chromium.org/2076583002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2076583002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:252: CriteriaHelper.pollInstrumentationThread( On 2016/06/21 21:32:12, Maria wrote: > On 2016/06/21 18:26:58, Xi Han wrote: > > Tests failed if the "CriteriaHelper.pollInstrumentationThread(..)" block was > > moved to the helper class, so I keep the entire startConnection() in the > > (Chrome)ChildProcessLauncherTest. > > Did ChildProcessLauncherTest failed or ChromeChildProcessLauncherTest failed? I > find this very odd and I'd like to understand why that would be the case... Were > you passing the context into the helper? The ChildProcessLauncherTest failed and I did pass the context to the helper. I moved the startConnection() to the helper class and tried again and the tests don't fail. Maybe my testing device wasn't in a good condition. Anyway, it is clean to move this function to the helper class.
lgtm
hanxi@chromium.org changed reviewers: + sievers@chromium.org
Hi Daniel: as promised, we added webAPKs tests in ChildProcessLauncherTest. This CL depends on CL (https://codereview.chromium.org/2049843004/), which is under a discussion with WebView Team and will sent to you to review soon. Please take a look, thanks!
https://codereview.chromium.org/2076583002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2076583002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: public static ChildProcessConnection allocateBoundConnectionForTesting(Context context, Can we maybe expose this through the test helper instead? Then ChildProcessLauncher doesn't have to be in the content-api, see crbug.com/617313.
https://codereview.chromium.org/2076583002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2076583002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: public static ChildProcessConnection allocateBoundConnectionForTesting(Context context, On 2016/06/23 17:20:15, sievers wrote: > Can we maybe expose this through the test helper instead? > Then ChildProcessLauncher doesn't have to be in the content-api, see > crbug.com/617313. We could, but we have to declare the private allocateBoundConnection() as public. Is it ok? Same questions for other XXXForTesting()functions.
https://codereview.chromium.org/2076583002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2076583002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: public static ChildProcessConnection allocateBoundConnectionForTesting(Context context, On 2016/06/23 18:47:04, Xi Han wrote: > On 2016/06/23 17:20:15, sievers wrote: > > Can we maybe expose this through the test helper instead? > > Then ChildProcessLauncher doesn't have to be in the content-api, see > > crbug.com/617313. > > We could, but we have to declare the private allocateBoundConnection() as > public. Is it ok? Same questions for other XXXForTesting()functions. But CPLTestHelper is in the same package. On second thought, I don't think it matters all that much whether this is public or not since we'd be enforcing DEPS in a different way. But then: we should not be testing implementation details from one layer (here: content) in a layer above (here: chrome). Do you think ChromeChildProcessLauncherTest.java could just merge into ChildProcessLauncherTest.java? The only dependency is WebApkSandboxedProcessService.class, but does that matter much or could it just be TestDerivedSandboxedProcessService?
On 2016/06/23 20:19:33, sievers wrote: > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > (right): > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: > public static ChildProcessConnection allocateBoundConnectionForTesting(Context > context, > On 2016/06/23 18:47:04, Xi Han wrote: > > On 2016/06/23 17:20:15, sievers wrote: > > > Can we maybe expose this through the test helper instead? > > > Then ChildProcessLauncher doesn't have to be in the content-api, see > > > crbug.com/617313. > > > > We could, but we have to declare the private allocateBoundConnection() as > > public. Is it ok? Same questions for other XXXForTesting()functions. > > But CPLTestHelper is in the same package. > > On second thought, I don't think it matters all that much whether this is public > or not since we'd be enforcing DEPS in a different way. > > But then: we should not be testing implementation details from one layer (here: > content) in a layer above (here: chrome). > > Do you think ChromeChildProcessLauncherTest.java could just merge into > ChildProcessLauncherTest.java? > > The only dependency is WebApkSandboxedProcessService.class, but does that matter > much or could it just be TestDerivedSandboxedProcessService? There is another dependency that we need to pre-install a test_webapk.apk, and that is in //chrome. It makes content tests look bad.
On 2016/06/23 20:34:59, Xi Han wrote: > On 2016/06/23 20:19:33, sievers wrote: > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > File > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > > (right): > > > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: > > public static ChildProcessConnection allocateBoundConnectionForTesting(Context > > context, > > On 2016/06/23 18:47:04, Xi Han wrote: > > > On 2016/06/23 17:20:15, sievers wrote: > > > > Can we maybe expose this through the test helper instead? > > > > Then ChildProcessLauncher doesn't have to be in the content-api, see > > > > crbug.com/617313. > > > > > > We could, but we have to declare the private allocateBoundConnection() as > > > public. Is it ok? Same questions for other XXXForTesting()functions. > > > > But CPLTestHelper is in the same package. > > > > On second thought, I don't think it matters all that much whether this is > public > > or not since we'd be enforcing DEPS in a different way. > > > > But then: we should not be testing implementation details from one layer > (here: > > content) in a layer above (here: chrome). > > > > Do you think ChromeChildProcessLauncherTest.java could just merge into > > ChildProcessLauncherTest.java? > > > > The only dependency is WebApkSandboxedProcessService.class, but does that > matter > > much or could it just be TestDerivedSandboxedProcessService? > > There is another dependency that we need to pre-install a test_webapk.apk, and > that is in //chrome. It makes content tests look bad. Only to override the class name and number of services? Because related to comment #51/52 in https://codereview.chromium.org/2049843004/, that would then be taken care of also.
On 2016/06/23 22:04:32, sievers wrote: > On 2016/06/23 20:34:59, Xi Han wrote: > > On 2016/06/23 20:19:33, sievers wrote: > > > > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > > File > > > > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > > > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: > > > public static ChildProcessConnection > allocateBoundConnectionForTesting(Context > > > context, > > > On 2016/06/23 18:47:04, Xi Han wrote: > > > > On 2016/06/23 17:20:15, sievers wrote: > > > > > Can we maybe expose this through the test helper instead? > > > > > Then ChildProcessLauncher doesn't have to be in the content-api, see > > > > > crbug.com/617313. > > > > > > > > We could, but we have to declare the private allocateBoundConnection() as > > > > public. Is it ok? Same questions for other XXXForTesting()functions. > > > > > > But CPLTestHelper is in the same package. > > > > > > On second thought, I don't think it matters all that much whether this is > > public > > > or not since we'd be enforcing DEPS in a different way. > > > > > > But then: we should not be testing implementation details from one layer > > (here: > > > content) in a layer above (here: chrome). > > > > > > Do you think ChromeChildProcessLauncherTest.java could just merge into > > > ChildProcessLauncherTest.java? > > > > > > The only dependency is WebApkSandboxedProcessService.class, but does that > > matter > > > much or could it just be TestDerivedSandboxedProcessService? > > > > There is another dependency that we need to pre-install a test_webapk.apk, and > > that is in //chrome. It makes content tests look bad. > > Only to override the class name and number of services? > > Because related to comment #51/52 in > https://codereview.chromium.org/2049843004/, that would then be taken care of > also. Unless I misunderstood your suggestion, the class WebApkSandboxedProcessService is in //chrome, override it can't resolve the dependency. Besides, I added the target "//chrome/android/webapk/shell_apk:test_webapk" in Build.gn, but I can't think a way to get rid of it if we want to move the test to content.
On 2016/06/23 23:02:42, Xi Han wrote: > On 2016/06/23 22:04:32, sievers wrote: > > On 2016/06/23 20:34:59, Xi Han wrote: > > > On 2016/06/23 20:19:33, sievers wrote: > > > > > > > > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > > > File > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2076583002/diff/200001/content/public/android... > > > > > > > > > > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:945: > > > > public static ChildProcessConnection > > allocateBoundConnectionForTesting(Context > > > > context, > > > > On 2016/06/23 18:47:04, Xi Han wrote: > > > > > On 2016/06/23 17:20:15, sievers wrote: > > > > > > Can we maybe expose this through the test helper instead? > > > > > > Then ChildProcessLauncher doesn't have to be in the content-api, see > > > > > > crbug.com/617313. > > > > > > > > > > We could, but we have to declare the private allocateBoundConnection() > as > > > > > public. Is it ok? Same questions for other XXXForTesting()functions. > > > > > > > > But CPLTestHelper is in the same package. > > > > > > > > On second thought, I don't think it matters all that much whether this is > > > public > > > > or not since we'd be enforcing DEPS in a different way. > > > > > > > > But then: we should not be testing implementation details from one layer > > > (here: > > > > content) in a layer above (here: chrome). > > > > > > > > Do you think ChromeChildProcessLauncherTest.java could just merge into > > > > ChildProcessLauncherTest.java? > > > > > > > > The only dependency is WebApkSandboxedProcessService.class, but does that > > > matter > > > > much or could it just be TestDerivedSandboxedProcessService? > > > > > > There is another dependency that we need to pre-install a test_webapk.apk, > and > > > that is in //chrome. It makes content tests look bad. > > > > Only to override the class name and number of services? > > > > Because related to comment #51/52 in > > https://codereview.chromium.org/2049843004/, that would then be taken care of > > also. > > Unless I misunderstood your suggestion, the class WebApkSandboxedProcessService > is in //chrome, override it can't resolve the dependency. Besides, I added the > target "//chrome/android/webapk/shell_apk:test_webapk" in Build.gn, but I can't > think a way to get rid of it if we want to move the test to content. The part I don't understand is why we need any dependency on WebApkAnything if we are just testing the launching logic and whether it can be overriden in a certain way. Why don't we just test that with a content test? It looks like you are just verifying that the allocators are separate and can have a separate connection limit. But what's specific to WebApk about that? What if I had another application that overrode this? I'm just saying you want to test the feature you expose at the level where you expose it (in the extreme case you just have a unit test), instead of every embedder going and writing a test for the same thing (a test that might actually test code cross-respository then). So s/WebApk/FooTest/g in ChromeChildProcessLauncherTest.java :) And regarding the test_apk: I thought we already wanted to add an override API so that we wouldn't have to supply it in the manifest, see other patch. The nice thing is that then this testing api is also not visible outside content/.
Patchset #5 (id:220001) has been deleted
You are right, I only want to test the logic to allocate a connection for different package names. So I add a function ChildProcessLauncher #allocateConnection(), which only allocates a connection but never calls connection.start(). It makes possible to test any package name, since it doesn't need connect to the service declared in the AndroidManifest.xml. PTAL, thanks!
Patchset #6 (id:260001) has been deleted
Hi Daniel, PTAL, thanks!
lgtm https://codereview.chromium.org/2076583002/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2076583002/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429: private static ChildProcessConnection allocateConnection(Context context, nit: is this function really useful or can it be folded into the one you added below? so many 'allocate connection' functions in here :)
Patchset #7 (id:300001) has been deleted
Hi Daniel, I updated the tests again to fix failures after using an arbitrary package name (same reason in CL https://codereview.chromium.org/2049843004/). Could you please take another look? Thanks! https://codereview.chromium.org/2076583002/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2076583002/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:429: private static ChildProcessConnection allocateConnection(Context context, On 2016/06/24 20:04:35, sievers wrote: > nit: is this function really useful or can it be folded into the one you added > below? > so many 'allocate connection' functions in here :) Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, mariakhomenko@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2076583002/#ps320001 (title: "Fix test failures.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, because the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a test WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the test WebAPK be the package of chrome_public_apk. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, because the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a test WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the test WebAPK be the package of chrome_public_apk. BUG=609122 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, because the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a test WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the test WebAPK be the package of chrome_public_apk. BUG=609122 ========== to ========== Add WebAPK's tests in ChildProcessLauncherTest. Instrument tests are added in ChildProcessLauncherTest to connect to child processes in a WebAPK: - Move ChildProcessLauncherTest from //content to //chrome, because the WebAPKs instruments tests require to pre-install a WebAPK. - A refactoring is done to build a test WebAPK. This is because WebAPK needs to load its host browser's code to create a renderer, we hard code the host browser package name in the test WebAPK be the package of chrome_public_apk. BUG=609122 Committed: https://crrev.com/4b7e057321503b5f833156ee2b26ba85e0384ddd Cr-Commit-Position: refs/heads/master@{#402046} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4b7e057321503b5f833156ee2b26ba85e0384ddd Cr-Commit-Position: refs/heads/master@{#402046} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
