Mocker for OSCrypt

OSCryptMocker

OSCrypt uses OS-level password storage services (e.g.
Keychain). Tests should avoid involving such services.
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction
replaces previous OS-specific mocking instructions in
tests. Additionally, I added it to some
tests which previously omitted mocking entirely.

BUG=602624
Committed: https://crrev.com/d281ff9e63f782362ef6ea0ccdde6c1238f34431
Cr-Commit-Position: refs/heads/master@{#396472}

[cfroussios, 2016-05-24 10:05:19]

Description was changed from

==========
Mocker for OSCrypt



BUG=602624
==========

to

==========
Mocker for OSCrypt

OSCrypt for Mac and Windows uses OS-level password storage services (e.g.
Keychain). Soon, such services will also be used in Linux
(https://codereview.chromium.org/1973483002/).

Tests that depend on OSCrypt should mock it, so that they don't use such
services.

This CL introduces the build target //components/os_crypt:test_support, which
contains an abstraction for mocking: OSCryptMocker.

This abstraction replaces OS-specific mocking instructions. Additionally, I
added it to some tests which previously omitted mocking entirely.


BUG=602624
==========

[cfroussios, 2016-05-24 11:31:03]

Description was changed from

==========
Mocker for OSCrypt

OSCrypt for Mac and Windows uses OS-level password storage services (e.g.
Keychain). Soon, such services will also be used in Linux
(https://codereview.chromium.org/1973483002/).

Tests that depend on OSCrypt should mock it, so that they don't use such
services.

This CL introduces the build target //components/os_crypt:test_support, which
contains an abstraction for mocking: OSCryptMocker.

This abstraction replaces OS-specific mocking instructions. Additionally, I
added it to some tests which previously omitted mocking entirely.


BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
 abstraction for mocking: OSCryptMocker.

This abstraction replaces previous OS-specific mocking 
instructions in tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

[cfroussios, 2016-05-24 12:42:32]

Description was changed from

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
 abstraction for mocking: OSCryptMocker.

This abstraction replaces previous OS-specific mocking 
instructions in tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
 abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

[cfroussios, 2016-05-24 12:51:13]

cfroussios@chromium.org changed reviewers:
+ vabr@chromium.org

[cfroussios, 2016-05-24 12:51:14]

Hi vabr. Would you be kind enough to be the first to review the introduction of
OSCrypt's mocker? Thanks

[vabr (Chromium), 2016-05-24 13:33:44]

Thank you, this LGTM!

Please have a look at the comments below. In some places I flag missing calls to
OSCrypt::TearDown(), but if you think it's better without them, that's fine by
me as well.

Cheers,
Vaclav

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac_unittest.cc
(right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac_unittest.cc:41:
OSCryptMocker::SetUpWithSingleton();
Should there also be the matching OSCryptMocker::TearDown() call?

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/signin/l...
chrome/browser/signin/local_auth_unittest.cc:26: void SetUp() override {
OSCryptMocker::SetUpWithSingleton(); }
Could you do this in the constructor and destructor of LocalAuthTest instead of
the SetUp and TearDown? The former is preferred, see:
 *
https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#...,
and
 *
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8...

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:265:
OSCryptMocker::SetUpWithSingleton();
Missing OSCryptMocker::TearDown?

https://codereview.chromium.org/2010463002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:313:
OSCryptMocker::SetUpWithSingleton();
Missing OSCryptMocker::TearDown?

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/BUI...
File components/os_crypt/BUILD.gn (right):

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/BUI...
components/os_crypt/BUILD.gn:58: sources = [
Please also use:
testonly = true

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/os_...
File components/os_crypt/os_crypt_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/os_...
components/os_crypt/os_crypt_unittest.cc:23: void SetUp() override {
OSCryptMocker::SetUpWithSingleton(); }
Also here, could this be in constructor and destructor instead of SetUp and
TearDown?

[msramek, 2016-05-24 13:40:57]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

[msramek, 2016-05-24 13:40:58]

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1014: void
SetUp() override { OSCryptMocker::SetUpWithSingleton(); }
Drive by! Wouldn't it suffice to call this in RemovePasswordsTester below? If
this is only needed for one or two tests, I would prefer not adding it to the
BrowsingDataRemoverTest class.

[cfroussios, 2016-05-25 09:49:36]

Patchset #7 (id:120001) has been deleted

[cfroussios, 2016-05-25 13:13:08]

cfroussios@chromium.org changed reviewers:
+ vabr@google.com
- vabr@chromium.org

[cfroussios, 2016-05-25 13:13:09]

Thanks for the feedback!

@vabr I modified more files, some of which are captured by your approval.

@msramek I modified the test according to your preference.

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1014: void
SetUp() override { OSCryptMocker::SetUpWithSingleton(); }
On 2016/05/24 13:40:57, msramek wrote:
> Drive by! Wouldn't it suffice to call this in RemovePasswordsTester below? If
> this is only needed for one or two tests, I would prefer not adding it to the
> BrowsingDataRemoverTest class.

Done (assuming below meant above)

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac_unittest.cc
(right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac_unittest.cc:41:
OSCryptMocker::SetUpWithSingleton();
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Should there also be the matching OSCryptMocker::TearDown() call?

Done.

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/signin/l...
chrome/browser/signin/local_auth_unittest.cc:26: void SetUp() override {
OSCryptMocker::SetUpWithSingleton(); }
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Could you do this in the constructor and destructor of LocalAuthTest instead
of
> the SetUp and TearDown? The former is preferred, see:
>  *
>
https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#...,
> and
>  *
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8...

Done.

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:265:
OSCryptMocker::SetUpWithSingleton();
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Missing OSCryptMocker::TearDown?

Done.

https://codereview.chromium.org/2010463002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_test_utils.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_test_utils.cc:313:
OSCryptMocker::SetUpWithSingleton();
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Missing OSCryptMocker::TearDown?

Done.

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/BUI...
File components/os_crypt/BUILD.gn (right):

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/BUI...
components/os_crypt/BUILD.gn:58: sources = [
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Please also use:
> testonly = true

Done.

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/os_...
File components/os_crypt/os_crypt_unittest.cc (right):

https://codereview.chromium.org/2010463002/diff/80001/components/os_crypt/os_...
components/os_crypt/os_crypt_unittest.cc:23: void SetUp() override {
OSCryptMocker::SetUpWithSingleton(); }
On 2016/05/24 13:33:44, vabr (Chromium) wrote:
> Also here, could this be in constructor and destructor instead of SetUp and
> TearDown?

Done.

[vabr (Chromium), 2016-05-25 13:21:02]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org
- vabr@google.com

[msramek, 2016-05-25 13:21:08]

msramek@chromium.org changed reviewers:
+ vabr@google.com
- vabr@chromium.org

[msramek, 2016-05-25 13:21:09]

Thanks! browsing_data/ LGTM.

[vabr (Chromium), 2016-05-25 13:22:46]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org
- vabr@google.com

[cfroussios, 2016-05-25 13:37:15]

cfroussios@chromium.org changed reviewers:
+ thestig@chromium.org

[cfroussios, 2016-05-25 13:37:16]

thestig@chromium.org:

This CL contains the skeleton for mocking oscrypt and applies it to affected
tests. You've seen the upcoming implementation in 
https://codereview.chromium.org/1973483002/

Please review the changes in:

components/os_crypt/*
chrome/test/base/in_process_browser_test.cc
chrome/browser/ui/webui/bidi_checker_web_ui_test.cc
chrome/browser/sync/test/integration/sync_test.cc
chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
chrome/browser/signin/local_auth_unittest.cc

Thanks a lot!

[vabr (Chromium), 2016-05-25 13:50:52]

LGTM, thanks!

[cfroussios, 2016-05-25 13:52:30]

cfroussios@chromium.org changed reviewers:
+ maxbogue@chromium.org

[cfroussios, 2016-05-25 13:52:31]

maxbogue@chromium.org: Please review changes in 

components/sync_driver/BUILD.gn
components/sync_driver/system_encryptor_unittests.cc

Thanks!

[cfroussios, 2016-05-25 13:59:29]

cfroussios@chromium.org changed reviewers:
+ rogerta@chromium.org

[cfroussios, 2016-05-25 13:59:30]

rogerta@chromium.org: Please review changes in 

components/signin/core/browser/BUILD.gn
components/signin/core/browser/webdata/token_service_table_unittest.cc

Thanks a lot!

[Roger Tawa OOO till Jul 10th, 2016-05-25 15:06:32]

lgtm

[Lei Zhang, 2016-05-25 21:02:03]

Description was changed from

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
 abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

[Lei Zhang, 2016-05-25 21:02:15]

Description was changed from

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.


BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.

BUG=602624
==========

[Lei Zhang, 2016-05-25 21:11:41]

lgtm for os_crypt and chrome/ sans password_manager

https://codereview.chromium.org/2010463002/diff/200001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/2010463002/diff/200001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:224: // Always use the MockKeychain
if OS encription is used (which is when
This looks out of date now.

https://codereview.chromium.org/2010463002/diff/200001/components/os_crypt/os...
File components/os_crypt/os_crypt_mocker.cc (right):

https://codereview.chromium.org/2010463002/diff/200001/components/os_crypt/os...
components/os_crypt/os_crypt_mocker.cc:19: OSCrypt::UseMockKeychain(false);
Thanks. I found it odd that nobody ever called this.

[maxbogue, 2016-05-27 15:10:59]

lgtm for components/sync_driver! Sorry it took me so long, I completely missed
the email for this :(

[cfroussios, 2016-05-27 15:28:22]

@maxbogue No worries. Thanks!

https://codereview.chromium.org/2010463002/diff/200001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/2010463002/diff/200001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:224: // Always use the MockKeychain
if OS encription is used (which is when
On 2016/05/25 21:11:40, Lei Zhang wrote:
> This looks out of date now.

Done.

https://codereview.chromium.org/2010463002/diff/200001/components/os_crypt/os...
File components/os_crypt/os_crypt_mocker.cc (right):

https://codereview.chromium.org/2010463002/diff/200001/components/os_crypt/os...
components/os_crypt/os_crypt_mocker.cc:19: OSCrypt::UseMockKeychain(false);
On 2016/05/25 21:11:40, Lei Zhang wrote:
> Thanks. I found it odd that nobody ever called this.

Acknowledged.

[cfroussios, 2016-05-27 15:29:02]

cfroussios@chromium.org changed reviewers:
+ sdefresne@chromium.org

[cfroussios, 2016-05-27 15:29:06]

sdefresne@chromium.org: Please review changes in 

components/os_crypt.gypi

Thanks!

[cfroussios, 2016-05-27 15:31:53]

The CQ bit was checked by cfroussios@chromium.org

[cfroussios, 2016-05-27 15:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, rogerta@chromium.org, vabr@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2010463002/#ps220001
(title: "Updated doc")

[commit-bot: I haz the power, 2016-05-27 15:32:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2010463002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2010463002/220001

[commit-bot: I haz the power, 2016-05-27 15:37:17]

Description was changed from

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.

BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.

BUG=602624
==========

[commit-bot: I haz the power, 2016-05-27 15:37:19]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-05-27 15:39:07]

Description was changed from

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g. 
Keychain). Tests should avoid involving such services. 
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target 
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction 
replaces previous OS-specific mocking instructions in 
tests. Additionally, I added it to some 
tests which previously omitted mocking entirely.

BUG=602624
==========

to

==========
OSCryptMocker

OSCrypt uses OS-level password storage services (e.g.
Keychain). Tests should avoid involving such services.
Currently, tests do that with OS-specific code. Soon,
linux will need such mocking too.

This CL introduces the build target
//components/os_crypt:test_support, which contains an
abstraction for mocking: OSCryptMocker. This abstraction
replaces previous OS-specific mocking instructions in
tests. Additionally, I added it to some
tests which previously omitted mocking entirely.

BUG=602624

Committed: https://crrev.com/d281ff9e63f782362ef6ea0ccdde6c1238f34431
Cr-Commit-Position: refs/heads/master@{#396472}
==========

[commit-bot: I haz the power, 2016-05-27 15:39:08]

Patchset 11 (id:??) landed as
https://crrev.com/d281ff9e63f782362ef6ea0ccdde6c1238f34431
Cr-Commit-Position: refs/heads/master@{#396472}

[cfroussios, 2016-05-27 15:40:50]

cfroussios@chromium.org changed reviewers:
- sdefresne@chromium.org

[cfroussios, 2016-05-27 15:40:51]

@sdefresne Sorry I added you. I did not notice that the ownership of
components/os_crypt.gypi was updated.

[sdefresne, 2016-05-28 13:10:07]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2016-05-28 13:10:08]

components/os_crypt.gypi lgtm