Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4698007:
Added IME tests for web forms (Closed)
|
Created:
10 years, 1 month ago by timothe Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, truty+cc_chromium.org, sosa+cc_chromium.org, seano+cc_chromium.org, ericli, petkov+cc_chromium.org, Yusuke Sato Visibility:
Public. |
DescriptionAdded IME tests for web forms
Change-Id: Idd3403eb1f9f5e4632e737729fdfad565f95d3e6
BUG=chromium-os:6921
TEST=Run the autotest desktopui_TestIme
Patch Set 1 #Patch Set 2 : ime testing in web forms #
Total comments: 20
Patch Set 3 : testing IME in web forms with the new settings UI and without the Session object causing timeouts #
Total comments: 24
Patch Set 4 : testing IME in forms and omnibox, now less dependent on previous state and without Session timeout #
Total comments: 6
Patch Set 5 : fixed typos #Patch Set 6 : fixed forgotten typo #Patch Set 7 : var naming style guide compliance #
Messages
Total messages: 11 (0 generated)
Thanks for working on this! I mostly have style nits. You'll need to make some minor changes when you next sync, but it shouldn't be too bad. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (left): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:212: Nit: No need to remove these lines. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:24: Nit: Two lines between functions. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:235: 'Engine %s failed in omnibox: Got %s, expected %s' % (engine_name, text, Nit: Line is greater than 80 characters. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:237: ax.send_hotkey('BackSpace') What does this line add? http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:238: Nit: Two lines between functions. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:252: Nit: Two lines between functions. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:267: #starting a session and waiting for the page to come up Nit: Space between # and starting, capitalize and add a period. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:286: '\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E') Nit: The ' should line up one space after the ( above. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:295: self.test_engine_form('xkb:us::eng', 'asdf', 'asdf') FYI: This line won't be needed once you sync and merge.
http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/control (right): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/control:20: This test checks whether the IME is working properly in the omnibox and web forms. Nit: 80 characters.
http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/control (right): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/control:20: This test checks whether the IME is working properly in the omnibox and web forms. On 2010/11/10 09:16:36, Zachary Kuznia wrote: > Nit: 80 characters. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (left): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:212: On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: No need to remove these lines. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:24: On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: Two lines between functions. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:235: 'Engine %s failed in omnibox: Got %s, expected %s' % (engine_name, text, On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: Line is greater than 80 characters. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:237: ax.send_hotkey('BackSpace') Before we were opening a new tab and closing it, now we just refresh because we want to use the same form at the same address so we need a way to clear the omnibox in case previous tests failed. On 2010/11/10 09:15:55, Zachary Kuznia wrote: > What does this line add? http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:238: On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: Two lines between functions. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:252: On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: Two lines between functions. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:267: #starting a session and waiting for the page to come up On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: Space between # and starting, capitalize and add a period. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:286: '\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E') On 2010/11/10 09:15:55, Zachary Kuznia wrote: > Nit: The ' should line up one space after the ( above. Done. http://codereview.chromium.org/4698007/diff/2001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:295: self.test_engine_form('xkb:us::eng', 'asdf', 'asdf') On 2010/11/10 09:15:55, Zachary Kuznia wrote: > FYI: This line won't be needed once you sync and merge. Done.
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (left): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:79: This blank line should go back in. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:119: # attempting to navigate the UI. This comment shouldn't be re-added. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:174: time.sleep(2) This value shouldn't need to change. Was it failing at 1? http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:293: def test_engine_omnibox(self, language, engine_name, input_string, expected_string): nit: 80 characters. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:311: #clear the omnibox for future tests Nit: Space after #, capitalize Clear, add a period. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:315: def test_engine_form(self, language, engine_name, input_string, expected_string): Nit: 80 characters http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:318: ax.send_hotkey('Ctrl-r') Navigate to the page here, so that it relies less on current state. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:343: # Navigate to the webpage containing the form See above. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:358: self.test_engine('zh-TW', 'm17n:zh:quick', 'aa ', '\xE9\x96\x93') Leave these in the original order to minimize your diff. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:360: self.test_engine('zh-CN', 'pinyin', 'nihao ', '\xE4\xBD\xA0\xE5\xA5\xBD') nit: 80 characters.
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (left): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:79: On 2010/11/16 10:22:58, Zachary Kuznia wrote: > This blank line should go back in. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:119: # attempting to navigate the UI. On 2010/11/16 10:22:58, Zachary Kuznia wrote: > This comment shouldn't be re-added. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:174: time.sleep(2) on my PC sometimes yes, the new language would not be activated with a 1 sec delay. On 2010/11/16 10:22:58, Zachary Kuznia wrote: > This value shouldn't need to change. Was it failing at 1? http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:293: def test_engine_omnibox(self, language, engine_name, input_string, expected_string): On 2010/11/16 10:22:58, Zachary Kuznia wrote: > nit: 80 characters. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:311: #clear the omnibox for future tests On 2010/11/16 10:22:58, Zachary Kuznia wrote: > Nit: Space after #, capitalize Clear, add a period. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:315: def test_engine_form(self, language, engine_name, input_string, expected_string): On 2010/11/16 10:22:58, Zachary Kuznia wrote: > Nit: 80 characters Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:318: ax.send_hotkey('Ctrl-r') On 2010/11/16 10:22:58, Zachary Kuznia wrote: > Navigate to the page here, so that it relies less on current state. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:343: # Navigate to the webpage containing the form On 2010/11/16 10:22:58, Zachary Kuznia wrote: > See above. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:358: self.test_engine('zh-TW', 'm17n:zh:quick', 'aa ', '\xE9\x96\x93') On 2010/11/16 10:22:58, Zachary Kuznia wrote: > Leave these in the original order to minimize your diff. Done. http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:360: self.test_engine('zh-CN', 'pinyin', 'nihao ', '\xE4\xBD\xA0\xE5\xA5\xBD') On 2010/11/16 10:22:58, Zachary Kuznia wrote: > nit: 80 characters. Done.
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:174: time.sleep(2) On 2010/11/17 01:47:05, timothe wrote: > on my PC sometimes yes, the new language would not be activated with a 1 sec > delay. > On 2010/11/16 10:22:58, Zachary Kuznia wrote: > > This value shouldn't need to change. Was it failing at 1? > Ok. Add a comment to note this. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:87: # TODO: Get rid of this function. Nit: 2 lines between functions. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:92: # TODO: Make this function talk to chrome directly Nit: 2 lines between function. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:322: if text != expected_string: Nit: Add spaces around '%'
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:174: time.sleep(2) On 2010/11/17 03:17:46, Zachary Kuznia wrote: > On 2010/11/17 01:47:05, timothe wrote: > > on my PC sometimes yes, the new language would not be activated with a 1 sec > > delay. > > On 2010/11/16 10:22:58, Zachary Kuznia wrote: > > > This value shouldn't need to change. Was it failing at 1? > > > > Ok. Add a comment to note this. Done. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:87: # TODO: Get rid of this function. On 2010/11/17 03:17:46, Zachary Kuznia wrote: > Nit: 2 lines between functions. Done. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:92: # TODO: Make this function talk to chrome directly On 2010/11/17 03:17:46, Zachary Kuznia wrote: > Nit: 2 lines between function. Done. http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:322: if text != expected_string: On 2010/11/17 03:17:46, Zachary Kuznia wrote: > Nit: Add spaces around '%' Done.
On 2010/11/17 04:06:57, timothe wrote: > http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... > File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): > > http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... > client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:174: time.sleep(2) > On 2010/11/17 03:17:46, Zachary Kuznia wrote: > > On 2010/11/17 01:47:05, timothe wrote: > > > on my PC sometimes yes, the new language would not be activated with a 1 sec > > > delay. > > > On 2010/11/16 10:22:58, Zachary Kuznia wrote: > > > > This value shouldn't need to change. Was it failing at 1? > > > > > > > Ok. Add a comment to note this. > > Done. > > http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... > client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:87: # TODO: Get rid of > this function. > On 2010/11/17 03:17:46, Zachary Kuznia wrote: > > Nit: 2 lines between functions. > > Done. > > http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... > client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:92: # TODO: Make this > function talk to chrome directly > On 2010/11/17 03:17:46, Zachary Kuznia wrote: > > Nit: 2 lines between function. > > Done. > > http://codereview.chromium.org/4698007/diff/16001/client/site_tests/desktopui... > client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:322: if text != > expected_string: > On 2010/11/17 03:17:46, Zachary Kuznia wrote: > > Nit: Add spaces around '%' > > Done. LGTM
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:20: self._testServer = site_httpd.HTTPListener(8000, docroot=self.bindir) _test_server per our python style guide.
http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... File client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py (right): http://codereview.chromium.org/4698007/diff/9001/client/site_tests/desktopui_... client/site_tests/desktopui_ImeTest/desktopui_ImeTest.py:20: self._testServer = site_httpd.HTTPListener(8000, docroot=self.bindir) On 2010/11/17 04:35:38, satorux1 wrote: > _test_server per our python style guide. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
