Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154023002:
adding action mode tests (Closed)
DescriptionAdd action mode tests.
BUG=628757
Committed: https://crrev.com/f4851343a554057597a3f7cc197b989e5326fd9e
Cr-Commit-Position: refs/heads/master@{#431751}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Make tests work for Android L #
Total comments: 1
Patch Set 3 : Disabled animations, use updated espresso in third-party after junit refactor. #
Total comments: 27
Patch Set 4 : Addressed Mike's comments #
Total comments: 4
Patch Set 5 : Addressed comments from mikecase@ #Patch Set 6 : Updated to use BaseJUnit4ClassRunner. #
Total comments: 20
Patch Set 7 : Addressed comments, added animation disable code that was missed in previous patch. #
Total comments: 8
Patch Set 8 : Updated L rootMatcher code per mikecase@ comments #
Total comments: 4
Patch Set 9 : Set rootMatcher in the catch too. #Patch Set 10 : Updated description #
Total comments: 4
Patch Set 11 : Re-arranged popup action logic per mikecase@ #Patch Set 12 : fix merge conflict #
Messages
Total messages: 33 (13 generated)
aluo@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, yolandyan@chromium.org
Tests for action mode in webview. Uses the AndroidJUnitRunner (see related cl https://codereview.chromium.org/2142413004).
https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/BUILD.gn (right): https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/BUILD.gn:57: "//third_party/hamcrest:hamcrest_library_java", We should see if we can get this to just have to depend on a group(espresso) target to keep things simpler. https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:68: private static final long UI_TIMEOUT = 10 * 1000; Use a org.chromium.base.test.util.ScalableTimeout here. Will let us scale timeout if needed (for example on slower devices) https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:149: /** Test Assist */ nit: I have usually seen javadoc in the format /** * Test Assist */ even when it is just one line. https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:153: if (Build.VERSION.SDK_INT < 24) return; I think TODOs should use one-line comments. e.g. // https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... File third_party/android_support_test_runner/BUILD.gn (right): https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... third_party/android_support_test_runner/BUILD.gn:5: import("//build/config/android/rules.gni") Would prefer if these additions to the third_party/ dir were in their own CL. https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... third_party/android_support_test_runner/BUILD.gn:17: jar_path = "lib/exposed-instrumentation-api-publish-0.5-no-dep.jar" this jar needs to get downloaded in a DEPs hook. https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... third_party/android_support_test_runner/BUILD.gn:20: android_java_prebuilt("junit_java") { shouldn't need this. There is a //third_party/junit already. https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... File third_party/android_support_test_runner/README.chromium (right): https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... third_party/android_support_test_runner/README.chromium:4: Version: 0.5 Why are you updating this? Do you have to?
https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/BUILD.gn (right): https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/BUILD.gn:57: "//third_party/hamcrest:hamcrest_library_java", On 2016/07/25 21:19:01, mikecase wrote: > We should see if we can get this to just have to depend on a group(espresso) > target to keep things simpler. Is there something special I need for groups to work? I tried this: group("espresso_all") { testonly = true deps = [ "//third_party/espresso:espresso_core_java", "//third_party/espresso:espresso_intents_java", "//third_party/espresso:espresso_web_java", "//third_party/hamcrest:hamcrest_core_java", "//third_party/hamcrest:hamcrest_integration_java", "//third_party/hamcrest:hamcrest_library_java", ] } then adding this group to the deps, but it didn't work. No errors in BUILD.gn file, but java couldn't find imports. https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:68: private static final long UI_TIMEOUT = 10 * 1000; On 2016/07/25 21:19:01, mikecase wrote: > Use a org.chromium.base.test.util.ScalableTimeout here. > > Will let us scale timeout if needed (for example on slower devices) Done. Actually this isn't used so I removed it. https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:149: /** Test Assist */ On 2016/07/25 21:19:01, mikecase wrote: > nit: I have usually seen javadoc in the format > > /** > * Test Assist > */ > > even when it is just one line. Done. https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:153: if (Build.VERSION.SDK_INT < 24) return; On 2016/07/25 21:19:01, mikecase wrote: > I think TODOs should use one-line comments. e.g. // Done. https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... File third_party/android_support_test_runner/BUILD.gn (right): https://codereview.chromium.org/2154023002/diff/1/third_party/android_support... third_party/android_support_test_runner/BUILD.gn:5: import("//build/config/android/rules.gni") On 2016/07/25 21:19:01, mikecase wrote: > Would prefer if these additions to the third_party/ dir were in their own CL. Yea this is a separate cl, shouldn't be part of this review once it's in.
https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/BUILD.gn (right): https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/BUILD.gn:57: "//third_party/hamcrest:hamcrest_library_java", On 2016/08/08 at 23:20:05, aluo wrote: > On 2016/07/25 21:19:01, mikecase wrote: > > We should see if we can get this to just have to depend on a group(espresso) > > target to keep things simpler. > > Is there something special I need for groups to work? I tried this: > > group("espresso_all") { > testonly = true > deps = [ > "//third_party/espresso:espresso_core_java", > "//third_party/espresso:espresso_intents_java", > "//third_party/espresso:espresso_web_java", > "//third_party/hamcrest:hamcrest_core_java", > "//third_party/hamcrest:hamcrest_integration_java", > "//third_party/hamcrest:hamcrest_library_java", > ] > } > > then adding this group to the deps, but it didn't work. No errors in BUILD.gn file, but java couldn't find imports. yeah, im not sure. try putting in a java_group() which should propagate the java_deps correctly. might not work though
On 2016/08/08 23:48:32, mikecase wrote: > https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... > File android_webview/tools/automated_ui_tests/BUILD.gn (right): > > https://codereview.chromium.org/2154023002/diff/1/android_webview/tools/autom... > android_webview/tools/automated_ui_tests/BUILD.gn:57: > "//third_party/hamcrest:hamcrest_library_java", > On 2016/08/08 at 23:20:05, aluo wrote: > > On 2016/07/25 21:19:01, mikecase wrote: > > > We should see if we can get this to just have to depend on a group(espresso) > > > target to keep things simpler. > > > > Is there something special I need for groups to work? I tried this: > > > > group("espresso_all") { > > testonly = true > > deps = [ > > "//third_party/espresso:espresso_core_java", > > "//third_party/espresso:espresso_intents_java", > > "//third_party/espresso:espresso_web_java", > > "//third_party/hamcrest:hamcrest_core_java", > > "//third_party/hamcrest:hamcrest_integration_java", > > "//third_party/hamcrest:hamcrest_library_java", > > ] > > } > > > > then adding this group to the deps, but it didn't work. No errors in BUILD.gn > file, but java couldn't find imports. > > yeah, im not sure. try putting in a java_group() which should propagate the > java_deps correctly. might not work though ok np, let me do some more digging... The java_group got me an error "Invalid java target name: //android_webview/tools/automated_ui_tests:espresso_all"
Want to rebase this CL. Then I'll review it. Once this lands we can just start adding tests. woohoo! https://codereview.chromium.org/2154023002/diff/20001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/2154023002/diff/20001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/AndroidManifest.xml:26: --> Delete commented out code.
Looks awesome. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:55: private void disableAnimation() { Add comments here. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:61: e.printStackTrace(); im not sure we want to catch this exception. Seems like a big problem if this doesnt work. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:104: onView(withId(R.id.edittext)).check(matches(withText("world"))); seems odd that this copies just "world". Do you where longClick() will click? Is there way to make it clearer by clicking directly where the word "world" is? https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:136: hasExtra(Intent.EXTRA_TEXT, "world"))))))); This indenting is pretty confusing. I would try to break it up differently. Especially the line that starts with allOf( seems to be indented inconsistently. Also, again, it doesnt seem obvious that longClick with select just "world" https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:153: hasEntry("query", "world"), hasEntry("new_search", true))))); try intending this differnetly to look prettier if you can. Also, the line that starts with "org.chromium" looks incorrectly indented. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:168: new UiSelector().packageName("com.google.android.googlequicksearchbox")); We have a lot of strings in these tests. Like the button names "Assist", "Share", and these package names. Might want to pull them out into constants in case they change? https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:182: @Override add empty line between these methods https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:200: rootMatchers.add(withDecorView(withChild(withText(name)))); Not sure I 100% understand what this is doing https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:208: /** Throw if we've gone through all the root matchers already */ Just use single line comment. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:209: if (rootMatchers.indexOf(rootMatcher) == rootMatchers.size() - 1) { Instead of doing this, I would just use a regular for loop and check the index like that. Calling indexOf(rootMatcher) seems avoidable. for (int i = 0; i < rootMatchers.size(); i++) if (i == rootMatchers.size() - 1) { https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:212: } catch (PerformException e2) { e2 is not such a good name. I think you can just use "e" https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:213: /** Take care of case when the item is in the overflow menu */ Use single line comment. Also, I think* that /** is usually just used for javadoc. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/WebViewTestBase.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/WebViewTestBase.java:32: } is this necessary?
Did you have comments for my feedback. Anyhow, some more comments while Im at it. nit: Fix capitalization in action mode tests. Like "Add WebView action mode tests." https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java (right): https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:63: e.printStackTrace(); This you can just.... Log.e(TAG, "Couldn't disable animation", e); and not print the stack trace. https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:246: private static final ViewAction longClickOnLastWord() { This seems super flaky. Im working on a CL to let us click on Javascript elements. For the time being, this is probably ok with a TODO to switch it to a better implementation.
Nit: Improve commit message punctuation. Adding UI action-mode tests for WebView. Just a few final comments. Everything looks good besides what I mentioned. Also, Ill let Yoland tkae a look at the Junit stuff. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:9: as I mentioned below, I think remove space. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:38: as I mentioned below, I think remove space. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:68: import android.test.suitebuilder.annotation.SmallTest; nit: check some other files for the format for imports. I dont think you should have any of these spaces between the "import android.**" https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:74: As above, look at format from other files for import, think you should remove this space https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:81: ditto here https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:120: private static final long ASSIST_TIMEOUT = scaleTimeout(5000); I think you should decalre these variables at the top of the class, before setUp() function. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:187: "org.chromium.webview_ui_test"), nit: Line up text. Wrong indent. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:218: // Matcher for N Can you do.... if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) // Set the matcher to the M matcher ... Im not a big fan of looping through the matchers. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:247: // TODO(aluo): Switch to new implementation by mikecase for clicking on elements nit: Probably reword this to something like... // TODO(aluo): This function is not guaranteed to click on element. Change to // implementation that gets bounding box for elements using Javascript.
lgtm with nits and also +mikecase's comments https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:133: onView(withId(R.id.edittext)).check(matches(withText("world"))); nits: it's better to have these on separate lines onView(withId(R.id.edittext)) .check() So if there is an exception, the line number will provide more detailed info first
Thanks guys for reviewing this again, I'll land this unless there are more comments in the latest patch, let me know, thanks. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:55: private void disableAnimation() { On 2016/09/29 19:27:50, mikecase wrote: > Add comments here. Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:61: e.printStackTrace(); On 2016/09/29 19:27:50, mikecase wrote: > im not sure we want to catch this exception. Seems like a big problem if this > doesnt work. Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:104: onView(withId(R.id.edittext)).check(matches(withText("world"))); On 2016/09/29 19:27:51, mikecase wrote: > seems odd that this copies just "world". Do you where longClick() will click? Is > there way to make it clearer by clicking directly where the word "world" is? Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:153: hasEntry("query", "world"), hasEntry("new_search", true))))); On 2016/09/29 19:27:51, mikecase wrote: > try intending this differnetly to look prettier if you can. Also, the line that > starts with "org.chromium" looks incorrectly indented. Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:168: new UiSelector().packageName("com.google.android.googlequicksearchbox")); On 2016/09/29 19:27:51, mikecase wrote: > We have a lot of strings in these tests. Like the button names "Assist", > "Share", and these package names. Might want to pull them out into constants in > case they change? Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:182: @Override On 2016/09/29 19:27:51, mikecase wrote: > add empty line between these methods Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:199: rootMatchers.add(withDecorView(isEnabled())); Add os comments for each matcher https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:200: rootMatchers.add(withDecorView(withChild(withText(name)))); On 2016/09/29 19:27:51, mikecase wrote: > Not sure I 100% understand what this is doing Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:203: onView(anyOf(withText(name), withContentDescription(name))) look into using id? https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:208: /** Throw if we've gone through all the root matchers already */ On 2016/09/29 19:27:51, mikecase wrote: > Just use single line comment. Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:209: if (rootMatchers.indexOf(rootMatcher) == rootMatchers.size() - 1) { On 2016/09/29 19:27:51, mikecase wrote: > Instead of doing this, I would just use a regular for loop and check the index > like that. Calling indexOf(rootMatcher) seems avoidable. > > > for (int i = 0; i < rootMatchers.size(); i++) > > if (i == rootMatchers.size() - 1) { Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:212: } catch (PerformException e2) { On 2016/09/29 19:27:51, mikecase wrote: > e2 is not such a good name. I think you can just use "e" Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:213: /** Take care of case when the item is in the overflow menu */ On 2016/09/29 19:27:51, mikecase wrote: > Use single line comment. Also, I think* that /** is usually just used for > javadoc. Acknowledged. https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/WebViewTestBase.java (right): https://codereview.chromium.org/2154023002/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/WebViewTestBase.java:32: } On 2016/09/29 19:27:51, mikecase wrote: > is this necessary? Acknowledged. https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java (right): https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/src/org/chromium/webview_ui_test/WebViewUiTestActivity.java:63: e.printStackTrace(); On 2016/10/04 22:12:56, mikecase wrote: > This you can just.... > > Log.e(TAG, "Couldn't disable animation", e); > > and not print the stack trace. Acknowledged. https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:246: private static final ViewAction longClickOnLastWord() { On 2016/10/04 22:12:56, mikecase wrote: > This seems super flaky. Im working on a CL to let us click on Javascript > elements. > > For the time being, this is probably ok with a TODO to switch it to a better > implementation. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:9: On 2016/11/04 22:22:52, mikecase wrote: > as I mentioned below, I think remove space. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:38: On 2016/11/04 22:22:52, mikecase wrote: > as I mentioned below, I think remove space. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:68: import android.test.suitebuilder.annotation.SmallTest; On 2016/11/04 22:22:52, mikecase wrote: > nit: check some other files for the format for imports. I dont think you should > have any of these spaces between the "import android.**" Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:74: On 2016/11/04 22:22:52, mikecase wrote: > As above, look at format from other files for import, think you should remove > this space Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:81: On 2016/11/04 22:22:52, mikecase wrote: > ditto here Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:120: private static final long ASSIST_TIMEOUT = scaleTimeout(5000); On 2016/11/04 22:22:51, mikecase wrote: > I think you should decalre these variables at the top of the class, before > setUp() function. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:133: onView(withId(R.id.edittext)).check(matches(withText("world"))); On 2016/11/04 22:46:03, the real yoland wrote: > nits: it's better to have these on separate lines > onView(withId(R.id.edittext)) > .check() > > So if there is an exception, the line number will provide more detailed info > first Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:187: "org.chromium.webview_ui_test"), On 2016/11/04 22:22:52, mikecase wrote: > nit: Line up text. Wrong indent. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:218: // Matcher for N On 2016/11/04 22:22:52, mikecase wrote: > Can you do.... > > if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) > // Set the matcher to the M matcher > > ... Im not a big fan of looping through the matchers. Acknowledged. https://codereview.chromium.org/2154023002/diff/100001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:247: // TODO(aluo): Switch to new implementation by mikecase for clicking on elements On 2016/11/04 22:22:52, mikecase wrote: > nit: Probably reword this to something like... > > // TODO(aluo): This function is not guaranteed to click on element. Change to > // implementation that gets bounding box for elements using Javascript. Acknowledged.
A tiny bit more feedback. Also, just fyi, "Acknowledged" I think is usually for "I didn't change anything but I understand your concern". I would reply "Done" or "Fixed" if you actively addressed comments. https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:198: "org.chromium.webview_ui_test"), nit: indent off by 1 space. https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:235: rootMatcher = DEFAULT; I dont think I get this completely. You set rootMatcher a few lines up... rootMatcher = withDecorView(withChild(withText(name))); ...and then before using it, set it again to.... rootMatcher = DEFAULT; https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:236: } catch (Throwable e) { Dont catch Throwable. Just catch both exceptions. https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: */ nit, I prefer the style of just multiple single line comments. // //
Thanks Mike, updated comments and patch. Yes I should've used Ack, that's the right one to use for corrections. https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:198: "org.chromium.webview_ui_test"), On 2016/11/08 16:47:02, mikecase wrote: > nit: indent off by 1 space. Done. https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:235: rootMatcher = DEFAULT; On 2016/11/08 16:47:02, mikecase wrote: > I dont think I get this completely. > > You set rootMatcher a few lines up... > > rootMatcher = withDecorView(withChild(withText(name))); > > ...and then before using it, set it again to.... > > rootMatcher = DEFAULT; if a ActionBar was displayed, then execution will reach the line rootMatcher = DEFAULT, otherwise it would throw before this and the existing value of rootMatcher would be used later. Updated comments about the default matcher being the espresso DEFAULT root matcher https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: */ On 2016/11/08 16:47:02, mikecase wrote: > nit, I prefer the style of just multiple single line comments. > // > // Done.
lgtm with my 1 last suggestion https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: } catch (NoMatchingViewException | AssertionFailedError e) { Do you actually want to catch AssertionFailedError? I assume you do not. https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:241: } Howabout doing something like this. Setting the rootMatcher in the catch{} block as well. Also, probably dont want to catch AssertionFailedError. try { onView(withClassName(endsWith("widget.ActionBarContextView"))) .check(matches(isDisplayed())); rootMatcher = DEFAULT; } catch (NoMatchingViewException e) { rootMatcher = withDecorView(withChild(withText(name))); }
https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/120001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:236: } catch (Throwable e) { On 2016/11/08 16:47:02, mikecase wrote: > Dont catch Throwable. Just catch both exceptions. Done. https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: } catch (NoMatchingViewException | AssertionFailedError e) { On 2016/11/11 18:52:15, mikecase wrote: > Do you actually want to catch AssertionFailedError? I assume you do not. I do need to use it, since view could be present but non-visible. If non-visible, the assertion isDisplayed fails and we still want to use the withDecorView matcher. https://codereview.chromium.org/2154023002/diff/140001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:241: } On 2016/11/11 18:52:15, mikecase wrote: > Howabout doing something like this. Setting the rootMatcher in the catch{} block > as well. > Also, probably dont want to catch AssertionFailedError. > > > try { > onView(withClassName(endsWith("widget.ActionBarContextView"))) > .check(matches(isDisplayed())); > rootMatcher = DEFAULT; > } catch (NoMatchingViewException e) { > rootMatcher = withDecorView(withChild(withText(name))); > } Done.
The CQ bit was checked by aluo@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 ========== adding action mode tests BUG=628757 ========== to ========== Add action mode tests. BUG=628757 ==========
https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:233: rootMatcher = withDecorView(withChild(withText(name))); setting rootMatcher here doesnt do anything anymore. Wait thinking of something more like.... Matcher<Root> rootMatcher; if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { try { // On L and lower, use the espresso DEFAULT root matcher if ActionBar is detected onView(withClassName(endsWith("widget.ActionBarContextView"))) rootMatcher = DEFAULT; } catch (NoMatchingViewException e) { rootMatcher = withDecorView(withChild(withText(name))); } else { rootMatcher = withDecorView(isEnabled()); } https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: } catch (NoMatchingViewException | AssertionFailedError e) { Still are catching AssertionFailedError.
https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java (right): https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:233: rootMatcher = withDecorView(withChild(withText(name))); On 2016/11/11 21:00:42, mikecase wrote: > setting rootMatcher here doesnt do anything anymore. > > > Wait thinking of something more like.... > > > > > Matcher<Root> rootMatcher; > > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { > > try { > // On L and lower, use the espresso DEFAULT root matcher if ActionBar is > detected > onView(withClassName(endsWith("widget.ActionBarContextView"))) > rootMatcher = DEFAULT; > } catch (NoMatchingViewException e) { > rootMatcher = withDecorView(withChild(withText(name))); > } > > else { > rootMatcher = withDecorView(isEnabled()); > } Done. https://codereview.chromium.org/2154023002/diff/180001/android_webview/tools/... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/ActionModeTest.java:239: } catch (NoMatchingViewException | AssertionFailedError e) { On 2016/11/11 21:00:42, mikecase wrote: > Still are catching AssertionFailedError. See my previous reply " I do need to use it, since view could be present but non-visible. If non-visible, the assertion isDisplayed fails and we still want to use the withDecorView matcher. "
The CQ bit was checked by aluo@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 aluo@chromium.org
The CQ bit was checked by aluo@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 aluo@chromium.org
The CQ bit was checked by aluo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yolandyan@chromium.org, mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2154023002/#ps220001 (title: "fix merge conflict")
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.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add action mode tests. BUG=628757 ========== to ========== Add action mode tests. BUG=628757 Committed: https://crrev.com/f4851343a554057597a3f7cc197b989e5326fd9e Cr-Commit-Position: refs/heads/master@{#431751} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f4851343a554057597a3f7cc197b989e5326fd9e Cr-Commit-Position: refs/heads/master@{#431751} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
