Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2864433003:
[ARC] Add browser test of ArcAuthService (Closed)
|
Created:
3 years, 7 months ago by blundell Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ARC] Add browser test of ArcAuthService
In preparation for changing ArcAuthService to consume signin information
via the Identity Service, this CL adds a browsertest of ArcAuthService.
The initial test tests the flow for a user (non-managed) account when
background auth is performed. This is the flow that is the most
significant consumer of the Identity Service in the ARC auth code.
Specifically, we test that if Chrome is seeded with information for a given
account via the signin code, then when AuthInstance connects to ArcAuthService
and requests account info, it receives account info corresponding to the
seeded information.
Currently, we skip the merge session part of the auth flow in this test.
The merge session is difficult to interact with in a testing context, and
even with this flow skipped, the interaction between the ARC auth code
and //components/signin is basically fully exercised by the test.
In the future, we will aim to add tests of the following cases
(some of which might be easy, and others quite difficult):
- Error cases for background auth
- Merge session
- Manual authentication
- Robot accounts
- Managed accounts
BUG=683120
Review-Url: https://codereview.chromium.org/2864433003
Cr-Commit-Position: refs/heads/master@{#473535}
Committed: https://chromium.googlesource.com/chromium/src/+/cb01102e078c3dbd41aad1ba652f43f6465e9e84
Patch Set 1 #Patch Set 2 : Browsertest #Patch Set 3 : Self-review #Patch Set 4 : Rebase #
Total comments: 15
Patch Set 5 : Response to review #
Total comments: 8
Patch Set 6 : Rebase #Patch Set 7 : Response to reviews #Messages
Total messages: 46 (33 generated)
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Comment out auth-related function impls to see if any tests fail ========== to ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context. Even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases: - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ==========
Description was changed from ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context. Even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases: - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ========== to ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context. Even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases: - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ==========
Description was changed from ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context. Even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases: - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ========== to ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ==========
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
blundell@chromium.org changed reviewers: + lhchavez@chromium.org
More tests are always welcome :) Thanks so much for doing this! https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_context.h (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_context.h:79: bool skip_merge_session_for_testing_; nit: we tend to prefer to initialize POD types in the header file. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:64: done_closure_.Run(); nit: base::ResetAndReturn(&done_closure_).Run(); https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:82: ArcAuthServiceTest() {} nit: = default. Same in L85 https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:93: user_manager_enabler_.reset(new chromeos::ScopedUserManagerEnabler( nit: prefer = base::MakeUnique<chromeos::ScopedUserManagerEnabler(...); Also, either use parens for the FakeChromeUserManager constructor, or add a comment as to why you're not adding them (see https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... ) https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:102: EXPECT_TRUE(temp_dir_.CreateUniqueTempDir()); maybe ASSERT_TRUE? The test cannot continue if this fails IIUC. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:193: FakeProfileOAuth2TokenService* token_service_; Maybe remove this member variable and turn it into a local in L117 https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.h (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.h:39: static std::string GetTokenExchangeEndpoint(); nit: add "ForTesting" suffix. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:565: local_state_.reset(new TestingPrefServiceSimple()); nit: = base::MakeUnique<TestingPrefServiceSimple>();
Thanks! https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_context.h (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_context.h:79: bool skip_merge_session_for_testing_; On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > nit: we tend to prefer to initialize POD types in the header file. Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:64: done_closure_.Run(); On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > nit: base::ResetAndReturn(&done_closure_).Run(); Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:82: ArcAuthServiceTest() {} On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > nit: = default. Same in L85 Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:102: EXPECT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > maybe ASSERT_TRUE? The test cannot continue if this fails IIUC. Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:193: FakeProfileOAuth2TokenService* token_service_; On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > Maybe remove this member variable and turn it into a local in L117 Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.h (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.h:39: static std::string GetTokenExchangeEndpoint(); On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > nit: add "ForTesting" suffix. Done. https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right): https://codereview.chromium.org/2864433003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:565: local_state_.reset(new TestingPrefServiceSimple()); On 2017/05/11 16:57:54, Luis Héctor Chávez wrote: > nit: = base::MakeUnique<TestingPrefServiceSimple>(); Done.
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm! Also, for consistency with the rest of the codebase, can you amend the commit message to s/ARC++/ARC/? https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:199: // Tests that when ARC++ requests account info for a non-managed account and nit: s/ARC++/ARC/
khmel@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:57: class FakeAuthInstance : public mojom::AuthInstance { Up to you and Luis but looks like it is better to place this components/arc/test https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:180: PrefService* const prefs = profile()->GetPrefs(); nit: Please use arc:SetArcPlayStoreEnabledForProfile EnableArcPlayStore https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc (right): https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc:59: return kEndPoint; nit: probably better to expose this via header
Description was changed from ========== [ARC++] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC++ auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC++ auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ========== to ========== [ARC] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ==========
Description was changed from ========== [ARC] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ========== to ========== [ARC] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ==========
Thanks for the reviews! https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:57: class FakeAuthInstance : public mojom::AuthInstance { On 2017/05/12 15:52:28, khmel wrote: > Up to you and Luis but looks like it is better to place this components/arc/test Thanks, I'll move it in a followup. https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:180: PrefService* const prefs = profile()->GetPrefs(); On 2017/05/12 15:52:28, khmel wrote: > nit: > Please use arc:SetArcPlayStoreEnabledForProfile > EnableArcPlayStore Turns out this function isn't even needed in this test, eliminated it. https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc:199: // Tests that when ARC++ requests account info for a non-managed account and On 2017/05/12 15:45:56, Luis Héctor Chávez wrote: > nit: s/ARC++/ARC/ Done. https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc (right): https://codereview.chromium.org/2864433003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc:59: return kEndPoint; On 2017/05/12 15:52:28, khmel wrote: > nit: probably better to expose this via header Done.
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
blundell@chromium.org changed reviewers: + alemate@chromium.org
alemate@: Can you do an OWNERS review of //chrome/browser/chromeos/login? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alemate@: friendly ping :)
blundell@chromium.org changed reviewers: + achuith@chromium.org
achuith@: Can you do an OWNERS review of //chrome/browser/chromeos/login? Thanks!
On 2017/05/17 09:42:46, blundell (OOO until May 22) wrote: > achuith@: Can you do an OWNERS review of //chrome/browser/chromeos/login? > Thanks! only looked at chromeos/login. lgtm.
lgtm
The CQ bit was checked by blundell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2864433003/#ps120001 (title: "Response to reviews")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495442225978480,
"parent_rev": "479179ca87f415cc18ae18682a3938a4486ea281", "commit_rev":
"cb01102e078c3dbd41aad1ba652f43f6465e9e84"}
Message was sent while issue was closed.
Description was changed from ========== [ARC] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 ========== to ========== [ARC] Add browser test of ArcAuthService In preparation for changing ArcAuthService to consume signin information via the Identity Service, this CL adds a browsertest of ArcAuthService. The initial test tests the flow for a user (non-managed) account when background auth is performed. This is the flow that is the most significant consumer of the Identity Service in the ARC auth code. Specifically, we test that if Chrome is seeded with information for a given account via the signin code, then when AuthInstance connects to ArcAuthService and requests account info, it receives account info corresponding to the seeded information. Currently, we skip the merge session part of the auth flow in this test. The merge session is difficult to interact with in a testing context, and even with this flow skipped, the interaction between the ARC auth code and //components/signin is basically fully exercised by the test. In the future, we will aim to add tests of the following cases (some of which might be easy, and others quite difficult): - Error cases for background auth - Merge session - Manual authentication - Robot accounts - Managed accounts BUG=683120 Review-Url: https://codereview.chromium.org/2864433003 Cr-Commit-Position: refs/heads/master@{#473535} Committed: https://chromium.googlesource.com/chromium/src/+/cb01102e078c3dbd41aad1ba652f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/cb01102e078c3dbd41aad1ba652f... |
