Unit test for Data Saver prompt notification

Unit test for Data Saver prompt notification

BUG=465607
Sets up logged in user and cellular network for testing, verifies that
the Data Saver prompt notification is shown once cellular becomes the
connected, default network.

Committed: https://crrev.com/3f5393c9dd01900f7a27a89df1a81a1d00f40bb8
Cr-Commit-Position: refs/heads/master@{#327497}

[Greg Levin, 2015-04-27 23:03:08]

glevin@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Greg Levin, 2015-04-27 23:03:09]

Here's my unit test... thanks for the help getting started!  Please have a look.

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification_unittest.cc (right):

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:85:
FakeChromeUserManager* user_manager = new FakeChromeUserManager();
Does this need a matching delete somewhere?  I copied this from
network_portal_detector_impl_unittest.cc, which doesn't seem to have one.

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:110: const
char kCellularDevicePath[] = "/device/stub_cellular_device1";
1) Do we prefer "const char" or "const std::string"?

2) For const strings that only get used in one function, do we prefer defining
them at the top of the file, or right before their first use in the function?

For each question, I’ve seen both used in unittest files.

[stevenjb, 2015-04-28 22:32:59]

nice. lgtm.

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification_unittest.cc (right):

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:85:
FakeChromeUserManager* user_manager = new FakeChromeUserManager();
On 2015/04/27 23:03:08, Greg Levin wrote:
> Does this need a matching delete somewhere?  I copied this from
> network_portal_detector_impl_unittest.cc, which doesn't seem to have one.

No, because we are passing ownership to user_manager_enabler_, but to make that
clear this should be:

scoped_ptr<UserManager> user_manager(new FakeChromeUserManager());
user_manager_enabler_.reset(new
ScopedUserManagerEnabler(user_manager.release()));

Ideally ScopedUserManagerEnabler would take a scoped_ptr<UserManager> to make
the ownership passing explicit, and we would use Pass() instead of release().

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:110: const
char kCellularDevicePath[] = "/device/stub_cellular_device1";
On 2015/04/27 23:03:08, Greg Levin wrote:
> 1) Do we prefer "const char" or "const std::string"?
> 
> 2) For const strings that only get used in one function, do we prefer defining
> them at the top of the file, or right before their first use in the function?
> 
> For each question, I’ve seen both used in unittest files.

1) Outside of the scope of a function or class we need to use const char[] to
avoid the overhead of global constructors that have to occur on application
startup. Inside a function const std::string is fine (but you'll see both people
are used to the global case).
2) If it's only used once you can just inline it same as e.g.
"stub_cellular_device1", particularly in a unit test. If it's used more than
once it's probably better at the top of the file.

[Greg Levin, 2015-04-29 14:46:09]

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification_unittest.cc (right):

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:85:
FakeChromeUserManager* user_manager = new FakeChromeUserManager();
On 2015/04/28 22:32:59, stevenjb wrote:
> On 2015/04/27 23:03:08, Greg Levin wrote:
> > Does this need a matching delete somewhere?  I copied this from
> > network_portal_detector_impl_unittest.cc, which doesn't seem to have one.
> 
> No, because we are passing ownership to user_manager_enabler_, but to make
that
> clear this should be:
> 
> scoped_ptr<UserManager> user_manager(new FakeChromeUserManager());
> user_manager_enabler_.reset(new
> ScopedUserManagerEnabler(user_manager.release()));
> 
> Ideally ScopedUserManagerEnabler would take a scoped_ptr<UserManager> to make
> the ownership passing explicit, and we would use Pass() instead of release().

Ok, got it, thanks!  I rearranged a couple lines to accommodate.

https://codereview.chromium.org/1108123003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification_unittest.cc:110: const
char kCellularDevicePath[] = "/device/stub_cellular_device1";
On 2015/04/28 22:32:59, stevenjb wrote:
> On 2015/04/27 23:03:08, Greg Levin wrote:
> > 1) Do we prefer "const char" or "const std::string"?
> > 
> > 2) For const strings that only get used in one function, do we prefer
defining
> > them at the top of the file, or right before their first use in the
function?
> > 
> > For each question, I’ve seen both used in unittest files.
> 
> 1) Outside of the scope of a function or class we need to use const char[] to
> avoid the overhead of global constructors that have to occur on application
> startup. Inside a function const std::string is fine (but you'll see both
people
> are used to the global case).
> 2) If it's only used once you can just inline it same as e.g.
> "stub_cellular_device1", particularly in a unit test. If it's used more than
> once it's probably better at the top of the file.

Done.

[Greg Levin, 2015-04-29 14:46:30]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2015-04-29 14:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1108123003/#ps40001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2015-04-29 14:46:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108123003/40001

[commit-bot: I haz the power, 2015-04-29 15:43:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-29 15:44:54]

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