Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4669008:
Addes Two tests to Passwords.py... (Closed)
|
Created:
10 years, 1 month ago by Venkat Yellapu Modified:
9 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionAdded Two tests to Passwords.py
Added back testDisplayAndSavePasswordInfobar.
BUG=NONE
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70798
Patch Set 1 #Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 12
Patch Set 5 : '' #
Total comments: 12
Patch Set 6 : '' #
Total comments: 3
Patch Set 7 : '' #
Total comments: 1
Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 8
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 2
Patch Set 14 : '' #Messages
Total messages: 16 (0 generated)
I've uploaded two password pyauto tests: testInfoBarDisappearByNavigatingPage testInfoBarDisappearByReload Please review and let me know the comments.
http://codereview.chromium.org/4669008/diff/2001/passwords.py File passwords.py (right): http://codereview.chromium.org/4669008/diff/2001/passwords.py#newcode50 passwords.py:50: self.ExecuteJavascript('document.getElementById("Email").value = "etouchqa"; window.domAutomationController.send("done")') Please do not use these usernames/passwds in tests. Hold on for now while I work on a general solution to these. http://codereview.chromium.org/4669008/diff/2001/passwords.py#newcode56 passwords.py:56: self.assertTrue(self.GetBrowserInfo()['windows'][0]['tabs'][0]['infobars']) >80 chars not allowed. Fix elsewhere
Reflected the changes as per the comments. Thanks.
http://codereview.chromium.org/4669008/diff/9001/passwords.py File passwords.py (right): http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode9 passwords.py:9: import re move these before pyauto imports, then leave a line http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode47 passwords.py:47: def testInfoBarDisappearByNavigatingPage(self): Why are these tests in this file instead of infobars.py http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode50 passwords.py:50: A bulk of the login function in this and next test appears common. Please refactor to a common function http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode72 passwords.py:72: logging.info('--------------- %s ' % str(self.GetBrowserInfo()['windows'][0] remove this http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode100 passwords.py:100: logging.info('--------------- %s ' % str(self.GetBrowserInfo()['windows'][0] remove this http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode104 passwords.py:104: def testRemoveAllPasswords(self): remove this test for now.
Sorry for delayed reply. Reflected all the changes as per the comments. PS: As you know, the test testInfoBarDisappearByNavigatingPage needs to be disabled until the issue crbug.com\63033 is fixed. please review it and let me know your comments. Thanks, Venkat. http://codereview.chromium.org/4669008/diff/9001/passwords.py File passwords.py (right): http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode9 passwords.py:9: import re On 2010/11/10 07:53:10, Nirnimesh wrote: > move these before pyauto imports, then leave a line Done. http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode47 passwords.py:47: def testInfoBarDisappearByNavigatingPage(self): On 2010/11/10 07:53:10, Nirnimesh wrote: > Why are these tests in this file instead of infobars.py Done. http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode50 passwords.py:50: On 2010/11/10 07:53:10, Nirnimesh wrote: > A bulk of the login function in this and next test appears common. Please > refactor to a common function Done. http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode72 passwords.py:72: logging.info('--------------- %s ' % str(self.GetBrowserInfo()['windows'][0] On 2010/11/10 07:53:10, Nirnimesh wrote: > remove this Done. http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode100 passwords.py:100: logging.info('--------------- %s ' % str(self.GetBrowserInfo()['windows'][0] On 2010/11/10 07:53:10, Nirnimesh wrote: > remove this Done. http://codereview.chromium.org/4669008/diff/9001/passwords.py#newcode104 passwords.py:104: def testRemoveAllPasswords(self): On 2010/11/10 07:53:10, Nirnimesh wrote: > remove this test for now. Done.
Add the entry to be disabled in PYAUTO_TESTS (see other examples in that file) http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py File functional/passwords.py (right): http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:8: leave another blank line http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:44: def _Login(self, url, username, password): use a similar function from test_utils http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:55: self.ExecuteJavascript('document.getElementById("gaia_loginform").submit(); ' 80+ chars http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:59: """Test that Password info is dismissed by navigating to different page.""" s/info/infobar/ http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:61: url = 'https://www.google.com/accounts/Login?hl=en&continue=https://www.google.com/' 80+ chars http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:66: #Login to Google a/c leave a space after # Repeat everywhere http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:70: self.WaitForInfobarCount(1) wrap this inside self.assertTrue() http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:73: self.WaitForInfobarCount(0) wrap this inside self.assertTrue() http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:74: self.assertEqual('History', self.GetActiveTabTitle()) why? http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:76: remove extra blank line http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:79: """Test that Password info bar disappears by the page reload.""" s/info bar/infobar/ http://codereview.chromium.org/4669008/diff/14001/functional/passwords.py#new... functional/passwords.py:81: url = 'https://www.google.com/accounts/Login?hl=en&continue=https://www.google.com/' 80+ chars
Reflected all the comments. And also disabled the test 'testInfoBarDisappearByNavigatingPage' in PYAUTO_TESTS. Thanks.
http://codereview.chromium.org/4669008/diff/20001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/4669008/diff/20001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:62: # crbug.com/63033 please move this to the 'win' & 'linux' sections. all passwords tests are disabled on Mac http://codereview.chromium.org/4669008/diff/20001/functional/passwords.py File functional/passwords.py (right): http://codereview.chromium.org/4669008/diff/20001/functional/passwords.py#new... functional/passwords.py:51: '=https://www.google.com/' what is this line? http://codereview.chromium.org/4669008/diff/20001/functional/passwords.py#new... functional/passwords.py:71: 'continue=https://www.google.com/' you don't need this line
Reflected.
http://codereview.chromium.org/4669008/diff/27001/functional/passwords.py File functional/passwords.py (left): http://codereview.chromium.org/4669008/diff/27001/functional/passwords.py#old... functional/passwords.py:46: def testDisplayAndSavePasswordInfobar(self): This old test looked useful to me as well. It might be nice to preserve it. You could use GoogleAccountsLogin() in it though.
Added back the test testDisplayAndSavePasswordInfobar.
http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py File functional/passwords.py (right): http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:52: test_utils.GoogleAccountsLogin(self, url_https, creds['username'], why not move all args to the next line http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:55: self.WaitUntil( remove stray tab char http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:56: lambda: self.GetDOMValue('document.readyState'), 'complete') indent by 4 spaces http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:58: 'Did not get save password infobar') Keep this aligned under self. http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:64: test_utils.VerifyGoogleAccountCredsFilled(self, creds['username'], move all args to next line so that it looks better http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:72: different page.""" align under Test http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:78: test_utils.GoogleAccountsLogin(self, url, creds['username'], this fn doesn't take the url arg any more. Please re-sync http://codereview.chromium.org/4669008/diff/33001/functional/passwords.py#new... functional/passwords.py:97: test_utils.GoogleAccountsLogin(self, url, creds['username'], ditto
Reflected the comments. Please note, When I re-synced the test 'testDisplayAndSavePasswordInfobar' was already checked-in. So I deleted it from my version. Thanks.
LGTM with 2 minor nits. Please fix and upload and let me know so I can commit. Thanks http://codereview.chromium.org/4669008/diff/45001/functional/passwords.py File functional/passwords.py (right): http://codereview.chromium.org/4669008/diff/45001/functional/passwords.py#new... functional/passwords.py:159: remove blank line http://codereview.chromium.org/4669008/diff/45001/functional/passwords.py#new... functional/passwords.py:174: remove blank line
Reflected the changes. Thanks.
Committed |
