Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
690613003:
Adding Unit Test Coverage for Select Action Bar Cut, Paste, Select All options (Closed)
|
Created:
6 years, 1 month ago by AKVT Modified:
6 years, 1 month ago Reviewers:
jdduke (slow) CC:
chromium-reviews, darin-cc_chromium.org, jam, aurimas (slooooooooow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding Select Action Bar Cut, Paste and Select All Unit Test cases.
Select Action Bar is not having enough unit test coverage.
This change takes care of adding required unit test case for
Cut, Paste and Select All operation.
Committed: https://crrev.com/cc152a1057f3c1005d46dadc7d5042b03527dc72
Cr-Commit-Position: refs/heads/master@{#303256}
Patch Set 1 #Patch Set 2 : Removed unwanted API. #
Total comments: 1
Patch Set 3 : Added Paste Test cases. #
Total comments: 9
Patch Set 4 : Fixed review comments. #Patch Set 5 : Rearranged the HTML to escape from flaky failure. #
Total comments: 2
Patch Set 6 : Adjusted the content and removed ambigous comment. #Patch Set 7 : Changed the test content to prevent flaky failures. #Patch Set 8 : Refactored the HTML content to prevent flaky failures. #
Total comments: 1
Messages
Total messages: 34 (5 generated)
ajith.v@samsung.com changed reviewers: + jdduke@chromium.org
@jdduke PTAL
@jdduke PTAL this change.
https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:404: public void testSelectActionBarTextAreaSelectAll() throws Exception { Do you have any more similar tests? I'd prefer we add them all at once instead of piecemeal.
On 2014/10/30 17:48:31, jdduke wrote: > https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > (right): > > https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:404: > public void testSelectActionBarTextAreaSelectAll() throws Exception { > Do you have any more similar tests? I'd prefer we add them all at once instead > of piecemeal. I am thinking to conclude with Paste option, which I have not prepared yet. That will be the last among this set.
On 2014/10/30 17:51:42, AKV wrote: > On 2014/10/30 17:48:31, jdduke wrote: > > > https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... > > File > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > (right): > > > > > https://codereview.chromium.org/690613003/diff/20001/content/public/android/j... > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:404: > > public void testSelectActionBarTextAreaSelectAll() throws Exception { > > Do you have any more similar tests? I'd prefer we add them all at once instead > > of piecemeal. > > I am thinking to conclude with Paste option, which I have not prepared yet. That > will be the last among this set. I'll wait for that and we can do review all at once.
@jdduke PTAL. I have covered all I planned for this feature.
https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: assertWaitForSelectActionBarVisible(true); Don't you also want to assert that getSelectedText() is empty, i.e., the password shouldn't be passed to content/? https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:455: // Paste option won't be there for Password, hence no action happens This comment is misleading (as are several others). Yes, the paste option isn't there, but you're skipping that check by calling paste() directly. https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:501: mContentViewCore.getSelectActionHandler().cut(); The biggest problem I have with these tests is that we have extra dismissal logic in SelectActionModeCallback.java that's being entirely skipped here. I'm not sure if there's an easy way to fully test that though... https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:583: private void copyStringToClipboard() { Could you make this function take a string, so it's clear at the callsite what's going on?
@jdduke please check my comments and share your opinion. https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: assertWaitForSelectActionBarVisible(true); On 2014/10/31 17:39:41, jdduke wrote: > Don't you also want to assert that getSelectedText() is empty, i.e., the > password shouldn't be passed to content/? Actually selected text we can't validate positively. Due to character issue. junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but was:<[**************]> But we can make negative check as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); Please share your opinion https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:455: // Paste option won't be there for Password, hence no action happens On 2014/10/31 17:39:41, jdduke wrote: > This comment is misleading (as are several others). Yes, the paste option isn't > there, but you're skipping that check by calling paste() directly. My intention is even if we call paste, expected action won't be happening. Please suggest any other way we can cover this scenario. https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:501: mContentViewCore.getSelectActionHandler().cut(); On 2014/10/31 17:39:41, jdduke wrote: > The biggest problem I have with these tests is that we have extra dismissal > logic in SelectActionModeCallback.java that's being entirely skipped here. I'm > not sure if there's an easy way to fully test that though... IMO, even though we have a SelectActionModeCallback, we are not using its functionality here, we just use Handler functionalilties. As I discussed in previous patch, we need to have separate coverage for SelectActionModeCallback using Menus. and directly call menu clicks without relying handler calls. In that case we can cover the dismissal scenarios. Please share your thoughts on this. https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:583: private void copyStringToClipboard() { On 2014/10/31 17:39:41, jdduke wrote: > Could you make this function take a string, so it's clear at the callsite what's > going on? Done.
lgtm https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: assertWaitForSelectActionBarVisible(true); On 2014/10/31 17:52:09, AKV wrote: > On 2014/10/31 17:39:41, jdduke wrote: > > Don't you also want to assert that getSelectedText() is empty, i.e., the > > password shouldn't be passed to content/? > Actually selected text we can't validate positively. Due to character issue. > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > was:<[**************]> > But we can make negative check > as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); Please > share your opinion > Hmm, nah this is OK.
On 2014/10/31 17:57:07, jdduke wrote: > lgtm > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > (right): > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > assertWaitForSelectActionBarVisible(true); > On 2014/10/31 17:52:09, AKV wrote: > > On 2014/10/31 17:39:41, jdduke wrote: > > > Don't you also want to assert that getSelectedText() is empty, i.e., the > > > password shouldn't be passed to content/? > > Actually selected text we can't validate positively. Due to character issue. > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > was:<[**************]> > > But we can make negative check > > as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); Please > > share your opinion > > > > Hmm, nah this is OK. Thanks
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690613003/60001
On 2014/10/31 17:59:24, AKV wrote: > On 2014/10/31 17:57:07, jdduke wrote: > > lgtm > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > File > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > (right): > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > > assertWaitForSelectActionBarVisible(true); > > On 2014/10/31 17:52:09, AKV wrote: > > > On 2014/10/31 17:39:41, jdduke wrote: > > > > Don't you also want to assert that getSelectedText() is empty, i.e., the > > > > password shouldn't be passed to content/? > > > Actually selected text we can't validate positively. Due to character issue. > > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > > was:<[**************]> > > > But we can make negative check > > > as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); > Please > > > share your opinion > > > > > > > Hmm, nah this is OK. > > Thanks Expecting some flaky failure. So added PS5 with safe content.
On 2014/10/31 18:29:10, AKV wrote: > On 2014/10/31 17:59:24, AKV wrote: > > On 2014/10/31 17:57:07, jdduke wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > File > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > > > assertWaitForSelectActionBarVisible(true); > > > On 2014/10/31 17:52:09, AKV wrote: > > > > On 2014/10/31 17:39:41, jdduke wrote: > > > > > Don't you also want to assert that getSelectedText() is empty, i.e., the > > > > > password shouldn't be passed to content/? > > > > Actually selected text we can't validate positively. Due to character > issue. > > > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > > > was:<[**************]> > > > > But we can make negative check > > > > as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); > > Please > > > > share your opinion > > > > > > > > > > Hmm, nah this is OK. > > > > Thanks > > Expecting some flaky failure. So added PS5 with safe content. Can you first explain how that fixes the issue?
On 2014/10/31 19:06:26, jdduke wrote: > On 2014/10/31 18:29:10, AKV wrote: > > On 2014/10/31 17:59:24, AKV wrote: > > > On 2014/10/31 17:57:07, jdduke wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > File > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > > > > assertWaitForSelectActionBarVisible(true); > > > > On 2014/10/31 17:52:09, AKV wrote: > > > > > On 2014/10/31 17:39:41, jdduke wrote: > > > > > > Don't you also want to assert that getSelectedText() is empty, i.e., > the > > > > > > password shouldn't be passed to content/? > > > > > Actually selected text we can't validate positively. Due to character > > issue. > > > > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > > > > was:<[**************]> > > > > > But we can make negative check > > > > > as assertNotSame(mContentViewCore.getSelectedText(), "SamplePassword"); > > > Please > > > > > share your opinion > > > > > > > > > > > > > Hmm, nah this is OK. > > > > > > Thanks > > > > Expecting some flaky failure. So added PS5 with safe content. > > Can you first explain how that fixes the issue? It's test environment issue. When I see the test case running, if the long press or click is on an editable node, then IME comes up, then immediate event is jumping away to different element, than what we have directed. This will happen most likely for the element which stays at the bottom of the page. Here I saved it by keeping it on top :)
On 2014/10/31 19:14:27, AKV wrote: > On 2014/10/31 19:06:26, jdduke wrote: > > On 2014/10/31 18:29:10, AKV wrote: > > > On 2014/10/31 17:59:24, AKV wrote: > > > > On 2014/10/31 17:57:07, jdduke wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > File > > > > > > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > > > > > assertWaitForSelectActionBarVisible(true); > > > > > On 2014/10/31 17:52:09, AKV wrote: > > > > > > On 2014/10/31 17:39:41, jdduke wrote: > > > > > > > Don't you also want to assert that getSelectedText() is empty, i.e., > > the > > > > > > > password shouldn't be passed to content/? > > > > > > Actually selected text we can't validate positively. Due to character > > > issue. > > > > > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > > > > > was:<[**************]> > > > > > > But we can make negative check > > > > > > as assertNotSame(mContentViewCore.getSelectedText(), > "SamplePassword"); > > > > Please > > > > > > share your opinion > > > > > > > > > > > > > > > > Hmm, nah this is OK. > > > > > > > > Thanks > > > > > > Expecting some flaky failure. So added PS5 with safe content. > > > > Can you first explain how that fixes the issue? > > It's test environment issue. When I see the test case running, if the long press > or click is on an editable node, then IME comes up, then immediate event is > jumping away to different element, than what we have directed. This will happen > most likely for the element which stays at the bottom of the page. Here I saved > it by keeping it on top :) This is the cause of all flaky failures which happened in the past.
On 2014/10/31 19:15:14, AKV wrote: > On 2014/10/31 19:14:27, AKV wrote: > > On 2014/10/31 19:06:26, jdduke wrote: > > > On 2014/10/31 18:29:10, AKV wrote: > > > > On 2014/10/31 17:59:24, AKV wrote: > > > > > On 2014/10/31 17:57:07, jdduke wrote: > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > > File > > > > > > > > > > > > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/690613003/diff/40001/content/public/android/j... > > > > > > > > > > > > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:397: > > > > > > assertWaitForSelectActionBarVisible(true); > > > > > > On 2014/10/31 17:52:09, AKV wrote: > > > > > > > On 2014/10/31 17:39:41, jdduke wrote: > > > > > > > > Don't you also want to assert that getSelectedText() is empty, > i.e., > > > the > > > > > > > > password shouldn't be passed to content/? > > > > > > > Actually selected text we can't validate positively. Due to > character > > > > issue. > > > > > > > junit.framework.ComparisonFailure: expected:<[••••••••••••••]> but > > > > > > > was:<[**************]> > > > > > > > But we can make negative check > > > > > > > as assertNotSame(mContentViewCore.getSelectedText(), > > "SamplePassword"); > > > > > Please > > > > > > > share your opinion > > > > > > > > > > > > > > > > > > > Hmm, nah this is OK. > > > > > > > > > > Thanks > > > > > > > > Expecting some flaky failure. So added PS5 with safe content. > > > > > > Can you first explain how that fixes the issue? > > > > It's test environment issue. When I see the test case running, if the long > press > > or click is on an editable node, then IME comes up, then immediate event is > > jumping away to different element, than what we have directed. This will > happen > > most likely for the element which stays at the bottom of the page. Here I > saved > > it by keeping it on top :) > > This is the cause of all flaky failures which happened in the past. Locally, it still works, I ran try to ensure things are fine. Otherwise I will correct later with new Patch.
jdduke@google.com changed reviewers: + jdduke@google.com
https://codereview.chromium.org/690613003/diff/80001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/80001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:455: // Paste option won't be there for Password, hence no action happens Why can't you paste into a password field? That seems odd, and you can definitely paste by long pressing the empty password field and tapping the "Paste" popup.
https://codereview.chromium.org/690613003/diff/80001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/80001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:455: // Paste option won't be there for Password, hence no action happens On 2014/11/03 17:39:33, jdduke1 wrote: > Why can't you paste into a password field? That seems odd, and you can > definitely paste by long pressing the empty password field and tapping the > "Paste" popup. Right. The comment line was misleading. We will have Paste option on Action bar, when we do text selection on password field.
@jdduke PTAL PS6. Once you approve I will commit. Thank you
On 2014/11/03 19:45:36, AKV wrote: > @jdduke PTAL PS6. Once you approve I will commit. Thank you I still don't understand how the last change fixed the failure?
On 2014/11/03 19:54:33, jdduke wrote: > On 2014/11/03 19:45:36, AKV wrote: > > @jdduke PTAL PS6. Once you approve I will commit. Thank you > > I still don't understand how the last change fixed the failure? As I informed earlier, it was a flaky failure. When IME is up, if the next element we are trying to send event is not in visible screen, it tries to pass event to some arbitrary node on screen, which was cause of failure in this case. Please let me know it's a known test environment bug
On 2014/11/03 19:57:10, AKV wrote: > On 2014/11/03 19:54:33, jdduke wrote: > > On 2014/11/03 19:45:36, AKV wrote: > > > @jdduke PTAL PS6. Once you approve I will commit. Thank you > > > > I still don't understand how the last change fixed the failure? > > As I informed earlier, it was a flaky failure. When IME is up, if the next > element we are trying to send event is not in visible screen, it tries to pass > event to some arbitrary node on screen, which was cause of failure in this case. > Please let me know it's a known test environment bug So then the test depends on the size of the device viewport? If a test is likely to fail just by changing the test device, I'm not sure it's worth adding.
jdduke@chromium.org changed reviewers: - jdduke@google.com
On 2014/11/03 20:00:13, jdduke wrote: > On 2014/11/03 19:57:10, AKV wrote: > > On 2014/11/03 19:54:33, jdduke wrote: > > > On 2014/11/03 19:45:36, AKV wrote: > > > > @jdduke PTAL PS6. Once you approve I will commit. Thank you > > > > > > I still don't understand how the last change fixed the failure? > > > > As I informed earlier, it was a flaky failure. When IME is up, if the next > > element we are trying to send event is not in visible screen, it tries to pass > > event to some arbitrary node on screen, which was cause of failure in this > case. > > Please let me know it's a known test environment bug > > So then the test depends on the size of the device viewport? If a test is likely > to fail just by changing the test device, I'm not sure it's worth adding. I shall correct the flaky failures by using different set of test cases. Once verified, I will upload new patch set. Thank you.
@jdduke PTAL. I have corrected the flaky failures with adjustment in content as well as with test coverage
lgtm https://codereview.chromium.org/690613003/diff/140001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/690613003/diff/140001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:35: + "<br/><p><span id=\"plain_text_1\">SamplePlainTextOne</span></p>" Interesting, I wonder if the selection/insertion handles were interfering with the long press.
lgtm
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690613003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/cc152a1057f3c1005d46dadc7d5042b03527dc72 Cr-Commit-Position: refs/heads/master@{#303256} |
