Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(64)

Issue 2632043002: Create ContentShellActivityTestRule and BaseJUnitRunner (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+512 lines, -164 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
A base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +25 lines, -0 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/ClipboardTest.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/SelectPopupTest.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/shell/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
M content/shell/android/javatests/AndroidManifest.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +5 lines, -0 lines 0 comments Download
A content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +184 lines, -0 lines 0 comments Download
M content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellPreconditionsTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +13 lines, -5 lines 0 comments Download
M content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellShellManagementTest.java View 1 2 3 chunks +23 lines, -12 lines 0 comments Download
M content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 9 chunks +59 lines, -147 lines 0 comments Download
A content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestCommon.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +195 lines, -0 lines 0 comments Download

Messages

Total messages: 71 (37 generated)
the real yoland
3 years, 11 months ago (2017-01-17 18:43:54 UTC) #2
jbudorick
https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/javatests/AndroidManifest.xml File content/shell/android/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/2632043002/diff/20001/content/shell/android/javatests/AndroidManifest.xml#newcode22 content/shell/android/javatests/AndroidManifest.xml:22: android:label="Android JUnit 4 runner" /> This should be something ...
3 years, 11 months ago (2017-01-18 15:10:09 UTC) #3
the real yoland
Added all the test rules needed for content_test_apk ContentShellShellManagementTest.java is auto changed by this script ...
3 years, 10 months ago (2017-02-22 00:44:57 UTC) #4
the real yoland
The TestRules are mostly copied from *TestBase.java files, I added patch 4 with the original ...
3 years, 10 months ago (2017-02-22 01:15:44 UTC) #5
the real yoland
https://codereview.chromium.org/2632043002/diff2/60001:100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:100001/content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java
3 years, 10 months ago (2017-02-22 01:27:38 UTC) #6
jbudorick
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode49 content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:49: * Test would use this ActivityTestRule to launch or ...
3 years, 10 months ago (2017-02-22 16:09:30 UTC) #7
the real yoland
https://codereview.chromium.org/2632043002/diff2/100001:120001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode49 content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:49: * Test would use this ActivityTestRule to launch ...
3 years, 10 months ago (2017-02-22 19:35:37 UTC) #8
the real yoland
Added TODO as discussed offline
3 years, 10 months ago (2017-02-22 21:23:20 UTC) #9
jbudorick
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode104 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 ...
3 years, 10 months ago (2017-02-22 21:27:39 UTC) #10
the real yoland
https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/100001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode104 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, ...
3 years, 10 months ago (2017-02-23 01:11:59 UTC) #11
mikecase (-- gone --)
this all lgtm with some comments https://codereview.chromium.org/2632043002/diff/160001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java#newcode68 content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:68: wait(5000); Consider replacing ...
3 years, 10 months ago (2017-02-23 19:21:35 UTC) #12
the real yoland
Comparison between .*TestBase and TestRules: https://codereview.chromium.org/2632043002/diff2/60001:180001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:180001/content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java https://codereview.chromium.org/2632043002/diff/160001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/160001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java#newcode68 content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:68: ...
3 years, 9 months ago (2017-02-27 18:32:49 UTC) #15
boliu
only looked at ContentDetectionActivityTestRule since that's the biggest file are all the Rule files *copied*? ...
3 years, 9 months ago (2017-02-27 19:50:56 UTC) #16
the real yoland
+boliu, ty for the review. All the Rule files are copied over but TestBase classes ...
3 years, 9 months ago (2017-02-27 20:03:23 UTC) #17
the real yoland
https://codereview.chromium.org/2632043002/diff2/60001:200001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java https://codereview.chromium.org/2632043002/diff2/60001:200001/content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java 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, ...
3 years, 9 months ago (2017-02-27 20:14:37 UTC) #18
boliu
On 2017/02/27 20:03:23, the real yoland wrote: > +boliu, ty for the review. All the ...
3 years, 9 months ago (2017-02-27 21:10:28 UTC) #19
the real yoland
On 2017/02/27 at 21:10:28, boliu wrote: > On 2017/02/27 20:03:23, the real yoland wrote: > ...
3 years, 9 months ago (2017-03-01 22:09:33 UTC) #20
boliu
https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/220001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode72 content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java:72: public void launchContentShellWithUrlSync(String url) { can this move to ...
3 years, 9 months ago (2017-03-01 23:01:23 UTC) #21
the real yoland
Discussed with +boliu offline, abandoning the the functional approach. Instead, create TestDelegateCallback interface so TestBase ...
3 years, 9 months ago (2017-03-02 20:16:48 UTC) #22
boliu
nice https://codereview.chromium.org/2632043002/diff/240001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/240001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java#newcode15 base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:15: public class BaseJUnitRunner extends AndroidJUnitRunner { is this ...
3 years, 9 months ago (2017-03-02 21:12:03 UTC) #23
the real yoland
https://codereview.chromium.org/2632043002/diff/240001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/240001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java#newcode15 base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:15: public class BaseJUnitRunner extends AndroidJUnitRunner { On 2017/03/02 at ...
3 years, 9 months ago (2017-03-02 21:26:56 UTC) #24
boliu
didn't include new file?
3 years, 9 months ago (2017-03-02 21:29:19 UTC) #26
the real yoland
On 2017/03/02 at 21:29:19, boliu wrote: > didn't include new file? Just added
3 years, 9 months ago (2017-03-02 21:29:52 UTC) #27
the real yoland
On 2017/03/02 at 21:29:52, the real yoland wrote: > On 2017/03/02 at 21:29:19, boliu wrote: ...
3 years, 9 months ago (2017-03-02 21:30:09 UTC) #28
boliu
On 2017/03/02 21:30:09, the real yoland wrote: > On 2017/03/02 at 21:29:52, the real yoland ...
3 years, 9 months ago (2017-03-02 21:32:58 UTC) #29
the real yoland
On 2017/03/02 at 21:32:58, boliu wrote: > On 2017/03/02 21:30:09, the real yoland wrote: > ...
3 years, 9 months ago (2017-03-02 21:36:12 UTC) #30
boliu
lgtm
3 years, 9 months ago (2017-03-02 21:38:40 UTC) #31
the real yoland
+agrieve fpr base/ review
3 years, 9 months ago (2017-03-03 02:17:02 UTC) #52
agrieve
On 2017/03/03 02:17:02, the real yoland wrote: > +agrieve fpr base/ review base/ lgtm
3 years, 9 months ago (2017-03-03 02:51:34 UTC) #53
jbudorick
lgtm w/ nits https://codereview.chromium.org/2632043002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java#newcode13 base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:13: * A custom runner for JUnit4 ...
3 years, 9 months ago (2017-03-03 19:56:29 UTC) #56
the real yoland
https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java (right): https://codereview.chromium.org/2632043002/diff/360001/content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellActivityTestRule.java#newcode171 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: ...
3 years, 9 months ago (2017-03-04 02:32:22 UTC) #59
the real yoland
https://codereview.chromium.org/2632043002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java File base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java (right): https://codereview.chromium.org/2632043002/diff/360001/base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java#newcode13 base/test/android/javatests/src/org/chromium/base/test/BaseJUnitRunner.java:13: * A custom runner for JUnit4 tests that checks ...
3 years, 9 months ago (2017-03-04 02:32:47 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2632043002/400001
3 years, 9 months ago (2017-03-04 04:09:41 UTC) #68
commit-bot: I haz the power
3 years, 9 months ago (2017-03-04 04:20:25 UTC) #71
Message was sent while issue was closed.
Committed patchset #16 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/e85326c343c144f494376c756339...

Powered by Google App Engine
This is Rietveld 408576698