Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10152004:
Add tests for one-click sign in infobar. (Closed)
|
Created:
8 years, 8 months ago by dyu1 Modified:
8 years, 8 months ago CC:
chromium-reviews, dennis_jeffrey, anantha Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd tests for one-click sign in infobar.
Triaged the bugs for one-click sign in and automated what was available.
Current set of automated tests do not include scenarios where sync is accepted as
there may be a UI change down the road related to those tests.
-testDisplayOneClickInfobar
-testNoOneClickInfobarAfterCancel
-testDisplayOneClickInfobarAfterDismiss
-testDisplayOneClickInfobarPerProfile
-testNoSameIDSigninForTwoProfiles
-testNoOneClickInfobarWhenCookiesBlocked
-testOneClickInfobarShownWhenWinLoseFocus
TESTS=none
BUG=124326
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133718
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 42
Patch Set 3 : #
Total comments: 19
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 26
Patch Set 6 : #Messages
Total messages: 17 (0 generated)
@rogerta: Please check test coverage and suggest additional tests if needed. I didn't write any tests that interacts with the custom dialog box based on our discussion.
What do these tests have to do with sync? (since you've put it in sync.py instead of infobars.py) http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:10: from webdriver_pages.settings import Behaviors, ContentTypes unused http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:264: lambda: self.ExecuteJavascript(js_template % 'Email', lambda should not be used with multiline functionals. Move them to a function http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:425: self.RunCommand(pyauto.IDC_NEW_WINDOW) use pyauto call: OpenNewBrowserWindow()
Well, one-click sign in infobar is a sync feature. If you want it under infobars.py I can move it there but later on there may be sync integration for additional tests. On 2012/04/20 00:43:13, Nirnimesh wrote: > What do these tests have to do with sync? (since you've put it in sync.py > instead of infobars.py) > > http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py > File chrome/test/functional/sync.py (right): > > http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... > chrome/test/functional/sync.py:10: from webdriver_pages.settings import > Behaviors, ContentTypes > unused > > http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... > chrome/test/functional/sync.py:264: lambda: self.ExecuteJavascript(js_template % > 'Email', > lambda should not be used with multiline functionals. Move them to a function > > http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... > chrome/test/functional/sync.py:425: self.RunCommand(pyauto.IDC_NEW_WINDOW) > use pyauto call: > OpenNewBrowserWindow()
On 2012/04/20 00:45:39, dyu1 wrote: > Well, one-click sign in infobar is a sync feature. If you want it under > infobars.py I can move it there but later on there may be sync integration for > additional tests. I don't see your tests using anything sync-related. Do you have to be signed in to sync for this feature to work?
I disabled one test since the fix was just checked in today. http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:10: from webdriver_pages.settings import Behaviors, ContentTypes On 2012/04/20 00:43:13, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:264: lambda: self.ExecuteJavascript(js_template % 'Email', Turns out this function is not even used anywhere in my tests. I think I originally had this function when working on logging into sync but postponed those tests based on this bug 124258. On 2012/04/20 00:43:13, Nirnimesh wrote: > lambda should not be used with multiline functionals. Move them to a function http://codereview.chromium.org/10152004/diff/1/chrome/test/functional/sync.py... chrome/test/functional/sync.py:425: self.RunCommand(pyauto.IDC_NEW_WINDOW) On 2012/04/20 00:43:13, Nirnimesh wrote: > use pyauto call: > OpenNewBrowserWindow() Done.
lgtm These are awesome tests David! Thanks for writing them. The one case missing is testing incognito: the one-click infobar should never show up in incognito mode. Is it possible for you to test that?
http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/PYAU... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/PYAU... chrome/test/functional/PYAUTO_TESTS:101: '-infobars.OneClickInfobarTest.testNoSameIDSigninForTwoProfiles', It looks like this issue was marked as fixed recently. Maybe we shouldn't disable it? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:192: URL_HTTPS = 'https://www.google.com/accounts/Login' maybe name this URL_LOGIN to correspond with URL_LOGOUT below? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:226: self, self.OC_INFOBAR_TYPE)) Probably not worth declaring this as a function. It's only called twice below, and the function consists of a single statement anyway. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:256: """Verify one-click infobar appear again after clicking dismiss button. 'appear' --> 'appears' http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:262: The one-click infobar should superceed the password infobar. 'superceed' --> 'supersede' http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:275: # TODO: Remove when crbug.com/108761 is fixed. add LDAP to the TODO http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:288: # TODO: Remove when crbug.com/108761 is fixed. add LDAP http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:299: and connect it to a Google account. Another new profile is created and 'connect' --> 'connects' http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:302: Regress crbug.com/122975 what does this mean? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:308: # TODO: Remove when crbug.com/108761 is fixed. add LDAP
Great tests! http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:206: test_utils.ClearPasswords(self) Each test begins with a clean profile. This is not necessary. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:235: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( Use WaitForInfobarCount(1) Then verify that it's the right kind of infobar. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:236: self, self.OC_INFOBAR_TYPE) is not None)) add a msg= param http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:247: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( just call test testDisplayOneClickInfobar() instead of lines 246-248 http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:266: self, self.OC_INFOBAR_TYPE) is not None)) use testDisplayOneClickInfobar. Repeat elsewhere http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:289: self.WaitUntil(self._CheckNumProfiles, args=[2]) # Verify 2 profiles exist. wrap inside assertTrue http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:304: test_utils.SignInToSyncAndVerifyState(self, 'test_google_account') The sync tests are disabled. Are you sure this works? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:306: self.OpenNewBrowserWindowWithNewProfile() I'd suggest creating a local wrapper function with lines 306-308, and use it everywhere http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:314: def _VerifyContentExceptionUI(self, content_type, hostname_pattern, behavior, unused? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:345: {'https://accounts.google.com/': {'cookies': 2}}) move the pattern at the top, along with all the other URLs. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:358: test_utils.WaitForInfobarTypeAndGetIndex(self, self.OC_INFOBAR_TYPE) you should not "wait" anymore. Just check that the OC infobar exists.
addressed all the comments and added a test to verify one-click infobar does not appear for incognito mode. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/PYAU... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/PYAU... chrome/test/functional/PYAUTO_TESTS:101: '-infobars.OneClickInfobarTest.testNoSameIDSigninForTwoProfiles', On 2012/04/20 17:35:04, dennis_jeffrey wrote: > It looks like this issue was marked as fixed recently. Maybe we shouldn't > disable it? Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:192: URL_HTTPS = 'https://www.google.com/accounts/Login' On 2012/04/20 17:35:04, dennis_jeffrey wrote: > maybe name this URL_LOGIN to correspond with URL_LOGOUT below? Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:206: test_utils.ClearPasswords(self) On 2012/04/20 18:12:05, Nirnimesh wrote: > Each test begins with a clean profile. This is not necessary. Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:226: self, self.OC_INFOBAR_TYPE)) Yes I thought about it, I left it for later, when the UI after accept is sorted out I will have an IF statement in this function. Right now the only action being passed in are cancel and dismiss. If the accept action gets passed in then there will be a need for additional statements. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > Probably not worth declaring this as a function. It's only called twice below, > and the function consists of a single statement anyway. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:235: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( I'm hesitant on using WaitForInfobarCount(1) since if multiple infobars appear then this will assert an error. GetInfobarIndexByType will wait to check each infobar that appears. On 2012/04/20 18:12:05, Nirnimesh wrote: > Use WaitForInfobarCount(1) > > Then verify that it's the right kind of infobar. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:236: self, self.OC_INFOBAR_TYPE) is not None)) On 2012/04/20 18:12:05, Nirnimesh wrote: > add a msg= param Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:247: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( On 2012/04/20 18:12:05, Nirnimesh wrote: > just call test testDisplayOneClickInfobar() instead of lines 246-248 Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:256: """Verify one-click infobar appear again after clicking dismiss button. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > 'appear' --> 'appears' Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:262: The one-click infobar should superceed the password infobar. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > 'superceed' --> 'supersede' Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:266: self, self.OC_INFOBAR_TYPE) is not None)) On 2012/04/20 18:12:05, Nirnimesh wrote: > use testDisplayOneClickInfobar. Repeat elsewhere Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:275: # TODO: Remove when crbug.com/108761 is fixed. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > add LDAP to the TODO Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:288: # TODO: Remove when crbug.com/108761 is fixed. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > add LDAP Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:289: self.WaitUntil(self._CheckNumProfiles, args=[2]) # Verify 2 profiles exist. On 2012/04/20 18:12:05, Nirnimesh wrote: > wrap inside assertTrue Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:299: and connect it to a Google account. Another new profile is created and On 2012/04/20 17:35:04, dennis_jeffrey wrote: > 'connect' --> 'connects' Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:302: Regress crbug.com/122975 I wrote a test for that bug. So if the test fails then it's a regression for bug 122975. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > what does this mean? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:304: test_utils.SignInToSyncAndVerifyState(self, 'test_google_account') Yes, this function works. This is the only way to physically log into an account. Sync does not have to work, as long as I'm logged in with a user account for profile 1. On 2012/04/20 18:12:05, Nirnimesh wrote: > The sync tests are disabled. Are you sure this works? http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:306: self.OpenNewBrowserWindowWithNewProfile() Do you mean 306 to 309? 307 and 308 are just comments. You want a private function? It's only being used in two places. On 2012/04/20 18:12:05, Nirnimesh wrote: > I'd suggest creating a local wrapper function with lines 306-308, and use it > everywhere http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:308: # TODO: Remove when crbug.com/108761 is fixed. On 2012/04/20 17:35:04, dennis_jeffrey wrote: > add LDAP Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:314: def _VerifyContentExceptionUI(self, content_type, hostname_pattern, behavior, On 2012/04/20 18:12:05, Nirnimesh wrote: > unused? Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:345: {'https://accounts.google.com/': {'cookies': 2}}) On 2012/04/20 18:12:05, Nirnimesh wrote: > move the pattern at the top, along with all the other URLs. Done. http://codereview.chromium.org/10152004/diff/5005/chrome/test/functional/info... chrome/test/functional/infobars.py:358: test_utils.WaitForInfobarTypeAndGetIndex(self, self.OC_INFOBAR_TYPE) On 2012/04/20 18:12:05, Nirnimesh wrote: > you should not "wait" anymore. Just check that the OC infobar exists. Done.
http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:248: self.testDisplayOneClickInfobar need parens? Also, this is not a big deal, but I'm not a fan of invoking one test from another, because logically it seems weird to me to have tests depend on each other. I'd recommend that you take the functionality you need from that other test and separate it out into a helper function, then call that helper function in the other test, as well as here. But I'll leave that up to you if you want to do it. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:264: self.testDisplayOneClickInfobar need parens? http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:280: self.testDisplayOneClickInfobar need parens? http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:288: msg='One-click login infobar appeared for only one profile.') are you sure this failure message will necessarily be the correct explanation? What if 0 or 3+ infobars appear? http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:324: # {'https://accounts.google.com/': {'cookies': 2}}) remove commented-out code?
http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:212: self.WaitUntil( Suggestion: Craig recently (actually, 2 hours ago - http://src.chromium.org/viewvc/chrome?view=rev&revision=133499) added an automation hook that makes this better. Instead of the WaitUntil here, you can call: self.WaitUntilNavigationCompletes(tab_index=tab_index, windex=windex) This guarantees that the tab has finished loading completely, and is ready to inject js, WITHOUT polling. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:228: def testDisplayOneClickInfobar(self, tab_index=0, windex=0): what are the args for? I don't see them used anywhere http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:234: self._LogIntoGoogleAccount(tab_index=0, windex=0) pass along tab_index & windex, instead of hardcoding to 0 http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:248: self.testDisplayOneClickInfobar this is a no-op. You didn't call it.
http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:212: self.WaitUntil( On 2012/04/23 22:07:45, Nirnimesh wrote: > Suggestion: > > Craig recently (actually, 2 hours ago - > http://src.chromium.org/viewvc/chrome?view=rev&revision=133499) added an > automation hook that makes this better. > Instead of the WaitUntil here, you can call: > self.WaitUntilNavigationCompletes(tab_index=tab_index, windex=windex) > > This guarantees that the tab has finished loading completely, and is ready to > inject js, WITHOUT polling. Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:228: def testDisplayOneClickInfobar(self, tab_index=0, windex=0): Used on line 289. On 2012/04/23 22:07:45, Nirnimesh wrote: > what are the args for? I don't see them used anywhere http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:234: self._LogIntoGoogleAccount(tab_index=0, windex=0) On 2012/04/23 22:07:45, Nirnimesh wrote: > pass along tab_index & windex, instead of hardcoding to 0 Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:248: self.testDisplayOneClickInfobar On 2012/04/23 22:07:45, Nirnimesh wrote: > this is a no-op. You didn't call it. Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:248: self.testDisplayOneClickInfobar Yes I thought of that originally but then that one test is essentially a one liner from the helper function. Which is the worst of the two evils? On 2012/04/23 21:59:52, dennis_jeffrey wrote: > need parens? > > Also, this is not a big deal, but I'm not a fan of invoking one test from > another, because logically it seems weird to me to have tests depend on each > other. I'd recommend that you take the functionality you need from that other > test and separate it out into a helper function, then call that helper function > in the other test, as well as here. But I'll leave that up to you if you want > to do it. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:264: self.testDisplayOneClickInfobar On 2012/04/23 21:59:52, dennis_jeffrey wrote: > need parens? Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:280: self.testDisplayOneClickInfobar On 2012/04/23 21:59:52, dennis_jeffrey wrote: > need parens? Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:288: msg='One-click login infobar appeared for only one profile.') On 2012/04/23 21:59:52, dennis_jeffrey wrote: > are you sure this failure message will necessarily be the correct explanation? > What if 0 or 3+ infobars appear? Done. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:324: # {'https://accounts.google.com/': {'cookies': 2}}) On 2012/04/23 21:59:52, dennis_jeffrey wrote: > remove commented-out code? Done.
LGTM with 1 nit. http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14001/chrome/test/functional/inf... chrome/test/functional/infobars.py:248: self.testDisplayOneClickInfobar On 2012/04/23 22:53:09, dyu1 wrote: > Yes I thought of that originally but then that one test is essentially a one > liner from the helper function. > > Which is the worst of the two evils? > > On 2012/04/23 21:59:52, dennis_jeffrey wrote: > > need parens? > > > > Also, this is not a big deal, but I'm not a fan of invoking one test from > > another, because logically it seems weird to me to have tests depend on each > > other. I'd recommend that you take the functionality you need from that other > > test and separate it out into a helper function, then call that helper > function > > in the other test, as well as here. But I'll leave that up to you if you want > > to do it. > I don't consider my recommendation as evil at all :-D. But I'm fine with the way it is now. http://codereview.chromium.org/10152004/diff/14003/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14003/chrome/test/functional/inf... chrome/test/functional/infobars.py:307: msg='One-click login infobar appeared for only one profile.') make the same change to the message here that you did in line 286 above
I created a private function for _DisplayOneClickInfobar() instead of calling testDisplayOneClickInfobar(). http://codereview.chromium.org/10152004/diff/14003/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/14003/chrome/test/functional/inf... chrome/test/functional/infobars.py:307: msg='One-click login infobar appeared for only one profile.') On 2012/04/23 23:19:32, dennis_jeffrey wrote: > make the same change to the message here that you did in line 286 above Done.
http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:211: # Wait until page completes loading. Remove redundant comment http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:212: self.WaitUntilNavigationCompletes(tab_index=tab_index, windex=windex) Please note that since this hook got added only today, these tests will fail on the QA bots. Please disable this testcase from the FULL suite until tomorrow. Edit http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:217: If action is accept then the account is synced. Declare the expectations. One-click infobar must be showing in the first tab of first window, yada yada.. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:223: action, infobar_index=test_utils.WaitForInfobarTypeAndGetIndex( save infobar index to a var before calling PerformActionOnInfobar, and supply it here.... for clarity http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:230: tab_index=: The tab index, default is 0. remove = http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:234: self.assertTrue(self.WaitUntil( multiple lined function nesting while using lambda seems abusive. How about: def _GotInfobar(): return test_utils.GetInfobarIndexByType( self, self.OC_INFOBAR_TYPE, tab_index=tab_index, windex=windex) self.assertTrue(self.WaitUntil(_GotInfobar, msg='...') http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:237: tab_index=tab_index, windex=windex) is not None), 'is not None' is redundant http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:241: """Verify one-click infobar appears after logging into google account. s/logging/login/ http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:260: test_utils.AssertInfobarTypeDoesNotAppear(self, self.OC_INFOBAR_TYPE) Move this after the next line so that the positive cases preceed the negative ones http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:261: test_utils.WaitForInfobarTypeAndGetIndex(self, self.PW_INFOBAR_TYPE) Maybe extend this test.. and verify again after restarting the browser? http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:279: def _CheckNumProfiles(self, expected_number): 'Check' in the function is a misnomer. How about: _IsNumProfiles? http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:291: # Wait until the profile has been created. Create a local helper _OpenSecondProfile() instead of repeating these lines over and over http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:319: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( why is the waitUntil required?
http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:211: # Wait until page completes loading. On 2012/04/24 00:32:14, Nirnimesh wrote: > Remove redundant comment Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:212: self.WaitUntilNavigationCompletes(tab_index=tab_index, windex=windex) This might not be checked in until tomorrow ;) On 2012/04/24 00:32:14, Nirnimesh wrote: > Please note that since this hook got added only today, these tests will fail on > the QA bots. Please disable this testcase from the FULL suite until tomorrow. > Edit http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:217: If action is accept then the account is synced. On 2012/04/24 00:32:14, Nirnimesh wrote: > Declare the expectations. One-click infobar must be showing in the first tab of > first window, yada yada.. Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:223: action, infobar_index=test_utils.WaitForInfobarTypeAndGetIndex( On 2012/04/24 00:32:14, Nirnimesh wrote: > save infobar index to a var before calling PerformActionOnInfobar, and supply it > here.... for clarity Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:230: tab_index=: The tab index, default is 0. On 2012/04/24 00:32:14, Nirnimesh wrote: > remove = Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:234: self.assertTrue(self.WaitUntil( On 2012/04/24 00:32:14, Nirnimesh wrote: > multiple lined function nesting while using lambda seems abusive. How about: > > def _GotInfobar(): > return test_utils.GetInfobarIndexByType( > self, self.OC_INFOBAR_TYPE, tab_index=tab_index, windex=windex) > > self.assertTrue(self.WaitUntil(_GotInfobar, msg='...') I tried this before and I couldn't get it to work. I placed the function inline. self._LogIntoGoogleAccount(tab_index=tab_index, windex=windex) def _GotInfobar(): return test_utils.GetInfobarIndexByType( self, self.OC_INFOBAR_TYPE, tab_index=tab_index, windex=windex) self.assertTrue(self.WaitUntil(_GotInfobar()), msg='The one-click login infobar did not appear.') http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:237: tab_index=tab_index, windex=windex) is not None), On 2012/04/24 00:32:14, Nirnimesh wrote: > 'is not None' is redundant Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:241: """Verify one-click infobar appears after logging into google account. On 2012/04/24 00:32:14, Nirnimesh wrote: > s/logging/login/ Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:260: test_utils.AssertInfobarTypeDoesNotAppear(self, self.OC_INFOBAR_TYPE) On 2012/04/24 00:32:14, Nirnimesh wrote: > Move this after the next line so that the positive cases preceed the negative > ones Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:261: test_utils.WaitForInfobarTypeAndGetIndex(self, self.PW_INFOBAR_TYPE) On 2012/04/24 00:32:14, Nirnimesh wrote: > Maybe extend this test.. and verify again after restarting the browser? Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:279: def _CheckNumProfiles(self, expected_number): On 2012/04/24 00:32:14, Nirnimesh wrote: > 'Check' in the function is a misnomer. > How about: _IsNumProfiles? Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:291: # Wait until the profile has been created. On 2012/04/24 00:32:14, Nirnimesh wrote: > Create a local helper _OpenSecondProfile() instead of repeating these lines over > and over Done. http://codereview.chromium.org/10152004/diff/23001/chrome/test/functional/inf... chrome/test/functional/infobars.py:319: self.assertTrue(self.WaitUntil(lambda: test_utils.GetInfobarIndexByType( On 2012/04/24 00:32:14, Nirnimesh wrote: > why is the waitUntil required? Done.
LGTM |
