Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
621153002:
A Sign in to GAIA test utility. (Closed)
|
Created:
6 years, 2 months ago by shadi Modified:
6 years, 1 month ago CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, haitaol+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionA Sign in to GAIA test utility.
The purpose of this CL is to provide a test utility that signs in a GAIA user using recommended sign in process.
Signing in opens chrome://chrome-signin URL and populates the user name and password via JS injections and blocks until a success or failed signing notification occurs.
BUG=431366
Committed: https://crrev.com/1a4f9fb332adbb177a02b50676f7db9637389406
Cr-Commit-Position: refs/heads/master@{#305085}
Patch Set 1 : Sign in with chrome://chrome-signin #
Total comments: 20
Patch Set 2 : fixing comments #
Total comments: 10
Patch Set 3 : Abstract common JS code for signin in tests #
Total comments: 6
Patch Set 4 : nits #
Messages
Total messages: 19 (7 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
shadi@chromium.org changed reviewers: + pvalenzuela@chromium.org, rogerta@chromium.org, zea@chromium.org
shadi@chromium.org changed required reviewers: + pvalenzuela@chromium.org, zea@chromium.org
PTAL
https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.cc (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:20: class SignInWaiter : public SigninManagerBase::Observer { I believe this class (and all other util method for this file) should be placed in an anonymous namespace. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.h (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:9: For consistency, let's do the following: 1) Rename this file signin_helper. 2) Put this code in a signin_helper namespace. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:10: // A static function which signs in a user using Chrome sign-in process. this isn't marked static https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:12: bool SignInToGAIA(Browser* browser, what do you think about renaming this to SignIn or SignInWithUI?
Some quick comments. That said, it's a bit odd that we're landing this without any associated test. How do we know it works? Also, I worry a bit about how brittle this will be in the face of otherwise innocuous changes to the js code of the signin page. Not sure if there's a better way though. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.cc (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:5: #include "base/scoped_observer.h" You should probably include <string> here, since you didn't do it in the .h file https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:20: class SignInWaiter : public SigninManagerBase::Observer { nit: put all this helper logic anonymous namespace? https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:31: bool DidSignIn() { Comments for these methods? https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:68: bool seen_; comments for these members? https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:75: // static This isn't actually static (here and below) https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.h (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:13: const std::string& username, I'm surprised you don't have to declare string?
Thanks for quick review. It is hard to test this sign in without testing against real GAIA server. The sign in team mocks the gaia auth page, I am not sure if doing the same for testing here is any useful. However, I plan on extracting the JS code in this CL since it is duplicated from the browser/ui/webui/signin/ browser tests. I'll upload a new patch and add someone from their team for review. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.cc (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:5: #include "base/scoped_observer.h" On 2014/11/07 23:55:48, Nicolas Zea wrote: > You should probably include <string> here, since you didn't do it in the .h file I added it in .h file. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:20: class SignInWaiter : public SigninManagerBase::Observer { On 2014/11/07 23:55:48, Nicolas Zea wrote: > nit: put all this helper logic anonymous namespace? Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:20: class SignInWaiter : public SigninManagerBase::Observer { On 2014/11/07 23:54:14, pvalenzuela wrote: > I believe this class (and all other util method for this file) should be placed > in an anonymous namespace. Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:31: bool DidSignIn() { On 2014/11/07 23:55:48, Nicolas Zea wrote: > Comments for these methods? Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:68: bool seen_; On 2014/11/07 23:55:48, Nicolas Zea wrote: > comments for these members? Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.cc:75: // static On 2014/11/07 23:55:48, Nicolas Zea wrote: > This isn't actually static (here and below) Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_test_utils.h (right): https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:9: On 2014/11/07 23:54:14, pvalenzuela wrote: > For consistency, let's do the following: > > 1) Rename this file signin_helper. > 2) Put this code in a signin_helper namespace. Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:10: // A static function which signs in a user using Chrome sign-in process. On 2014/11/07 23:54:14, pvalenzuela wrote: > this isn't marked static Yeah it should not be static. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:12: bool SignInToGAIA(Browser* browser, On 2014/11/07 23:54:14, pvalenzuela wrote: > what do you think about renaming this to SignIn or SignInWithUI? Done. https://codereview.chromium.org/621153002/diff/60001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_test_utils.h:13: const std::string& username, On 2014/11/07 23:55:48, Nicolas Zea wrote: > I'm surprised you don't have to declare string? Yeah, the compiler did not complain probably because the cc file has it included in one of its .h files. I added #include <string> for correctness anyway :).
Looking good Shadi. A few comments below. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_helper.cc (right): https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:25: class SignInWaiter : public SigninManagerBase::Observer { Please use the SigninTracker class to wait for signin. It's the best way to do it, and will be support if the definition of being signed in changes. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:136: const std::string& password) { Should this function be called SignInWithUI? https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:137: SigninManager* signinManager = In c++, variables don't use camelcase. Use signin_manager. Applies to other variables below too. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:142: VLOG(1) << "Navigating to " << signinURL; I think all VLOGs should be DVLOGs now. https://codereview.chromium.org/621153002/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/621153002/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1212: 'browser/sync/test/integration/signin_helper.h', Should this code live under browser/sync or browser/signin?
PTAL. I put the JS code to sign in into GAIA iframe in common utils file. It is used by existing signin browser test. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/signin_helper.cc (right): https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:25: class SignInWaiter : public SigninManagerBase::Observer { On 2014/11/10 16:33:49, Roger Tawa wrote: > Please use the SigninTracker class to wait for signin. It's the best way to do > it, and will be support if the definition of being signed in changes. Done. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:136: const std::string& password) { On 2014/11/10 16:33:49, Roger Tawa wrote: > Should this function be called SignInWithUI? Done. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:137: SigninManager* signinManager = On 2014/11/10 16:33:49, Roger Tawa wrote: > In c++, variables don't use camelcase. Use signin_manager. Applies to other > variables below too. Done. https://codereview.chromium.org/621153002/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/signin_helper.cc:142: VLOG(1) << "Navigating to " << signinURL; On 2014/11/10 16:33:49, Roger Tawa wrote: > I think all VLOGs should be DVLOGs now. Done. https://codereview.chromium.org/621153002/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/621153002/diff/80001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1212: 'browser/sync/test/integration/signin_helper.h', On 2014/11/10 16:33:49, Roger Tawa wrote: > Should this code live under browser/sync or browser/signin? I moved the code to browser/ui/webui/signin where the other browser tests using it are.
lgtm with some comments below. https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/login_ui_test_utils.cc (right): https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:110: const std::string& password) { typo: Singin --> Signin https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:114: "document.getElementById('signIn').click();"; Indent 3 lines above 3 more spaces. https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:131: GURL signin_URL = signin::GetPromoURL(signin::SOURCE_START_PAGE, false); use lowercase signin_url
I like the extra effort to extract the signin logic to a common location. LGTM once you fix Roger's remaining issues
Thanks for reviews. https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/login_ui_test_utils.cc (right): https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:110: const std::string& password) { On 2014/11/13 15:12:59, Roger Tawa wrote: > typo: Singin --> Signin Done. https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:114: "document.getElementById('signIn').click();"; On 2014/11/13 15:12:59, Roger Tawa wrote: > Indent 3 lines above 3 more spaces. Done. https://codereview.chromium.org/621153002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/login_ui_test_utils.cc:131: GURL signin_URL = signin::GetPromoURL(signin::SOURCE_START_PAGE, false); On 2014/11/13 15:12:59, Roger Tawa wrote: > use lowercase signin_url Done.
The CQ bit was checked by shadi@chromium.org
shadi@chromium.org changed required reviewers: - pvalenzuela@chromium.org, zea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/621153002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1a4f9fb332adbb177a02b50676f7db9637389406 Cr-Commit-Position: refs/heads/master@{#305085} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
