Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1004283002:
Add end-to-end testing tool for Smart Lock. (Closed)
|
Created:
5 years, 9 months ago by Tim Song Modified:
5 years, 9 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd end-to-end testing tool for Smart Lock.
This tool is based on telemetry and exercises the Smart Lock setup flow. This CL
also introduces the framework that will allow implementing other end-to-end
tests.
BUG=chromium:442728
NOTRY=true
Committed: https://crrev.com/7b2cf72484889c7098c2b380a3815915e6d11aca
Cr-Commit-Position: refs/heads/master@{#322604}
Patch Set 1 #
Total comments: 42
Patch Set 2 : fixes #
Total comments: 2
Patch Set 3 : style #
Messages
Total messages: 11 (3 generated)
tengs@chromium.org changed reviewers: + isherman@chromium.org
Thanks, Tim. Lots of Python nits, but overall looks pretty good! =) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:12: curr_dir = os.path.dirname(os.path.realpath(__file__)) nit: s/curr_dir/current_directory or at least current_dir https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:29: screen and signin screen. nit: The first line is supposed to fit on a single line. So, I'd write this like so: """ Wrapper for the ChromeOS account picker screen. The account picker screen is used on both the lock screen and the sign-in screen. """ https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:34: """ nit: No need to wrap the closing """ (applies throughout for one-line doc strings) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:63: return self._oobe.EvaluateJavaScript( nit: Extra space after "return" https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:69: 'document.getElementById("pod-row").pods[0].authType') nit: Maybe define a string constant for "document.getElementById("pod-row").pods[0]"? https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:94: """ Waits for the Smart Lock icon to reach the given state. nit: Please leave a blank line after this one. (Applies throughout, for all lines following the single-line doc string.) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:95: Note: The first user pod on the screen is used. nit: This note seems to apply to most of the methods in the class. Maybe it should be moved up? https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:111: Note: The first user pod on the screen is used. nit: Please document that this can throw an exception. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:131: nit: I believe that top-level definitions are supposed to have two newlines above them. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): nit: Maybe prepend "is_" to the method name? https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): nit: Doc string https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:152: """ nit: Please document that this can throw an exception. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:179: """ nit: Please document that this can throw an exception. (Applies for all remaining unguarded uses of WaitFor as well.) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:211: def pairing_state(self): nit: Doc string, including exception warning. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:246: time.sleep(10) Ick. What's the total runtime for the test? https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:316: def username(self): nit: Doc string. (Applies to all other undocumented methods as well.) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:329: self.SessionState.SIGNIN_SCREEN nit: Please use a regular if/else since this spans three lines anyway. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:446: smart_lock_url = 'chrome://settings/search#Smart%20Lock' nit: NAMED_CONSTANT https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cryptauth.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cryptauth.py:92: 'eligibleDevices' in response else [] nit: I think this is normally wrapped like so: eligibleDevices = ( response['eligibleDevices'] if 'eligibleDevices' in response else []) (Applies to other uses of "\" for wrapping as well.) https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/setup_test.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/setup_test.py:91: logger.inro('Pinging phones succeeded!') nit: logger.info. This is why I prefer compiled languages :P
https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:12: curr_dir = os.path.dirname(os.path.realpath(__file__)) On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: s/curr_dir/current_directory or at least current_dir Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:29: screen and signin screen. On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: The first line is supposed to fit on a single line. So, I'd write this > like so: > > """ Wrapper for the ChromeOS account picker screen. > > The account picker screen is used on both the lock screen and the sign-in > screen. > """ Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:34: """ On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: No need to wrap the closing """ (applies throughout for one-line doc > strings) Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:63: return self._oobe.EvaluateJavaScript( On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: Extra space after "return" Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:69: 'document.getElementById("pod-row").pods[0].authType') On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Maybe define a string constant for > "document.getElementById("pod-row").pods[0]"? Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:94: """ Waits for the Smart Lock icon to reach the given state. On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: Please leave a blank line after this one. (Applies throughout, for all > lines following the single-line doc string.) Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:95: Note: The first user pod on the screen is used. On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: This note seems to apply to most of the methods in the class. Maybe it > should be moved up? Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:111: Note: The first user pod on the screen is used. On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Please document that this can throw an exception. Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:131: On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: I believe that top-level definitions are supposed to have two newlines > above them. Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: Doc string Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: Maybe prepend "is_" to the method name? I have an assert in the test class to make sure that Smart Lock is off by default. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:152: """ On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Please document that this can throw an exception. Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:179: """ On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Please document that this can throw an exception. (Applies for all > remaining unguarded uses of WaitFor as well.) Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:211: def pairing_state(self): On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Doc string, including exception warning. Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:246: time.sleep(10) On 2015/03/24 01:25:14, Ilya Sherman wrote: > Ick. What's the total runtime for the test? The test can take 30-60 seconds to run, depending on if the phone receives the GCM message and connects. Most of the time, the first try should find the phone, but if not, trying again immediately doesn't seem to really help. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:316: def username(self): On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: Doc string. (Applies to all other undocumented methods as well.) Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:329: self.SessionState.SIGNIN_SCREEN On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: Please use a regular if/else since this spans three lines anyway. Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:446: smart_lock_url = 'chrome://settings/search#Smart%20Lock' On 2015/03/24 01:25:13, Ilya Sherman wrote: > nit: NAMED_CONSTANT Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cryptauth.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cryptauth.py:92: 'eligibleDevices' in response else [] On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: I think this is normally wrapped like so: > > eligibleDevices = ( > response['eligibleDevices'] if 'eligibleDevices' in response else []) > > (Applies to other uses of "\" for wrapping as well.) Done. https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/setup_test.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/setup_test.py:91: logger.inro('Pinging phones succeeded!') On 2015/03/24 01:25:14, Ilya Sherman wrote: > nit: logger.info. This is why I prefer compiled languages :P Done. I agree!
LGTM % a final few nits: https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): On 2015/03/25 23:08:43, Tim Song wrote: > On 2015/03/24 01:25:13, Ilya Sherman wrote: > > nit: Maybe prepend "is_" to the method name? > > I have an assert in the test class to make sure that Smart Lock is off by > default. I just meant, "is_smart_lock_enabled" sounds more like a boolean method name than "smart_lock_enabled", which is more ambiguous. https://codereview.chromium.org/1004283002/diff/20001/components/proximity_au... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/20001/components/proximity_au... components/proximity_auth/e2e_test/cros.py:119: session. The Python style is to list "Raises:" just as you list "Args:" and "Returns:". Take a gander at the style guide for examples.
https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/1/components/proximity_auth/e... components/proximity_auth/e2e_test/cros.py:145: def smart_lock_enabled(self): On 2015/03/25 23:17:29, Ilya Sherman wrote: > On 2015/03/25 23:08:43, Tim Song wrote: > > On 2015/03/24 01:25:13, Ilya Sherman wrote: > > > nit: Maybe prepend "is_" to the method name? > > > > I have an assert in the test class to make sure that Smart Lock is off by > > default. > > I just meant, "is_smart_lock_enabled" sounds more like a boolean method name > than "smart_lock_enabled", which is more ambiguous. Done. https://codereview.chromium.org/1004283002/diff/20001/components/proximity_au... File components/proximity_auth/e2e_test/cros.py (right): https://codereview.chromium.org/1004283002/diff/20001/components/proximity_au... components/proximity_auth/e2e_test/cros.py:119: session. On 2015/03/25 23:17:29, Ilya Sherman wrote: > The Python style is to list "Raises:" just as you list "Args:" and "Returns:". > Take a gander at the style guide for examples. Done.
The CQ bit was checked by tengs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1004283002/#ps40001 (title: "style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004283002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7b2cf72484889c7098c2b380a3815915e6d11aca Cr-Commit-Position: refs/heads/master@{#322604} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
