|
|
Created:
3 years, 11 months ago by the real yoland Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate ContentShellActivityTestRule and BaseJUnitRunner
TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base
classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and
ContentShellTestBase are refactored to use ContentShellTestCommon for implementation
of the utility method that tests uses. ContentShellPreconditionsTest and
ContentShellShellManagement are two tests converted to JUnit4 using
chromium-junit-migration script
(https://github.com/yoland68/chromium-junit-auto-migrate)
BaseJUnitRunner is the JUnit4 replacement for BaseChromiumInstrumentationTestRunner
for multidex support in devices lower than L
BUG=640116
Review-Url: https://codereview.chromium.org/2632043002
Cr-Commit-Position: refs/heads/master@{#454760}
Committed: https://chromium.googlesource.com/chromium/src/+/e85326c343c144f494376c756339dfca0e87d25b
Patch Set 1 #Patch Set 2 : Add Activity Test Rule #
Total comments: 14
Patch Set 3 : All test rules for content_test_apk #Patch Set 4 : comparing base #Patch Set 5 : All test rules for content_test_apk #Patch Set 6 : Make apis public and change year #
Total comments: 9
Patch Set 7 : Changes after +jbudorick's comments #Patch Set 8 : Add TODO for ContentShellActivityTestRule #Patch Set 9 : Add TODO for launchContentShellWithUrlSync #
Total comments: 6
Patch Set 10 : change after mike's commments #
Total comments: 6
Patch Set 11 : Changes after bo's comments #Patch Set 12 : Refactor to TestBase and TestRule to use same delegate class (Only ContentShellTestBase) #
Total comments: 6
Patch Set 13 : Change to adapter pattern #
Total comments: 6
Patch Set 14 : change name to TestCommon #Patch Set 15 : Change ContentShellTestbase to use getInstrumentation() #
Total comments: 4
Patch Set 16 : Change javadoc #Messages
Total messages: 71 (37 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... File content/shell/android/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/AndroidManifest.xml:22: android:label="Android JUnit 4 runner" /> This should be something like "JUnit4-based tests for org.chromium.content_shell_apk" https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:46: public class ContentShellActivityTestRule extends ActivityTestRule<ContentShellActivity> { This needs a javadoc comment. https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:81: protected ContentShellActivity launchContentShellWithUrl(String url) { So a test that wanted to call this would call mContentShellRule.launchContentShellWithUrl("https://www.example.com") instead of just launchContentShellWithUrl("https://www.example.com") ? https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:92: // TODO(cjhopman): These functions are inconsistent with launchContentShell***. Should be Do this. https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:179: /** nit: +1 blank line before this https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:215: // TODO(aelias): This method needs to be removed once http://crbug.com/179511 is fixed. Can you check if we still need this method? 179511 has been closed as WontFix... https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:249: //TODO(yolandyan): this should be done through parameterized test This CL should wait on parameterized tests landing, then.
Added all the test rules needed for content_test_apk ContentShellShellManagementTest.java is auto changed by this script (https://github.com/yoland68/chromium-junit-auto-migrate) https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... File content/shell/android/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/AndroidManifest.xml:22: android:label="Android JUnit 4 runner" /> On 2017/01/18 at 15:10:08, jbudorick wrote: > This should be something like "JUnit4-based tests for org.chromium.content_shell_apk" Done https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:46: public class ContentShellActivityTestRule extends ActivityTestRule<ContentShellActivity> { On 2017/01/18 at 15:10:09, jbudorick wrote: > This needs a javadoc comment. Done https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:81: protected ContentShellActivity launchContentShellWithUrl(String url) { On 2017/01/18 at 15:10:09, jbudorick wrote: > So a test that wanted to call this would call > > mContentShellRule.launchContentShellWithUrl("https://www.example.com") > > instead of just > > launchContentShellWithUrl("https://www.example.com") > > ? yes https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:92: // TODO(cjhopman): These functions are inconsistent with launchContentShell***. Should be On 2017/01/18 at 15:10:09, jbudorick wrote: > Do this. Done https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:179: /** On 2017/01/18 at 15:10:09, jbudorick wrote: > nit: +1 blank line before this Done https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:215: // TODO(aelias): This method needs to be removed once http://crbug.com/179511 is fixed. On 2017/01/18 at 15:10:09, jbudorick wrote: > Can you check if we still need this method? 179511 has been closed as WontFix... This is still used https://cs.chromium.org/chromium/src/content/public/android/javatests/src/org..., it's left on purpose according to this: https://codereview.chromium.org/2199223002#msg41 https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/j... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:249: //TODO(yolandyan): this should be done through parameterized test On 2017/01/18 at 15:10:08, jbudorick wrote: > This CL should wait on parameterized tests landing, then. This is actually no longer true, parameterized test is good for parameterized a single test.
The TestRules are mostly copied from *TestBase.java files, I added patch 4 with the original TestBase files as diff base: https://codereview.chromium.org/2632043002/diff2/60001:80001/content/shell/an... https://codereview.chromium.org/2632043002/diff2/60001:80001/content/public/a... https://codereview.chromium.org/2632043002/diff2/60001:80001/content/public/a... https://codereview.chromium.org/2632043002/diff2/60001:80001/content/public/t...
https://codereview.chromium.org/2632043002/diff2/60001:100001/content/shell/a... https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/...
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:49: * Test would use this ActivityTestRule to launch or get ContentShellActivity. nits: "Test would" -> "Tests can" "launch or get" -> "interact with" https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:50: * For more information on ActivityTestRule, check nit: I'm not sure this is necessary. https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:104: public void launchActivityWithTestUrl(String url) { Either leave the TODO in or address it. I think it's still valid. https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:252: // TODO(yolandyan): this should be done through parameterized test This appears to be a preland TODO.
https://codereview.chromium.org/2632043002/diff2/100001:120001/content/shell/... https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:49: * Test would use this ActivityTestRule to launch or get ContentShellActivity. On 2017/02/22 at 16:09:30, jbudorick wrote: > nits: "Test would" -> "Tests can" > "launch or get" -> "interact with" Done https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:50: * For more information on ActivityTestRule, check On 2017/02/22 at 16:09:30, jbudorick wrote: > nit: I'm not sure this is necessary. Done https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:104: public void launchActivityWithTestUrl(String url) { On 2017/02/22 at 16:09:29, jbudorick wrote: > Either leave the TODO in or address it. I think it's still valid. I see, changed to launchContentShellWithUrlSync(..)?
Added TODO as discussed offline
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:104: public void launchActivityWithTestUrl(String url) { On 2017/02/22 19:35:37, the real yoland wrote: > On 2017/02/22 at 16:09:29, jbudorick wrote: > > Either leave the TODO in or address it. I think it's still valid. > > I see, changed to launchContentShellWithUrlSync(..)? We should either: 1) move the getIsolatedTestFileUrl calss into the tests that call this, or 2) name this launchContentShellWithTestUrlSync w/ a TODO to do (1)
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:104: public void launchActivityWithTestUrl(String url) { On 2017/02/22 at 21:27:39, jbudorick wrote: > On 2017/02/22 19:35:37, the real yoland wrote: > > On 2017/02/22 at 16:09:29, jbudorick wrote: > > > Either leave the TODO in or address it. I think it's still valid. > > > > I see, changed to launchContentShellWithUrlSync(..)? > > We should either: > 1) move the getIsolatedTestFileUrl calss into the tests that call this, or > 2) name this launchContentShellWithTestUrlSync w/ a TODO to do (1) Done 2
this all lgtm with some comments https://codereview.chromium.org/2632043002/diff/160001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:68: wait(5000); Consider replacing this value with CONSTANT. Maybe use a scaleTimeout thingy https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... File content/shell/android/BUILD.gn (right): https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... content/shell/android/BUILD.gn:155: "javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java", Should NativeLibraryTestRule.java and JavaBridgeActivityTestRule.java and ContentDetectionActivityTestRule.java be added here. https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:265: + " See ContentShellTestBase#runTest.", nit: this indentation looks a little off
Description was changed from ========== Create TestRules for content shell test apk BUG=640116 ========== to ========== Create TestRules for content shell test apk TestRules in JUnit4 are the replacements for test base classes in JUnit3. The TestRule created here are mostly copied over from its previous base class (e.g ContentDetectionActivityTestRule is from ContentDetectionTestBase). BUG=640116 ==========
yolandyan@chromium.org changed reviewers: + boliu@chromium.org
Comparison between .*TestBase and TestRules: https://codereview.chromium.org/2632043002/diff2/60001:180001/content/shell/a... https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/... https://codereview.chromium.org/2632043002/diff/160001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:68: wait(5000); On 2017/02/23 at 19:21:35, mikecase wrote: > Consider replacing this value with CONSTANT. Maybe use a scaleTimeout thingy Done https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... File content/shell/android/BUILD.gn (right): https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... content/shell/android/BUILD.gn:155: "javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java", On 2017/02/23 at 19:21:35, mikecase wrote: > Should NativeLibraryTestRule.java and JavaBridgeActivityTestRule.java and ContentDetectionActivityTestRule.java be added here. Done https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:265: + " See ContentShellTestBase#runTest.", On 2017/02/23 at 19:21:35, mikecase wrote: > nit: this indentation looks a little off Done
only looked at ContentDetectionActivityTestRule since that's the biggest file are all the Rule files *copied*? In that case, definitely need to refactor things to avoid code copying as much as possible https://codereview.chromium.org/2632043002/diff/180001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2632043002/diff/180001/base/BUILD.gn#newcode2571 base/BUILD.gn:2571: "test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java", should this be in a separate CL? https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:94: return launchActivity(intent); an assert here that launchActivity in the constructor is false https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:131: public void waitForActiveShellToBeDoneLoading() { a lot of this code is directly copied from ContentShellTestBase can we refactor this to avoid copying code as much as possible
+boliu, ty for the review. All the Rule files are copied over but TestBase classes are going to be deleted after the JUnit4 migration. They are only coexisting temporarily so we incrementally convert tests to JUnit4 and run both 3 and 4 tests at the same time.
https://codereview.chromium.org/2632043002/diff2/60001:200001/content/shell/a... https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/... https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/... https://codereview.chromium.org/2632043002/diff/180001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2632043002/diff/180001/base/BUILD.gn#newcode2571 base/BUILD.gn:2571: "test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java", On 2017/02/27 at 19:50:56, boliu wrote: > should this be in a separate CL? Oops, deleted https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:94: return launchActivity(intent); On 2017/02/27 at 19:50:56, boliu wrote: > an assert here that launchActivity in the constructor is false I see, done https://codereview.chromium.org/2632043002/diff/180001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:131: public void waitForActiveShellToBeDoneLoading() { On 2017/02/27 at 19:50:56, boliu wrote: > a lot of this code is directly copied from ContentShellTestBase > > can we refactor this to avoid copying code as much as possible as commented, TestBase is going away in the near future, and we will change the tests as fast as possible (https://codereview.chromium.org/2708243004) to avoid TestBase and TestRule having the same code for a long time.
On 2017/02/27 20:03:23, the real yoland wrote: > +boliu, ty for the review. All the Rule files are copied over but TestBase > classes are going to be deleted after the JUnit4 migration. > > They are only coexisting temporarily so we incrementally convert tests to JUnit4 > and run both 3 and 4 tests at the same time. How close is that switch?
On 2017/02/27 at 21:10:28, boliu wrote: > On 2017/02/27 20:03:23, the real yoland wrote: > > +boliu, ty for the review. All the Rule files are copied over but TestBase > > classes are going to be deleted after the JUnit4 migration. > > > > They are only coexisting temporarily so we incrementally convert tests to JUnit4 > > and run both 3 and 4 tests at the same time. > > How close is that switch? As chatted offline, refactor the ContentShellTestBase and ContentShellActivityTestRule to use the same delegate object for various APIs
https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:72: public void launchContentShellWithUrlSync(String url) { can this move to delegate too? I guess you would lose the assert mLaunchActivity, but then you could move mLaunchActivity into the delegate as well? https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:121: public void loadUrl(final NavigationController navigationController, this and handleBlockingCallbackAction can just move to delegate too? https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBaseDelegate.java (right): https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBaseDelegate.java:119: uiThreadAction.run(); something is inconsistent here, the contract here should be that uiThreadAction will be run on the UI thread, but this is not the case, but looking at the code, some code is correct, but some code (like loadUrl) is not
Discussed with +boliu offline, abandoning the the functional approach. Instead, create TestDelegateCallback interface so TestBase and TestRule would implement them. I named all the methods in TestDelegateCallback inteface with the postfix "ForDelegate". So it would not be confused with existing similar methods that are inherited (e.g. `getActivity()` vs `getActivityForDelegate()`) https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:72: public void launchContentShellWithUrlSync(String url) { On 2017/03/01 at 23:01:23, boliu wrote: > can this move to delegate too? I guess you would lose the assert mLaunchActivity, but then you could move mLaunchActivity into the delegate as well? Changed to adapter pattern https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:121: public void loadUrl(final NavigationController navigationController, On 2017/03/01 at 23:01:23, boliu wrote: > this and handleBlockingCallbackAction can just move to delegate too? Done https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBaseDelegate.java (right): https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBaseDelegate.java:119: uiThreadAction.run(); On 2017/03/01 at 23:01:23, boliu wrote: > something is inconsistent here, the contract here should be that uiThreadAction will be run on the UI thread, but this is not the case, but looking at the code, some code is correct, but some code (like loadUrl) is not Done
nice https://codereview.chromium.org/2632043002/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:15: public class BaseJUnitRunner extends AndroidJUnitRunner { is this on the right CL? https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:94: mDelegate.launchContentShellWithUrlSync(url); assert as well https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestDelegate.java (right): https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestDelegate.java:47: public final class ContentShellTestDelegate { Ok, let's bikeshed the name a bit.. Delegate is the wrong one, that generally implies going up an abstraction layer rather than down. (Eg it makes more sense to call the Callback thing Delegate rather than the other way around). For this, there's Impl, Core, Common.. or something else?
https://codereview.chromium.org/2632043002/diff/240001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/240001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:15: public class BaseJUnitRunner extends AndroidJUnitRunner { On 2017/03/02 at 21:12:02, boliu wrote: > is this on the right CL? Yes, we do want to use this runner for content_shell_test_apk for multidex support below Lollipop https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:94: mDelegate.launchContentShellWithUrlSync(url); On 2017/03/02 at 21:12:02, boliu wrote: > assert as well Done https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestDelegate.java (right): https://codereview.chromium.org/2632043002/diff/240001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestDelegate.java:47: public final class ContentShellTestDelegate { On 2017/03/02 at 21:12:02, boliu wrote: > Ok, let's bikeshed the name a bit.. > > Delegate is the wrong one, that generally implies going up an abstraction layer rather than down. (Eg it makes more sense to call the Callback thing Delegate rather than the other way around). > > For this, there's Impl, Core, Common.. or something else? I see, changed to ContentShellTestCommon, since Core might be mistakenly associated with ContentViewCore and change the method to `xxxForImpl` is somewhat vague
Patchset #14 (id:260001) has been deleted
didn't include new file?
On 2017/03/02 at 21:29:19, boliu wrote: > didn't include new file? Just added
On 2017/03/02 at 21:29:52, the real yoland wrote: > On 2017/03/02 at 21:29:19, boliu wrote: > > didn't include new file? > > Just added https://codereview.chromium.org/2632043002/diff/280001/content/shell/android/...
On 2017/03/02 21:30:09, the real yoland wrote: > On 2017/03/02 at 21:29:52, the real yoland wrote: > > On 2017/03/02 at 21:29:19, boliu wrote: > > > didn't include new file? > > > > Just added > > https://codereview.chromium.org/2632043002/diff/280001/content/shell/android/... I mean you didn't upload ContentShellTestCommon? or is code review that slow..?
On 2017/03/02 at 21:32:58, boliu wrote: > On 2017/03/02 21:30:09, the real yoland wrote: > > On 2017/03/02 at 21:29:52, the real yoland wrote: > > > On 2017/03/02 at 21:29:19, boliu wrote: > > > > didn't include new file? > > > > > > Just added > > > > https://codereview.chromium.org/2632043002/diff/280001/content/shell/android/... > > I mean you didn't upload ContentShellTestCommon? or is code review that slow..? Nope, my mistake, added now
lgtm
The CQ bit was checked by yolandyan@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...
Description was changed from ========== Create TestRules for content shell test apk TestRules in JUnit4 are the replacements for test base classes in JUnit3. The TestRule created here are mostly copied over from its previous base class (e.g ContentDetectionActivityTestRule is from ContentDetectionTestBase). BUG=640116 ========== to ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. BUG=640116 ==========
Description was changed from ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. BUG=640116 ========== to ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ==========
Description was changed from ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ========== to ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ==========
Description was changed from ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ========== to ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ==========
The CQ bit was checked by yolandyan@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...
Description was changed from ========== Create ContentShellActivityTestRule for content shell test apk TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ========== to ========== Create ContentShellActivityTestRule and BaseJUnitRunner TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ==========
Description was changed from ========== Create ContentShellActivityTestRule and BaseJUnitRunner TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BUG=640116 ========== to ========== Create ContentShellActivityTestRule and BaseJUnitRunner TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BaseJUnitRunner is the JUnit4 replacement for BaseChromiumInstrumentationTestRunner for multidex support in devices lower than L BUG=640116 ==========
Patchset #16 (id:320001) has been deleted
Patchset #15 (id:300001) has been deleted
The CQ bit was checked by yolandyan@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #15 (id:340001) has been deleted
The CQ bit was checked by yolandyan@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...
yolandyan@chromium.org changed reviewers: + agrieve@chromium.org
+agrieve fpr base/ review
On 2017/03/03 02:17:02, the real yoland wrote: > +agrieve fpr base/ review base/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits https://codereview.chromium.org/2632043002/diff/360001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/360001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:13: * A custom runner for JUnit4 tests that checks requirements to conditionally ignore tests. nit: update this https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:171: @SuppressWarnings("javadoc") nit: ?
The CQ bit was checked by yolandyan@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...
https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:171: @SuppressWarnings("javadoc") On 2017/03/03 at 19:56:29, jbudorick wrote: > nit: ? Removed
https://codereview.chromium.org/2632043002/diff/360001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/360001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:13: * A custom runner for JUnit4 tests that checks requirements to conditionally ignore tests. On 2017/03/03 at 19:56:29, jbudorick wrote: > nit: update this Done
Patchset #16 (id:380001) has been deleted
The CQ bit was checked by yolandyan@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.
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, boliu@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2632043002/#ps400001 (title: "Change javadoc")
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": 400001, "attempt_start_ts": 1488600574532040, "parent_rev": "26c6a0e07e73b1e9d357edcea55566873a99bb20", "commit_rev": "e85326c343c144f494376c756339dfca0e87d25b"}
Message was sent while issue was closed.
Description was changed from ========== Create ContentShellActivityTestRule and BaseJUnitRunner TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BaseJUnitRunner is the JUnit4 replacement for BaseChromiumInstrumentationTestRunner for multidex support in devices lower than L BUG=640116 ========== to ========== Create ContentShellActivityTestRule and BaseJUnitRunner TestRule(ContentShellActivityTestRule) in JUnit4 are the replacements for test base classes (ContentShellTestBase) in JUnit3. ContentShellActivityTestRule and ContentShellTestBase are refactored to use ContentShellTestCommon for implementation of the utility method that tests uses. ContentShellPreconditionsTest and ContentShellShellManagement are two tests converted to JUnit4 using chromium-junit-migration script (https://github.com/yoland68/chromium-junit-auto-migrate) BaseJUnitRunner is the JUnit4 replacement for BaseChromiumInstrumentationTestRunner for multidex support in devices lower than L BUG=640116 Review-Url: https://codereview.chromium.org/2632043002 Cr-Commit-Position: refs/heads/master@{#454760} Committed: https://chromium.googlesource.com/chromium/src/+/e85326c343c144f494376c756339... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:400001) as https://chromium.googlesource.com/chromium/src/+/e85326c343c144f494376c756339... |