Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2010803005:
Add events tests for inputText in IME test (Closed)
DescriptionAdd events tests for inputText in IME test
BUG=614937
Committed: https://crrev.com/c364ba363241859f8fb92069293e30feee583e8b
Cr-Commit-Position: refs/heads/master@{#399411}
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix a typo in input_forms.html and clear redundant event logs #
Total comments: 8
Patch Set 3 : fix test failure #
Total comments: 1
Patch Set 4 : clear events logs in setUp #
Total comments: 2
Patch Set 5 : split out some logs #Patch Set 6 : sync, try again #
Messages
Total messages: 24 (7 generated)
Description was changed from ========== Add events tests for inputText in IME test BUG=614937 ========== to ========== Add events tests for inputText in IME test BUG=614937 ==========
yabinh@chromium.org changed reviewers: + changwan@chromium.org
changwan@, can you take a look at this patch?
https://codereview.chromium.org/2010803005/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1140: why don't you clear logs after waiting here? Also, you can run this inside setUp() function. https://codereview.chromium.org/2010803005/diff/1/content/test/data/android/i... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2010803005/diff/1/content/test/data/android/i... content/test/data/android/input/input_forms.html:36: // in the future. I don't understand this part. Do we ever get any mutation event? What do you mean when you say 'only the first observed element' will be observed? Why are you registering the same observer twice? https://codereview.chromium.org/2010803005/diff/1/content/test/data/android/i... content/test/data/android/input/input_forms.html:37: var mutationConfig = { attributes: false, childList: true, chracterData: true }; did you try different combinations for input and contenteditable?
After fixing the typo, it can work fine. changwan@, can you take another look?
https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:82: clearEventLogs(); This is likely to cause flakiness. Sometimes logs will be generated after calling this function. You need to call waitForEventLogs() first. https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1123: clearEventLogs(); ditto https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1156: clearEventLogs(); here too https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1182: waitForEventLogs("keydown(229),input,keyup(229)"); no way this isn't creating selectionchange at all. could you add Thread.sleep(1000) before this line and see if test result changes? (same as testInputTextEvents_CommitText) https://codereview.chromium.org/2010803005/diff/20001/content/test/data/andro... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2010803005/diff/20001/content/test/data/andro... content/test/data/android/input/input_forms.html:34: var mutationConfig = { attributes: false, childList: false, characterData: true }; nice catch! by the way, it's still weird that we don't get any mutation event. could you investigate it further? (it doesn't have to be in this CL.)
https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:82: clearEventLogs(); On 2016/05/27 01:46:15, Changwan Ryu wrote: > This is likely to cause flakiness. Sometimes logs will be generated after > calling this function. You need to call waitForEventLogs() first. Acknowledged. https://codereview.chromium.org/2010803005/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1182: waitForEventLogs("keydown(229),input,keyup(229)"); You are right. It can create selectionchange events. Please see patch set 3. https://codereview.chromium.org/2010803005/diff/20001/content/test/data/andro... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2010803005/diff/20001/content/test/data/andro... content/test/data/android/input/input_forms.html:34: var mutationConfig = { attributes: false, childList: false, characterData: true }; On 2016/05/27 01:46:15, Changwan Ryu wrote: > nice catch! by the way, it's still weird that we don't get any mutation event. > could you investigate it further? (it doesn't have to be in this CL.) Acknowledged.
https://codereview.chromium.org/2010803005/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1170: waitForEventLogs("selectionchange"); could you move these two lines to setup?
changwan@, can you take another look at this patch?
https://codereview.chromium.org/2010803005/diff/60001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/60001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1182: waitForEventLogs("selectionchange,selectionchange,keydown(229),input," could you split this out?
https://codereview.chromium.org/2010803005/diff/60001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2010803005/diff/60001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1182: waitForEventLogs("selectionchange,selectionchange,keydown(229),input," On 2016/05/30 05:37:51, Changwan Ryu wrote: > could you split this out? OK. please check patch set 5.
changwan@chromium.org changed reviewers: + sievers@chromium.org
lgtm. adding sievers@ for content/ approval
for the record, this shouldn't be merged before crbug.com/601707 is fixed since we found that some test results are flaky. yabinh@, could you also check that the flakiness can be avoided when https://codereview.chromium.org/1877073003/ is patched?
lgtm stamp
On 2016/06/01 08:00:13, Changwan Ryu wrote: > for the record, this shouldn't be merged before crbug.com/601707 is fixed since > we found that some test results are flaky. > > yabinh@, could you also check that the flakiness can be avoided when > https://codereview.chromium.org/1877073003/ is patched? I have checked. After applying the above patch, the flakiness of test*Events_DeleteSurroundingText() doesn't exist any more.
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from changwan@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2010803005/#ps100001 (title: "sync, try again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010803005/100001
Message was sent while issue was closed.
Description was changed from ========== Add events tests for inputText in IME test BUG=614937 ========== to ========== Add events tests for inputText in IME test BUG=614937 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add events tests for inputText in IME test BUG=614937 ========== to ========== Add events tests for inputText in IME test BUG=614937 Committed: https://crrev.com/c364ba363241859f8fb92069293e30feee583e8b Cr-Commit-Position: refs/heads/master@{#399411} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c364ba363241859f8fb92069293e30feee583e8b Cr-Commit-Position: refs/heads/master@{#399411} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
