Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137783008:
testOobeLocalization implementation (Closed)
|
Created:
6 years, 11 months ago by michaelpg Modified:
6 years, 11 months ago Reviewers:
achuithb CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptiontestOobeLocalization implementation
Test that the proper languages and keyboards are displayed at OOBE for
each supported region.
BUG=327034
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245609
Patch Set 1 #
Total comments: 17
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Change regions #Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:169: self._cri.RunCmdOnDevice(['vpd', '-s', '"%s"="%s"' % item]) How does this work? 2 %s and only 1 item? https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:174: self._cri.RunCmdOnDevice(['rm', '-rf', '/home/chronos/*']) Which files do we really need to remove? Should we be using cryptohome remove functions instead? https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:181: js = ''' Let's add a comment explaining what the js is doing. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:189: })("%s", "%s", %s); Any way we can be consistent with using " or not? https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:191: self.assertTrue(browser.oobe.EvaluateJavaScript( Let's just return true/false from this function and assert in the calling function https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:218: keyboard_mechanical_layout): Are we not using keyboard_mechnical_layout?
https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:169: self._cri.RunCmdOnDevice(['vpd', '-s', '"%s"="%s"' % item]) On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > How does this work? 2 %s and only 1 item? Each item is a (key, value) pair. I could write this more verbosely as: for (key, value) in region.__dict__.items(): self._cri.RunCmdOnDevice(['vpd', '-s', '"%s"="%s"' % (key, value)]) https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:174: self._cri.RunCmdOnDevice(['rm', '-rf', '/home/chronos/*']) On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > Which files do we really need to remove? Should we be using cryptohome remove > functions instead? 'Local State' and .oobe_completed must be removed. dpolukhin suggested removing all files to be safe. I don't have a problem with going back to removing the two files I know about and leaving it that way unless we start to see test failures. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:181: js = ''' On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > Let's add a comment explaining what the js is doing. Done. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:189: })("%s", "%s", %s); On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > Any way we can be consistent with using " or not? The JS function takes two strings and one boolean - I don't see a good reason to change that :-\ https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:191: self.assertTrue(browser.oobe.EvaluateJavaScript( On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > Let's just return true/false from this function and assert in the calling > function Done. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:218: keyboard_mechanical_layout): On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > Are we not using keyboard_mechnical_layout? Nope, it's there but doesn't seem to be used outside of tests. Should I replace the arg with a '_' in the function definition? https://codereview.chromium.org/137783008/diff/90001/chrome/test/telemetry/ch... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/90001/chrome/test/telemetry/ch... chrome/test/telemetry/chromeos/login_unittest.py:215: browser, 'keyboard-select', region.keyboard_layout)) How does this indentation look?
https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:169: self._cri.RunCmdOnDevice(['vpd', '-s', '"%s"="%s"' % item]) On 2014/01/15 02:21:55, Michael Giuffrida wrote: > On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > > How does this work? 2 %s and only 1 item? > > Each item is a (key, value) pair. I could write this more verbosely as: > > for (key, value) in region.__dict__.items(): > self._cri.RunCmdOnDevice(['vpd', '-s', '"%s"="%s"' % (key, value)]) That may be clearer, or maybe just add a comment saying item is a tuple. I guess it should be obvious now that I think about it. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:174: self._cri.RunCmdOnDevice(['rm', '-rf', '/home/chronos/*']) On 2014/01/15 02:21:55, Michael Giuffrida wrote: > On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > > Which files do we really need to remove? Should we be using cryptohome remove > > functions instead? > > 'Local State' and .oobe_completed must be removed. dpolukhin suggested removing > all files to be safe. I don't have a problem with going back to removing the two > files I know about and leaving it that way unless we start to see test failures. I think that may be best? Just remove the 2 files for now? https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:218: keyboard_mechanical_layout): On 2014/01/15 02:21:55, Michael Giuffrida wrote: > On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > > Are we not using keyboard_mechnical_layout? > > Nope, it's there but doesn't seem to be used outside of tests. > > Should I replace the arg with a '_' in the function definition? Maybe we can just save it in the object in case we want to check it in the future? https://codereview.chromium.org/137783008/diff/90001/chrome/test/telemetry/ch... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/90001/chrome/test/telemetry/ch... chrome/test/telemetry/chromeos/login_unittest.py:215: browser, 'keyboard-select', region.keyboard_layout)) On 2014/01/15 02:21:55, Michael Giuffrida wrote: > How does this indentation look? Technically I believe self._OobeHasOption should be on a separate line? Anyway this looks fine.
Updated the Region class and list of regions from chromeos regions.py. Also mirrored Chrome OS's logic in selecting fallback languages when it's the right thing to do. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... File chrome/test/telemetry/chromeos/login_unittest.py (right): https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:174: self._cri.RunCmdOnDevice(['rm', '-rf', '/home/chronos/*']) On 2014/01/15 19:52:11, achuith.bhandarkar wrote: > On 2014/01/15 02:21:55, Michael Giuffrida wrote: > > On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > > > Which files do we really need to remove? Should we be using cryptohome > remove > > > functions instead? > > > > 'Local State' and .oobe_completed must be removed. dpolukhin suggested > removing > > all files to be safe. I don't have a problem with going back to removing the > two > > files I know about and leaving it that way unless we start to see test > failures. > > I think that may be best? Just remove the 2 files for now? Done. https://codereview.chromium.org/137783008/diff/1/chrome/test/telemetry/chrome... chrome/test/telemetry/chromeos/login_unittest.py:218: keyboard_mechanical_layout): On 2014/01/15 19:52:11, achuith.bhandarkar wrote: > On 2014/01/15 02:21:55, Michael Giuffrida wrote: > > On 2014/01/15 01:46:59, achuith.bhandarkar wrote: > > > Are we not using keyboard_mechnical_layout? > > > > Nope, it's there but doesn't seem to be used outside of tests. > > > > Should I replace the arg with a '_' in the function definition? > > Maybe we can just save it in the object in case we want to check it in the > future? Done.
lgtm. Let's get this landed so we can create the autotest.
On 2014/01/17 20:38:58, achuith.bhandarkar wrote: > lgtm. > > Let's get this landed so we can create the autotest. You can edit the description to add NOTRY=True since this will not break the build.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/137783008/170002
Message was sent while issue was closed.
Change committed as 245609 |
