Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
521583003:
Adding Text Selection Action Bar Unit Test cases. (Closed)
DescriptionAdding Text Selection Action Bar Unit Test cases.
Currently there is not enough unit test cases available to cover
the functionality of Text Selection Action Bar. In this patch covering
essentail unit test cases for the same.
BUG=
Committed: https://crrev.com/34ddd8e3947b5ebcce0039ead0e79d4963cbf9c5
Cr-Commit-Position: refs/heads/master@{#292971}
Patch Set 1 #Patch Set 2 : Renamed the Test file as per review comments. #
Total comments: 11
Patch Set 3 : Fixed review comments. #
Total comments: 6
Patch Set 4 : Corrected UT related to Empty input fields. #
Total comments: 2
Patch Set 5 : Removed unwanted test case. #Patch Set 6 : Removed failing Plain text UTs. Will refine with next CL. #Patch Set 7 : Splitted the long content to two parts for solving test failure. #
Total comments: 2
Patch Set 8 : Corrected the Page Scale to handle all content in Same URL #
Total comments: 6
Patch Set 9 : Fixed review comments. #
Total comments: 4
Patch Set 10 : Removed trivial assertWaitForSelectActionBarStatus(false) from starting of UT. #Patch Set 11 : Fixed the failed test case issue. #Messages
Total messages: 48 (6 generated)
ajith.v@samsung.com changed reviewers: + aurimas@chromium.org, jdduke@chromium.org
@jdduke & aurimas PTAL this change.
On 2014/08/29 17:18:50, AKV wrote: > @jdduke & aurimas PTAL this change. I'm already adding some standalone ContentViewCore selection-related tests here (https://codereview.chromium.org/516523003/), so let's wait until that lands to proceed.
On 2014/08/29 17:20:18, jdduke wrote: > On 2014/08/29 17:18:50, AKV wrote: > > @jdduke & aurimas PTAL this change. > > I'm already adding some standalone ContentViewCore selection-related tests here > (https://codereview.chromium.org/516523003/), so let's wait until that lands to > proceed. That is, the tests here should be folded into ContentViewCoreSelectionTest.
On 2014/08/29 17:20:46, jdduke wrote: > On 2014/08/29 17:20:18, jdduke wrote: > > On 2014/08/29 17:18:50, AKV wrote: > > > @jdduke & aurimas PTAL this change. > > > > I'm already adding some standalone ContentViewCore selection-related tests > here > > (https://codereview.chromium.org/516523003/), so let's wait until that lands > to > > proceed. > > That is, the tests here should be folded into ContentViewCoreSelectionTest. @jdduke Thanks for the information. I went through https://codereview.chromium.org/516523003/ it contains advanced Selection Action Bar Test cases. What I added here is basic UTs which was missing earlier. Can we proceed with this, before landing the other. Please share your opinion.
On 2014/08/29 17:26:49, AKV wrote: > On 2014/08/29 17:20:46, jdduke wrote: > > On 2014/08/29 17:20:18, jdduke wrote: > > > On 2014/08/29 17:18:50, AKV wrote: > > > > @jdduke & aurimas PTAL this change. > > > > > > I'm already adding some standalone ContentViewCore selection-related tests > > here > > > (https://codereview.chromium.org/516523003/), so let's wait until that lands > > to > > > proceed. > > > > That is, the tests here should be folded into ContentViewCoreSelectionTest. > > @jdduke Thanks for the information. I went through > https://codereview.chromium.org/516523003/ it contains advanced Selection Action > Bar Test cases. What I added here is basic UTs which was missing earlier. Can we > proceed with this, before landing the other. Please share your opinion. No, this is not worth a separate file, and there's no urgency in landing this test.
On 2014/08/29 17:27:37, jdduke wrote: > On 2014/08/29 17:26:49, AKV wrote: > > On 2014/08/29 17:20:46, jdduke wrote: > > > On 2014/08/29 17:20:18, jdduke wrote: > > > > On 2014/08/29 17:18:50, AKV wrote: > > > > > @jdduke & aurimas PTAL this change. > > > > > > > > I'm already adding some standalone ContentViewCore selection-related tests > > > here > > > > (https://codereview.chromium.org/516523003/), so let's wait until that > lands > > > to > > > > proceed. > > > > > > That is, the tests here should be folded into ContentViewCoreSelectionTest. > > > > @jdduke Thanks for the information. I went through > > https://codereview.chromium.org/516523003/ it contains advanced Selection > Action > > Bar Test cases. What I added here is basic UTs which was missing earlier. Can > we > > proceed with this, before landing the other. Please share your opinion. > > No, this is not worth a separate file, and there's no urgency in landing this > test. That said, you're welcome to rename this test to ContentViewCoreSelectionTest, and I can rebase my patch appropriately.
On 2014/08/29 17:29:04, jdduke wrote: > On 2014/08/29 17:27:37, jdduke wrote: > > On 2014/08/29 17:26:49, AKV wrote: > > > On 2014/08/29 17:20:46, jdduke wrote: > > > > On 2014/08/29 17:20:18, jdduke wrote: > > > > > On 2014/08/29 17:18:50, AKV wrote: > > > > > > @jdduke & aurimas PTAL this change. > > > > > > > > > > I'm already adding some standalone ContentViewCore selection-related > tests > > > > here > > > > > (https://codereview.chromium.org/516523003/), so let's wait until that > > lands > > > > to > > > > > proceed. > > > > > > > > That is, the tests here should be folded into > ContentViewCoreSelectionTest. > > > > > > @jdduke Thanks for the information. I went through > > > https://codereview.chromium.org/516523003/ it contains advanced Selection > > Action > > > Bar Test cases. What I added here is basic UTs which was missing earlier. > Can > > we > > > proceed with this, before landing the other. Please share your opinion. > > > > No, this is not worth a separate file, and there's no urgency in landing this > > test. > > That said, you're welcome to rename this test to ContentViewCoreSelectionTest, > and I can rebase my patch appropriately. @jdduke Thanks for the clarification. PTAL PS2
https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:4: // Remove empty // https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:17: * Integration tests for text selection action bar based on fixed regressions. "for text selection-related behavior.", no need to mention regressions or the action bar explicitly. https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:66: public void testSelectActionBarShouldClearOnTappingOutsideSelection() throws Exception { ShouldClear->Cleared https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:78: DOMUtils.clickNode(this, mContentViewCore, "textarea"); This is a click, not a long press, so how does this work? https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:87: DOMUtils.clickNode(this, mContentViewCore, "empty_textarea"); This is a click, not a long press. https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:96: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); This isn't a long press? This is a click?
@jdduke PTAL PS3 https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:4: // On 2014/08/29 17:43:25, jdduke wrote: > Remove empty // Done. https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:17: * Integration tests for text selection action bar based on fixed regressions. On 2014/08/29 17:43:24, jdduke wrote: > "for text selection-related behavior.", no need to mention regressions or the > action bar explicitly. Done. https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:78: DOMUtils.clickNode(this, mContentViewCore, "textarea"); On 2014/08/29 17:43:24, jdduke wrote: > This is a click, not a long press, so how does this work? Sorry it was a typo while correcting another UT. Thanks https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:87: DOMUtils.clickNode(this, mContentViewCore, "empty_textarea"); On 2014/08/29 17:43:25, jdduke wrote: > This is a click, not a long press. Thanks. Sorry it was a typo while correcting another UT. https://codereview.chromium.org/521583003/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:96: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); On 2014/08/29 17:43:24, jdduke wrote: > This isn't a long press? This is a click? Done. Thanks
https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.longPressNode(this, mContentViewCore, "anchor"); You should activate the action bar first somehow, as waiting for (false) will always return immediately because that is the default state. https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:66: public void testSelectActionBarShouldClearedOnTappingOutsideSelection() throws Exception { ShouldCleared -> Cleared https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:86: assertWaitForSelectActionBarStatus(false); You should activate the action bar first somehow, as waiting for (false) will always return immediately because that is the default state.
@jdduke PTAL https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.longPressNode(this, mContentViewCore, "anchor"); On 2014/08/29 18:06:36, jdduke wrote: > You should activate the action bar first somehow, as waiting for (false) will > always return immediately because that is the default state. When we keep text selection on some plain text and then long pressing on an anchor, text selection is not getting cleared. If it's not intended behavior we may have to fix in code and then modify this test. Please share your opinion. https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:66: public void testSelectActionBarShouldClearedOnTappingOutsideSelection() throws Exception { On 2014/08/29 18:06:36, jdduke wrote: > ShouldCleared -> Cleared Done. https://codereview.chromium.org/521583003/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:86: assertWaitForSelectActionBarStatus(false); On 2014/08/29 18:06:36, jdduke wrote: > You should activate the action bar first somehow, as waiting for (false) will > always return immediately because that is the default state. Done. Thanks
https://codereview.chromium.org/521583003/diff/60001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/60001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.longPressNode(this, mContentViewCore, "anchor"); If we can't make this test meaningful, let's remove it and try to rethink how we should test this.
https://codereview.chromium.org/521583003/diff/60001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/60001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.longPressNode(this, mContentViewCore, "anchor"); On 2014/08/29 18:23:07, jdduke wrote: > If we can't make this test meaningful, let's remove it and try to rethink how we > should test this. Basically when we long pressing on an anchor, we are not triggering any text selection. So action bar shouldn't appear. But context menu appears. That was the simple flow I covered. If we want to remove this I am Ok to do that. By keeping text selection ON, and long pressing on an anchor, we are able to see Contextmenu as well as previous text selection on Chrome Shell. I am not sure about that behavior is correct or not. I will try to fix that issue separately and add additional text case for it. Is that Ok ?
On 2014/08/29 18:30:05, AKV wrote: > Basically when we long pressing on an anchor, we are not triggering any text > selection. So action bar shouldn't appear. But context menu appears. That was > the simple flow I covered. If we want to remove this I am Ok to do that. > Right, we should remove it because it's possible the action bar *will* show after some time, but because you check for false, it will always succeed immediately, and we'd miss the failure. > By keeping text selection ON, and long pressing on an anchor, we are able to see > Contextmenu as well as previous text selection on Chrome Shell. I am not sure > about that behavior is correct or not. I will try to fix that issue separately > and add additional text case for it. Is that Ok ? I'm not sure it's that big of a deal, you can file a bug but this is an issue that should probably be solved at the Blink layer.
On 2014/08/29 18:36:47, jdduke wrote: > On 2014/08/29 18:30:05, AKV wrote: > > Basically when we long pressing on an anchor, we are not triggering any text > > selection. So action bar shouldn't appear. But context menu appears. That was > > the simple flow I covered. If we want to remove this I am Ok to do that. > > > > Right, we should remove it because it's possible the action bar *will* show > after some time, but because you check for false, it will always succeed > immediately, and we'd miss the failure. Thanks for the detailed explanation. I agree with you, it will hide the failure. > > > By keeping text selection ON, and long pressing on an anchor, we are able to > see > > Contextmenu as well as previous text selection on Chrome Shell. I am not sure > > about that behavior is correct or not. I will try to fix that issue separately > > and add additional text case for it. Is that Ok ? > > I'm not sure it's that big of a deal, you can file a bug but this is an issue > that should probably be solved at the Blink layer. Thanks, will file a bug for it. PTAL PS5
lgtm, thanks.
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/521583003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
On 2014/08/29 23:56:53, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) Looks like a legit failure.
On 2014/08/30 00:27:03, jdduke wrote: > On 2014/08/29 23:56:53, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > android_dbg_tests_recipe on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) > > Looks like a legit failure. When I ran these tests locally, no failure observed. From error pattern of failure, issue is with long pressing on plain text. Will look in detail later. Thanks
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/521583003/90001
The CQ bit was unchecked by jdduke@chromium.org
On 2014/08/31 14:33:14, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/ajith.v@samsung.com/521583003/90001 Removing half the tests because they're failing is not acceptable. Please fix the tests and then I'll reapprove. Not lgtm.
On 2014/08/31 15:24:33, jdduke wrote: > On 2014/08/31 14:33:14, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/ajith.v@samsung.com/521583003/90001 > > Removing half the tests because they're failing is not acceptable. Please fix > the tests and then I'll reapprove. Not lgtm. @jdduke - When I ran the tests locally, no issues observed. But when I commit, it's failing. Could you please point out any reason for failure ?
@jdduke PTAL. Earlier failure was due to long content in the page. And the test environment is not able to apply the events correctly. The failure was always happening for the content below the page. Test function, which depends on top of the page is passing always. I had splits the content into 2 parts for failed test cases for overcoming this issue.
@jdduke PTAL
https://codereview.chromium.org/521583003/diff/110001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/110001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:22: "<html><head><meta name=\"viewport\"" + Why not use a smaller scale factor here (1.1?)? Then everything should still be visible, and you don't need two page sets. Please try that.
@jdduke PTAL. Corrected using same URL https://codereview.chromium.org/521583003/diff/110001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/110001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:22: "<html><head><meta name=\"viewport\"" + On 2014/09/02 15:01:21, jdduke wrote: > Why not use a smaller scale factor here (1.1?)? Then everything should still be > visible, and you don't need two page sets. Please try that. Thanks for the suggestion. Done.
https://codereview.chromium.org/521583003/diff/130001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:20: private static final String DATA_URL_1 = UrlUtils.encodeHtmlDataUri( Nit: No need for _1 suffix. https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:60: DOMUtils.clickNode(this, mContentViewCore, "empty_input_text"); Why not clickNode on |plain_text_2|? That way we're not confusing the issue (whether the handle should show when you tamp an empty editable field). https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:89: DOMUtils.longPressNode(this, mContentViewCore, "empty_textarea"); You'll want to insert another valid selection between these last two, you can just insert DOMUtils.longPressNode(this, mContentViewCore, "textarea"); assertWaitForSelectActionBarStatus(true); between the "empty_input_text" and "empty_textarea" long presses.
@jdduke PTAL. Refined other UTs as well. https://codereview.chromium.org/521583003/diff/130001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:20: private static final String DATA_URL_1 = UrlUtils.encodeHtmlDataUri( On 2014/09/02 16:05:25, jdduke wrote: > Nit: No need for _1 suffix. Done. https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:60: DOMUtils.clickNode(this, mContentViewCore, "empty_input_text"); On 2014/09/02 16:05:25, jdduke wrote: > Why not clickNode on |plain_text_2|? That way we're not confusing the issue > (whether the handle should show when you tamp an empty editable field). Done. https://codereview.chromium.org/521583003/diff/130001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:89: DOMUtils.longPressNode(this, mContentViewCore, "empty_textarea"); On 2014/09/02 16:05:25, jdduke wrote: > You'll want to insert another valid selection between these last two, you can > just insert > > DOMUtils.longPressNode(this, mContentViewCore, "textarea"); > assertWaitForSelectActionBarStatus(true); > > between the "empty_input_text" and "empty_textarea" long presses. Done.
https://codereview.chromium.org/521583003/diff/150001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/150001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); Why would you add this? As I said before, assertWaitForSelectActionBarStatus(false); should always return immediately after initial setup (in fact, you should put this assertion as the last line in the |setUp()| function. https://codereview.chromium.org/521583003/diff/150001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:60: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); Again this isn't necessary, please remove all of these clicks that you added at the beginning of the tests.
@jdduke PTAL https://codereview.chromium.org/521583003/diff/150001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java (right): https://codereview.chromium.org/521583003/diff/150001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); On 2014/09/02 16:22:44, jdduke wrote: > Why would you add this? As I said before, > assertWaitForSelectActionBarStatus(false); should always return immediately > after initial setup (in fact, you should put this assertion as the last line in > the |setUp()| function. Done. Moved assertWaitForSelectActionBarStatus(false); to setup() function. https://codereview.chromium.org/521583003/diff/150001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:60: DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); On 2014/09/02 16:22:44, jdduke wrote: > Again this isn't necessary, please remove all of these clicks that you added at > the beginning of the tests. Removed all assertWaitForSelectActionBarStatus(false); from starting of test due to trivial pass. Thanks
On 2014/09/02 16:35:58, AKV wrote: > @jdduke PTAL > > https://codereview.chromium.org/521583003/diff/150001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java > (right): > > https://codereview.chromium.org/521583003/diff/150001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:49: > DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); > On 2014/09/02 16:22:44, jdduke wrote: > > Why would you add this? As I said before, > > assertWaitForSelectActionBarStatus(false); should always return immediately > > after initial setup (in fact, you should put this assertion as the last line > in > > the |setUp()| function. > > Done. Moved assertWaitForSelectActionBarStatus(false); to setup() function. > > https://codereview.chromium.org/521583003/diff/150001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:60: > DOMUtils.clickNode(this, mContentViewCore, "plain_text_2"); > On 2014/09/02 16:22:44, jdduke wrote: > > Again this isn't necessary, please remove all of these clicks that you added > at > > the beginning of the tests. > Removed all assertWaitForSelectActionBarStatus(false); from starting of test due > to trivial pass. Thanks @jdduke - Try failed for Last test case testSelectActionBarNotShownOnLongPressingDifferentEmptyInputs at org.chromium.content.browser.ContentViewCoreSelectionTest.assertWaitForSelectActionBarStatus(ContentViewCoreSelectionTest.java:98) org.chromium.content.browser.ContentViewCoreSelectionTest.testSelectActionBarNotShownOnLongPressingDifferentEmptyInputs(ContentViewCoreSelectionTest.java:93) http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri... I have tried with modified UT on try which is passing @ http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes.... Please check whether it's Ok to upload.
@jdduke PTAL this new patch set. This is passing try job @ http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
On 2014/09/02 17:51:35, AKV wrote: > @jdduke PTAL this new patch set. This is passing try job @ > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... lgtm
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/521583003/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001) as 7da8be70e45d91995fb4476713d7920ce77cb90f
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:190001) has been created in https://codereview.chromium.org/549433004/ by arv@chromium.org. The reason for reverting is: This is breaking on Android Tests (dbg) .
Message was sent while issue was closed.
On 2014/09/08 15:22:27, arv wrote: > A revert of this CL (patchset #11 id:190001) has been created in > https://codereview.chromium.org/549433004/ by mailto:arv@chromium.org. > > The reason for reverting is: This is breaking on Android Tests (dbg) > . Interesting, a CQ test failed in android_rel_triggered_tests, yet it still landed.
Message was sent while issue was closed.
On 2014/09/08 15:24:26, jdduke wrote: > On 2014/09/08 15:22:27, arv wrote: > > A revert of this CL (patchset #11 id:190001) has been created in > > https://codereview.chromium.org/549433004/ by mailto:arv@chromium.org. > > > > The reason for reverting is: This is breaking on Android Tests (dbg) > > . > > Interesting, a CQ test failed in android_rel_triggered_tests, yet it still > landed. AKV: To avoid future flakes and failures, we should probably rewrite this test to hook into |ContentViewCore.onSelectionEvent| directly, making it more of a unit test than an integration test, and hopefully avoiding any races and/or underlying changes from the platform.
Message was sent while issue was closed.
On 2014/09/08 15:24:26, jdduke wrote: > On 2014/09/08 15:22:27, arv wrote: > > A revert of this CL (patchset #11 id:190001) has been created in > > https://codereview.chromium.org/549433004/ by mailto:arv@chromium.org. > > > > The reason for reverting is: This is breaking on Android Tests (dbg) > > . > > Interesting, a CQ test failed in android_rel_triggered_tests, yet it still > landed. android_rel_triggered_tests isn't part of CQ, jobs are sent to it as an experiment. however the CQ ignores experimental bots. the UI doesn't show this, which is a problem and causes confusion
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/34ddd8e3947b5ebcce0039ead0e79d4963cbf9c5 Cr-Commit-Position: refs/heads/master@{#292971} |
