Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10555005:
Address bug where the one-click sign-in bar would never show again once (Closed)
|
Created:
8 years, 6 months ago by mathp Modified:
8 years, 4 months ago CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, brettw-cc_chromium.org, anantha, ajwong+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAddress bug where the one-click sign-in bar would never show again once
a user (with associated profile) had clicked "Cancel". New behavior
is that we now remember for which email the bar was dismissed, and show
it again in case the user signs in with a different email. If the user
accepts the one-click sign-in at one point, the bar never shows again
for this profile
BUG=118280
TEST=Cancel the one-click sign-in bar, log-in with another Google account,
see that it still shows up.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148760
Patch Set 1 #Patch Set 2 : fixed import error #
Total comments: 7
Patch Set 3 : Unused import, whitespace #
Total comments: 20
Patch Set 4 : Addressed comments about infobars.py #Patch Set 5 : mostly addressed rogerta's comments #
Total comments: 2
Patch Set 6 : New test #Patch Set 7 : lint #
Total comments: 6
Patch Set 8 : Moved one if #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : Removed pyauto test and left a comment #Patch Set 12 : Merge #Messages
Total messages: 30 (0 generated)
https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... File chrome/test/functional/infobars.py (right): https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:200: tab_index=0, windex=0): This is not indented far enough, the tab_index=0 should be under where self is. https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:257: for a specific account. Please follow the style guide when commenting in the module. http://www.corp.google.com/eng/doc/pyguide.xml?showone=Comments#Comments https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:281: specific account but logging in with another account. Same as above for commenting in the module.
https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... File chrome/test/functional/infobars.py (right): https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:200: tab_index=0, windex=0): On 2012/06/15 15:59:47, dyu1 wrote: > This is not indented far enough, the tab_index=0 should be under where self is. Done. https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:257: for a specific account. On 2012/06/15 15:59:47, dyu1 wrote: > Please follow the style guide when commenting in the module. > > http://www.corp.google.com/eng/doc/pyguide.xml?showone=Comments#Comments Done. https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:281: specific account but logging in with another account. On 2012/06/15 15:59:47, dyu1 wrote: > Same as above for commenting in the module. Done. https://chromiumcodereview.appspot.com/10555005/diff/2002/chrome/test/functio... chrome/test/functional/infobars.py:281: specific account but logging in with another account. On 2012/06/15 15:59:47, dyu1 wrote: > Same as above for commenting in the module. Done.
Looks good Mathieu! Some comments below. By moving more code into CanOffer() you will need to write a couple more tests. You'll need to mock FakeSigninManager::IsAllowedUsername(), see sync_setup_handler_unittest.cc for details. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: if ((realm == GURL(GaiaUrls::GetInstance()->gaia_login_form_realm()) || Please open a bug about the full email problem, and add a link to that bug here in a comment. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/password_manager/password_manager_delegate_impl.cc:147: true)) { could put the true on the previous line. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/password_manager/password_manager_delegate_impl.h (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/password_manager/password_manager_delegate_impl.h:9: #include <string> there is no need for this include in this header. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/signin/signin_manager_factory.cc (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/signin/signin_manager_factory.cc:40: new ListValue, PrefService::UNSYNCABLE_PREF); remove two spaces, line up new with prefs http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper.cc:247: } i think it would be better to move this to the if (check_connected) block, http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper.cc:333: } this if block should now be moved into CanOffer() since the email is available there. put it inside the if (check_connected) block. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper.cc:340: return; this check should also be moved inside the if (check_connected) block of CanOffer(). http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/ui/sync/one_click_signin_helper.h (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper.h:11: #include <vector> remove these new headers, not needed here. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:103: EXPECT_FALSE(OneClickSigninHelper::CanOffer(NULL, "user@gmail.com", false)); Should probably always use an empty email when check_connected is false, and a non-empty email when checked_connected is true. Applies to calls below too. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:124: true)); should probably add another EXPECT_FALSE case with a different email address. http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... File chrome/test/functional/infobars.py (right): http://r866-d34857b13568-rogerta.chromiumcodereview-hr.appspot.com/10555005/d... chrome/test/functional/infobars.py:296: test_utils.AssertInfobarTypeDoesNotAppear(self, self.OC_INFOBAR_TYPE) lines 294-296 are redundant with the previous test. no? don't you want to use test_google_account_3 at line 294 and then make sure the oc infobar is shown? since you are testing a new profile below, would need to use a different email address from test_google_account_2.
http://codereview.chromium.org/10555005/diff/11012/chrome/browser/password_ma... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: if ((realm == GURL(GaiaUrls::GetInstance()->gaia_login_form_realm()) || On 2012/06/15 17:56:00, Roger Tawa wrote: > Please open a bug about the full email problem, and add a link to that bug here > in a comment. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager_delegate_impl.cc:147: true)) { On 2012/06/15 17:56:00, Roger Tawa wrote: > could put the true on the previous line. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/password_ma... File chrome/browser/password_manager/password_manager_delegate_impl.h (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager_delegate_impl.h:9: #include <string> On 2012/06/15 17:56:00, Roger Tawa wrote: > there is no need for this include in this header. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/signin/sign... File chrome/browser/signin/signin_manager_factory.cc (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/signin/sign... chrome/browser/signin/signin_manager_factory.cc:40: new ListValue, PrefService::UNSYNCABLE_PREF); On 2012/06/15 17:56:00, Roger Tawa wrote: > remove two spaces, line up new with prefs Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper.cc:247: } On 2012/06/15 17:56:00, Roger Tawa wrote: > i think it would be better to move this to the if (check_connected) block, Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper.h (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper.h:11: #include <vector> On 2012/06/15 17:56:00, Roger Tawa wrote: > remove these new headers, not needed here. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:103: EXPECT_FALSE(OneClickSigninHelper::CanOffer(NULL, "user@gmail.com", false)); On 2012/06/15 17:56:00, Roger Tawa wrote: > Should probably always use an empty email when check_connected is false, and a > non-empty email when checked_connected is true. Applies to calls below too. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:124: true)); On 2012/06/15 17:56:00, Roger Tawa wrote: > should probably add another EXPECT_FALSE case with a different email address. Done. http://codereview.chromium.org/10555005/diff/11012/chrome/test/functional/inf... File chrome/test/functional/infobars.py (right): http://codereview.chromium.org/10555005/diff/11012/chrome/test/functional/inf... chrome/test/functional/infobars.py:296: test_utils.AssertInfobarTypeDoesNotAppear(self, self.OC_INFOBAR_TYPE) On 2012/06/15 17:56:00, Roger Tawa wrote: > lines 294-296 are redundant with the previous test. no? don't you want to use > test_google_account_3 at line 294 and then make sure the oc infobar is shown? > > since you are testing a new profile below, would need to use a different email > address from test_google_account_2. Done.
Looks good Mathieu. wrt extra testing and mocking, you'll need to create a mock of SigninManager where you can control the return value of IsAllowedUsername(). So do the following: 1/ copy the class SigninManagerMock from sync_setup_handler_unittest.cc into one_click_signin_helper_unittest.cc. 2/ In CreateMockWebContents() make sure to use the mock signin manager instead of a real one. See SyncSetupHandlerTest::SetpUp() for how to do this. 3/ Control what IsAllowedUsername() returns in a given test by using EXPECT_CALL, see TEST_P(SyncSetupHandlerTest, SubmitAuthWithInvalidUsername) for an example. Once you do that, you should be able to move those two other if blocks into CanOffer() and then write tests cases for all possibilities. http://codereview.chromium.org/10555005/diff/15001/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): http://codereview.chromium.org/10555005/diff/15001/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:129: true)); align 2nd and 3rd arguments with first.
http://codereview.chromium.org/10555005/diff/15001/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): http://codereview.chromium.org/10555005/diff/15001/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:129: true)); On 2012/06/21 13:45:51, Roger Tawa wrote: > align 2nd and 3rd arguments with first. Done.
Looks very good Mathieu. A couple of minor nits, but one question about an if block, see below. Thanks. http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper.cc:337: } how about this if block? Can it be moved to CanOffer() too? http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper.h (right): http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper.h:11: #include <vector> are these two includes needed here? It seems not. http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): http://codereview.chromium.org/10555005/diff/36001/chrome/browser/ui/sync/one... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:173: "foo@gmail.com", pass an empty string for email?
https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... chrome/browser/ui/sync/one_click_signin_helper.cc:337: } On 2012/06/27 21:17:08, Roger Tawa wrote: > how about this if block? Can it be moved to CanOffer() too? Done. https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... File chrome/browser/ui/sync/one_click_signin_helper.h (right): https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... chrome/browser/ui/sync/one_click_signin_helper.h:11: #include <vector> On 2012/06/27 21:17:08, Roger Tawa wrote: > are these two includes needed here? It seems not. Done. Lint seemed to think so. Apparently not! https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right): https://chromiumcodereview.appspot.com/10555005/diff/36001/chrome/browser/ui/... chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:173: "foo@gmail.com", On 2012/06/27 21:17:08, Roger Tawa wrote: > pass an empty string for email? Done.
lgtm Looks great Mathieu, thanks! You will now need to ask for owner reviews from: tim@ jam@ nirnimesh@ https://chromiumcodereview.appspot.com/10555005/diff/40001/chrome/browser/ui/... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://chromiumcodereview.appspot.com/10555005/diff/40001/chrome/browser/ui/... chrome/browser/ui/sync/one_click_signin_helper.cc:260: VLOG(1) << cache.GetUserNameOfProfileAtIndex(i); do we need to leave the VLOG in before commit?
https://chromiumcodereview.appspot.com/10555005/diff/40001/chrome/browser/ui/... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://chromiumcodereview.appspot.com/10555005/diff/40001/chrome/browser/ui/... chrome/browser/ui/sync/one_click_signin_helper.cc:260: VLOG(1) << cache.GetUserNameOfProfileAtIndex(i); On 2012/06/29 14:33:44, Roger Tawa wrote: > do we need to leave the VLOG in before commit? Done.
please write the test using browser_tests. pyauto is for testers. it's not the supported test harness for chrome (i.e. doesn't run on trybots, doesn't turn the buildbot red). developers are only responsible for knowing how to run/debug browser_tests, so if they break this test, they wouldn't know how to debug it (i.e. not all developers know python or how to debug it).
On 2012/06/29 16:09:58, John Abd-El-Malek wrote: > please write the test using browser_tests. > > pyauto is for testers. it's not the supported test harness for chrome (i.e. > doesn't run on trybots, doesn't turn the buildbot red). developers are only > responsible for knowing how to run/debug browser_tests, so if they break this > test, they wouldn't know how to debug it (i.e. not all developers know python or > how to debug it). looking at the python code more, I see this is restarting the browser. is there a way to test this, instead of restarting the browser, by using multi-profile so that the original profile has no windows opened? then when a window is opened with its profile, it should go through the same codepath as starting chrome (for that profile)
On 2012/06/29 16:09:58, John Abd-El-Malek wrote: > please write the test using browser_tests. > > pyauto is for testers. it's not the supported test harness for chrome (i.e. [getting off topic, but I felt strongly about this and wanted to reply]: Why should pyauto only be used by testers? I've worked with several developers on Chrome/ChromeOS in the context of automated performance testing and they've found pyauto to be very useful. I know that many developers don't know how to run pyauto tests (or even that pyauto tests exist), but when situations arise where pyauto could be helpful, people should know about it about be able to use it. It shouldn't matter whether they're testers or developers. What is the reason developers are not responsible for knowing how to run and debug pyauto tests? Pyauto is used in lots of cool ways nowadays, from functional testing to performance testing to endurance testing, on both Chrome and ChromeOS. > doesn't run on trybots, doesn't turn the buildbot red). developers are only > responsible for knowing how to run/debug browser_tests, so if they break this > test, they wouldn't know how to debug it (i.e. not all developers know python or > how to debug it).
On 2012/06/29 17:06:01, dennis_jeffrey wrote: > On 2012/06/29 16:09:58, John Abd-El-Malek wrote: > > please write the test using browser_tests. > > > > pyauto is for testers. it's not the supported test harness for chrome (i.e. > > [getting off topic, but I felt strongly about this and wanted to reply]: > > Why should pyauto only be used by testers? I've worked with several developers > on Chrome/ChromeOS in the context of automated performance testing and they've > found pyauto to be very useful. I know that many developers don't know how to > run pyauto tests (or even that pyauto tests exist), but when situations arise > where pyauto could be helpful, people should know about it about be able to use > it. It shouldn't matter whether they're testers or developers. > > What is the reason developers are not responsible for knowing how to run and > debug pyauto tests? Pyauto is used in lots of cool ways nowadays, from > functional testing to performance testing to endurance testing, on both Chrome > and ChromeOS. The one line summary is that we don't want to support multiple test harnesses. Adding multiple test harnesses has a lot of cost, some of which I've outlined above. We lived with that before and don't want to go through with that again. There's a bunch of discussion happening about this in other threads. It's a bit delayed right now because some people who we want the opinion of are away for vacation/Google IO, but I'll try to reach a conclusion soon and let everyone know. > > > doesn't run on trybots, doesn't turn the buildbot red). developers are only > > responsible for knowing how to run/debug browser_tests, so if they break this > > test, they wouldn't know how to debug it (i.e. not all developers know python > or > > how to debug it).
Hi John, I've used and modified the pyauto tests before, and for things like this they are very useful. Not running on trybots and not closing the tree apply to other tests too, so I don't hold that against pyauto tests.
On 2012/06/29 17:13:13, Roger Tawa wrote: > Hi John, > > I've used and modified the pyauto tests before, and for things like this they > are very useful. Not running on trybots and not closing the tree apply to other > tests too, so I don't hold that against pyauto tests. John, a couple of corrections to what you've said in this CL. 1. pyauto tests is not just for testers. Sure, a good bunch of them have been written by folks in test team, but that does not make it useless for developers. On the contrary, every week these tests finds bugs in chrome. every week. And it useful to discover these bugs soon enough and it then does make sense to expect developers to understand a few lines of python. It's not rocket science. 2. pyauto tests can be run on demand on trybots with -t pyauto_functional_tests 3. You continue to ignore the chromeos angle every time. It's not like browser_tests doesn't have problems, or that browser_tests fit all scenarios. It depends on how familiar a developer is with the guts of it. Not everyone is so. In the context of this CL, since it restarts the browser, the entire use case would need to be changed and assumptions made about startup as per your suggestion, just to be able to use browser_tests because it is not meant to handle restart scenarios. This does not make it any easier for debugging. 4. I'd rather have more tests in 2 frameworks, than fewer tests in 1 framework. I've seen the pitfalls with ui_tests, and lesson learnt. More often than not, many of those tests used sleep(), and other such ugly code. That's primarily what made these tests bad. pyauto doesn't have most of these problems. 5. pyauto is very well supported on win/linux/mac/chromeos. There's a whole doc for it: http://dev.chromium.org/developers/testing/pyauto - No decision has been made to make it seem otherwise.
On Fri, Jun 29, 2012 at 10:54 AM, <nirnimesh@chromium.org> wrote: > On 2012/06/29 17:13:13, Roger Tawa wrote: > >> Hi John, >> > > I've used and modified the pyauto tests before, and for things like this >> they >> are very useful. Not running on trybots and not closing the tree apply to >> > other > >> tests too, so I don't hold that against pyauto tests. >> > > John, a couple of corrections to what you've said in this CL. > 1. pyauto tests is not just for testers. Sure, a good bunch of them have > been > written by folks in test team, but that does not make it useless for > developers. > On the contrary, every week these tests finds bugs in chrome. every week. As automated tests that testers used to do manually, I would expect that they would catch regressions. > And it > useful to discover these bugs soon enough and it then does make sense to > expect > developers to understand a few lines of python. It's not rocket science. > The fundamental issue here is whether we want to support a new test harness or not. It goes far beyond a developer understanding a few lines of python. As I mentioned in the other thread, let's discuss this in person. Darin looks ooo today, so let's wait until we're all back and then we can discuss this. > 2. pyauto tests can be run on demand on trybots with -t > pyauto_functional_tests > What matters is whether things turn the tree/try bots red or not. This follows from point 1 about whether we want to support a new test harness for developers or not. The current status quo is that we don't, which is why it doesn't turn the tree red and developers aren't responsible for keeping those tests green. > 3. You continue to ignore the chromeos angle every time. I don't think I'm ignoring this. ChromeOS team can write tests using whatever harness/language they want. The issue is when the test harness needs to be part of Chrome, because then point 1 affects things. > It's not like > browser_tests doesn't have problems, or that browser_tests fit all > scenarios. It > depends on how familiar a developer is with the guts of it. Not everyone > is so. > browser_tests is currently the only supported test harness for Chrome developers. That's where we are today. Adding more test harnesses to be supported needs consensus first. > In the context of this CL, since it restarts the browser, the entire use > case > would need to be changed and assumptions made about startup as per your > suggestion, just to be able to use browser_tests because it is not meant to > handle restart scenarios. This does not make it any easier for debugging. > As I've mentioned, it seems there are ways of testing this scenario without a browser restart, through multi-profiles. > 4. I'd rather have more tests in 2 frameworks, than fewer tests in 1 > framework. > No one is arguing for less tests here. The issue is supporting more test harnesses. > I've seen the pitfalls with ui_tests, and lesson learnt. More often than > not, > many of those tests used sleep(), and other such ugly code. That's > primarily > what made these tests bad. pyauto doesn't have most of these problems. > I'm not talking about the bad patterns in ui_tests. The problems we ran into was all the plumbing that has be to added to plumb state between two processes. Also, having to debug two processes at once instead of one with browser_tests. > > 5. pyauto is very well supported on win/linux/mac/chromeos. There's a > whole doc > for it: http://dev.chromium.org/**developers/testing/pyauto<http://dev.chromium.org/d... No decision has been > made to make it seem otherwise. > I understand it runs on all OSs. But right now, it's not part of the developer workflow for engineers working on Chrome (i.e. don't have to fix tests when it turns red, it doesn't turn the tree red.) > > http://codereview.chromium.**org/10555005/<http://codereview.chromium.org/105... >
On 2012/06/29 18:33:34, John Abd-El-Malek wrote: > On Fri, Jun 29, 2012 at 10:54 AM, <mailto:nirnimesh@chromium.org> wrote: > > > On 2012/06/29 17:13:13, Roger Tawa wrote: > > > >> Hi John, > >> > > > > I've used and modified the pyauto tests before, and for things like this > >> they > >> are very useful. Not running on trybots and not closing the tree apply to > >> > > other > > > >> tests too, so I don't hold that against pyauto tests. > >> > > > > John, a couple of corrections to what you've said in this CL. > > 1. pyauto tests is not just for testers. Sure, a good bunch of them have > > been > > written by folks in test team, but that does not make it useless for > > developers. > > On the contrary, every week these tests finds bugs in chrome. every week. > > > As automated tests that testers used to do manually, I would expect that > they would catch regressions. > > > > And it > > useful to discover these bugs soon enough and it then does make sense to > > expect > > developers to understand a few lines of python. It's not rocket science. > > > > The fundamental issue here is whether we want to support a new test harness > or not. It goes far beyond a developer understanding a few lines of python. > > As I mentioned in the other thread, let's discuss this in person. Darin > looks ooo today, so let's wait until we're all back and then we can discuss > this. > > > > 2. pyauto tests can be run on demand on trybots with -t > > pyauto_functional_tests > > > > What matters is whether things turn the tree/try bots red or not. This > follows from point 1 about whether we want to support a new test harness > for developers or not. The current status quo is that we don't, which is > why it doesn't turn the tree red and developers aren't responsible for > keeping those tests green. > > > > 3. You continue to ignore the chromeos angle every time. > > > I don't think I'm ignoring this. ChromeOS team can write tests using > whatever harness/language they want. The issue is when the test harness > needs to be part of Chrome, because then point 1 affects things. > > > > It's not like > > browser_tests doesn't have problems, or that browser_tests fit all > > scenarios. It > > depends on how familiar a developer is with the guts of it. Not everyone > > is so. > > > > browser_tests is currently the only supported test harness for Chrome > developers. That's where we are today. Adding more test harnesses to be > supported needs consensus first. I'm not sure why you'd think that pyauto is a new test harness. It's been there for over 2 years.
On 2012/06/29 18:33:34, John Abd-El-Malek wrote: > On Fri, Jun 29, 2012 at 10:54 AM, <mailto:nirnimesh@chromium.org> wrote: > > > On 2012/06/29 17:13:13, Roger Tawa wrote: > > > >> Hi John, > >> > > > > I've used and modified the pyauto tests before, and for things like this > >> they > >> are very useful. Not running on trybots and not closing the tree apply to > >> > > other > > > >> tests too, so I don't hold that against pyauto tests. > >> > > > > John, a couple of corrections to what you've said in this CL. > > 1. pyauto tests is not just for testers. Sure, a good bunch of them have > > been > > written by folks in test team, but that does not make it useless for > > developers. > > On the contrary, every week these tests finds bugs in chrome. every week. > > > As automated tests that testers used to do manually, I would expect that > they would catch regressions. There are some tests that can't be performed manually such as pointer lock / fullscreen with pointer lock. PyAuto along with JS came very handy for testing these use cases. There are so many features now and so many more test cases that it only makes sense to automate the time intensive manual tests. There are bugs found due to changes to the code that could normally be missed if tests are run manually. > > > > And it > > useful to discover these bugs soon enough and it then does make sense to > > expect > > developers to understand a few lines of python. It's not rocket science. > > > > The fundamental issue here is whether we want to support a new test harness > or not. It goes far beyond a developer understanding a few lines of python. > > As I mentioned in the other thread, let's discuss this in person. Darin > looks ooo today, so let's wait until we're all back and then we can discuss > this. > > > > 2. pyauto tests can be run on demand on trybots with -t > > pyauto_functional_tests > > > > What matters is whether things turn the tree/try bots red or not. This > follows from point 1 about whether we want to support a new test harness > for developers or not. The current status quo is that we don't, which is > why it doesn't turn the tree red and developers aren't responsible for > keeping those tests green. Why not let the individual developer choose if they want to support PyAuto or not? If they choose to write browser_tests vs PyAuto test? It's not difficult to debug or look into test failures once they write a couple. Typically the sheriff who monitor's the PyAuto tree are the ones that does the initial debugging. > > > > 3. You continue to ignore the chromeos angle every time. > > > I don't think I'm ignoring this. ChromeOS team can write tests using > whatever harness/language they want. The issue is when the test harness > needs to be part of Chrome, because then point 1 affects things. > > > > It's not like > > browser_tests doesn't have problems, or that browser_tests fit all > > scenarios. It > > depends on how familiar a developer is with the guts of it. Not everyone > > is so. > > > > browser_tests is currently the only supported test harness for Chrome > developers. That's where we are today. Adding more test harnesses to be > supported needs consensus first. > > > > In the context of this CL, since it restarts the browser, the entire use > > case > > would need to be changed and assumptions made about startup as per your > > suggestion, just to be able to use browser_tests because it is not meant to > > handle restart scenarios. This does not make it any easier for debugging. > > > > As I've mentioned, it seems there are ways of testing this scenario without > a browser restart, through multi-profiles. > > > > 4. I'd rather have more tests in 2 frameworks, than fewer tests in 1 > > framework. > > > > No one is arguing for less tests here. The issue is supporting more test > harnesses. > > > > I've seen the pitfalls with ui_tests, and lesson learnt. More often than > > not, > > many of those tests used sleep(), and other such ugly code. That's > > primarily > > what made these tests bad. pyauto doesn't have most of these problems. > > > > I'm not talking about the bad patterns in ui_tests. The problems we ran > into was all the plumbing that has be to added to plumb state between two > processes. Also, having to debug two processes at once instead of one with > browser_tests. > > > > > > 5. pyauto is very well supported on win/linux/mac/chromeos. There's a > > whole doc > > for it: > http://dev.chromium.org/**developers/testing/pyauto%3Chttp://dev.chromium.org... > No decision has been > > made to make it seem otherwise. > > > > I understand it runs on all OSs. But right now, it's not part of the > developer workflow for engineers working on Chrome (i.e. don't have to fix > tests when it turns red, it doesn't turn the tree red.) > > > > > > > http://codereview.chromium.**org/10555005/%3Chttp://codereview.chromium.org/1...> > >
Please indicate if pyauto tests are sufficient in this case. Thanks. On 2012/06/29 18:44:39, dyu1 wrote: > On 2012/06/29 18:33:34, John Abd-El-Malek wrote: > > On Fri, Jun 29, 2012 at 10:54 AM, <mailto:nirnimesh@chromium.org> wrote: > > > > > On 2012/06/29 17:13:13, Roger Tawa wrote: > > > > > >> Hi John, > > >> > > > > > > I've used and modified the pyauto tests before, and for things like this > > >> they > > >> are very useful. Not running on trybots and not closing the tree apply to > > >> > > > other > > > > > >> tests too, so I don't hold that against pyauto tests. > > >> > > > > > > John, a couple of corrections to what you've said in this CL. > > > 1. pyauto tests is not just for testers. Sure, a good bunch of them have > > > been > > > written by folks in test team, but that does not make it useless for > > > developers. > > > On the contrary, every week these tests finds bugs in chrome. every week. > > > > > > As automated tests that testers used to do manually, I would expect that > > they would catch regressions. > > There are some tests that can't be performed manually such as pointer lock / > fullscreen with pointer lock. PyAuto along with JS came very handy for testing > these use cases. > > There are so many features now and so many more test cases that it only makes > sense to automate the time intensive manual tests. There are bugs found due to > changes to the code that could normally be missed if tests are run manually. > > > > > > > > > > And it > > > useful to discover these bugs soon enough and it then does make sense to > > > expect > > > developers to understand a few lines of python. It's not rocket science. > > > > > > > The fundamental issue here is whether we want to support a new test harness > > or not. It goes far beyond a developer understanding a few lines of python. > > > > As I mentioned in the other thread, let's discuss this in person. Darin > > looks ooo today, so let's wait until we're all back and then we can discuss > > this. > > > > > > > 2. pyauto tests can be run on demand on trybots with -t > > > pyauto_functional_tests > > > > > > > What matters is whether things turn the tree/try bots red or not. This > > follows from point 1 about whether we want to support a new test harness > > for developers or not. The current status quo is that we don't, which is > > why it doesn't turn the tree red and developers aren't responsible for > > keeping those tests green. > > Why not let the individual developer choose if they want to support PyAuto or > not? If they choose to write browser_tests vs PyAuto test? It's not difficult to > debug or look into test failures once they write a couple. Typically the sheriff > who monitor's the PyAuto tree are the ones that does the initial debugging. > > > > > > > > > 3. You continue to ignore the chromeos angle every time. > > > > > > I don't think I'm ignoring this. ChromeOS team can write tests using > > whatever harness/language they want. The issue is when the test harness > > needs to be part of Chrome, because then point 1 affects things. > > > > > > > It's not like > > > browser_tests doesn't have problems, or that browser_tests fit all > > > scenarios. It > > > depends on how familiar a developer is with the guts of it. Not everyone > > > is so. > > > > > > > browser_tests is currently the only supported test harness for Chrome > > developers. That's where we are today. Adding more test harnesses to be > > supported needs consensus first. > > > > > > > In the context of this CL, since it restarts the browser, the entire use > > > case > > > would need to be changed and assumptions made about startup as per your > > > suggestion, just to be able to use browser_tests because it is not meant to > > > handle restart scenarios. This does not make it any easier for debugging. > > > > > > > As I've mentioned, it seems there are ways of testing this scenario without > > a browser restart, through multi-profiles. > > > > > > > 4. I'd rather have more tests in 2 frameworks, than fewer tests in 1 > > > framework. > > > > > > > No one is arguing for less tests here. The issue is supporting more test > > harnesses. > > > > > > > I've seen the pitfalls with ui_tests, and lesson learnt. More often than > > > not, > > > many of those tests used sleep(), and other such ugly code. That's > > > primarily > > > what made these tests bad. pyauto doesn't have most of these problems. > > > > > > > I'm not talking about the bad patterns in ui_tests. The problems we ran > > into was all the plumbing that has be to added to plumb state between two > > processes. Also, having to debug two processes at once instead of one with > > browser_tests. > > > > > > > > > > 5. pyauto is very well supported on win/linux/mac/chromeos. There's a > > > whole doc > > > for it: > > > http://dev.chromium.org/**developers/testing/pyauto%253Chttp://dev.chromium.o... > > No decision has been > > > made to make it seem otherwise. > > > > > > > I understand it runs on all OSs. But right now, it's not part of the > > developer workflow for engineers working on Chrome (i.e. don't have to fix > > tests when it turns red, it doesn't turn the tree red.) > > > > > > > > > > > > > http://codereview.chromium.**org/10555005/%253Chttp://codereview.chromium.org...> > > >
On 2012/07/11 15:15:26, mathp wrote: > Please indicate if pyauto tests are sufficient in this case. Thanks. In my opinion, it's better and simpler to use pyauto for restart scenarios. browser_tests is not meant to handle these. Sure you could make assumptions about the startup and use multi-profiles but the test does not remain as clean then. (And besides, there's no multi-profiles on chromeos)
On 2012/07/11 23:04:14, Nirnimesh wrote: > On 2012/07/11 15:15:26, mathp wrote: > > Please indicate if pyauto tests are sufficient in this case. Thanks. > > In my opinion, it's better and simpler to use pyauto for restart scenarios. > browser_tests is not meant to handle these. Sure you could make assumptions > about the startup and use multi-profiles but the test does not remain as clean > then. (And besides, there's no multi-profiles on chromeos) Ok. John Adb-El-Malek, would you approve this as an owner?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@google.com/10555005/59003
Failed to apply patch for chrome/browser/ui/sync/one_click_signin_helper_unittest.cc: While running patch -p0 --forward --force; patching file chrome/browser/ui/sync/one_click_signin_helper_unittest.cc Hunk #1 FAILED at 2. Hunk #2 succeeded at 34 (offset 2 lines). Hunk #3 succeeded at 94 with fuzz 2 (offset 38 lines). Hunk #4 succeeded at 105 with fuzz 1 (offset 38 lines). Hunk #5 succeeded at 122 (offset 37 lines). Hunk #6 FAILED at 102. 2 out of 6 hunks FAILED -- saving rejects to file chrome/browser/ui/sync/one_click_signin_helper_unittest.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@google.com/10555005/57003
Try job failure for 10555005-57003 on linux_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@google.com/10555005/57003
Change committed as 148760 |
