Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2169223002:
Implements unittests for hats notification controller (Closed)
|
Created:
4 years, 5 months ago by malaykeshav Modified:
4 years, 5 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements unittests for hats notification controller
BUG=611781
COMPONENT=Chrome OS, HaTS, Unittest
Committed: https://crrev.com/645052fc36e185431edffa4b83f8ccb6b0c39309
Cr-Commit-Position: refs/heads/master@{#407592}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Implements unittests for hats notification controller #
Total comments: 2
Patch Set 3 : Implements unittests for hats notification controller #
Total comments: 4
Patch Set 4 : Implements unittests for hats notification controller #
Messages
Total messages: 31 (20 generated)
The CQ bit was checked by malaykeshav@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.
malaykeshav@chromium.org changed reviewers: + jdufault@chromium.org, stevenjb@chromium.org
Hooray for tests. https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:6: #include "chrome/browser/chromeos/first_run/goodies_displayer.h" needed? https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:25: #include "ui/message_center/message_center.h" This is a lot of includes, are they all needed? https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:109: // true. How are we testing that initialization fails? https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:133: content::RunAllBlockingPoolTasksUntilIdle(); Does it make sense to group the above into a helper method? https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:141: hats_notification_controller->Initialize(false); So, are we relying entirely on AddAndFireObserve getting called in the portal detector to test successful initialization? Is there a way we can do that explicitly? https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:174: new NetworkState(std::string()), online_state); It is not clear here where we are actually testing anything. https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:192: g_browser_process->notification_ui_manager()); Do we need the static cast? Also, no need for a local here for a single use.
https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:6: #include "chrome/browser/chromeos/first_run/goodies_displayer.h" On 2016/07/22 at 16:14:54, stevenjb wrote: > needed? Removed https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:25: #include "ui/message_center/message_center.h" On 2016/07/22 at 16:14:54, stevenjb wrote: > This is a lot of includes, are they all needed? Done https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:109: // true. On 2016/07/22 at 16:14:54, stevenjb wrote: > How are we testing that initialization fails? When initialization fails, the timestamp for kHatsLastInteractionTimestamp is updated to Time::Now(). https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:133: content::RunAllBlockingPoolTasksUntilIdle(); On 2016/07/22 at 16:14:53, stevenjb wrote: > Does it make sense to group the above into a helper method? Done https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:141: hats_notification_controller->Initialize(false); On 2016/07/22 at 16:14:54, stevenjb wrote: > So, are we relying entirely on AddAndFireObserve getting called in the portal detector to test successful initialization? Is there a way we can do that explicitly? Its more of a starting with the initialization. It starts a check for internet connectivity. Creation of notification and adding it to the notification center is a different test. https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:174: new NetworkState(std::string()), online_state); On 2016/07/22 at 16:14:54, stevenjb wrote: > It is not clear here where we are actually testing anything. Added the final check condition https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:192: g_browser_process->notification_ui_manager()); On 2016/07/22 at 16:14:54, stevenjb wrote: > Do we need the static cast? Also, no need for a local here for a single use. Removed
https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:109: // true. On 2016/07/22 20:03:09, malaykeshav wrote: > On 2016/07/22 at 16:14:54, stevenjb wrote: > > How are we testing that initialization fails? > > When initialization fails, the timestamp for kHatsLastInteractionTimestamp is > updated to Time::Now(). That is not at all obvious, we should add a comment. Also "now_timestamp" is very confusing since "now" is no longer true in the ASSERT. I would suggest "initial_timestamp". https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:141: hats_notification_controller->Initialize(false); On 2016/07/22 20:03:09, malaykeshav wrote: > On 2016/07/22 at 16:14:54, stevenjb wrote: > > So, are we relying entirely on AddAndFireObserve getting called in the portal > detector to test successful initialization? Is there a way we can do that > explicitly? > > Its more of a starting with the initialization. It starts a check for internet > connectivity. > > Creation of notification and adding it to the notification center is a different > test. My point is, it is not at all clear that anything is actually getting tested here. One specifically should fail here if initialization does not start? Ideally we should be able to explicitly confirm that initialization started. At minimum we should comment how we are testing for that. Adding an observer does not necessarily imply that initialization succeeded. https://codereview.chromium.org/2169223002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:178: EXPECT_TRUE(notification == nullptr); I believe that EXPECT_FALSE(notification) should work?
The CQ bit was checked by malaykeshav@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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:109: // true. On 2016/07/22 at 20:13:53, stevenjb wrote: > On 2016/07/22 20:03:09, malaykeshav wrote: > > On 2016/07/22 at 16:14:54, stevenjb wrote: > > > How are we testing that initialization fails? > > > > When initialization fails, the timestamp for kHatsLastInteractionTimestamp is > > updated to Time::Now(). > > That is not at all obvious, we should add a comment. Also "now_timestamp" is very confusing since "now" is no longer true in the ASSERT. I would suggest "initial_timestamp". Done https://codereview.chromium.org/2169223002/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:141: hats_notification_controller->Initialize(false); Combined this with the test below to check for whether the notification is created. https://codereview.chromium.org/2169223002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:178: EXPECT_TRUE(notification == nullptr); On 2016/07/22 at 20:13:53, stevenjb wrote: > I believe that EXPECT_FALSE(notification) should work? Done
The CQ bit was checked by malaykeshav@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.
https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:112: new NetworkState(std::string()), online_state); This will leak. Even though it's a test, we should declare NetworkState locally and pass a pointer to it to OnPortalDetectionComplete. https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:189: new NetworkState(std::string()), online_state); All of these should pass a pointer to a local NetworkState.
https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc (right): https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:112: new NetworkState(std::string()), online_state); On 2016/07/25 at 16:04:18, stevenjb wrote: > This will leak. Even though it's a test, we should declare NetworkState locally and pass a pointer to it to OnPortalDetectionComplete. Done https://codereview.chromium.org/2169223002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc:189: new NetworkState(std::string()), online_state); On 2016/07/25 at 16:04:18, stevenjb wrote: > All of these should pass a pointer to a local NetworkState. Done
The CQ bit was checked by malaykeshav@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by malaykeshav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implements unittests for hats notification controller BUG=611781 COMPONENT=Chrome OS, HaTS, Unittest ========== to ========== Implements unittests for hats notification controller BUG=611781 COMPONENT=Chrome OS, HaTS, Unittest Committed: https://crrev.com/645052fc36e185431edffa4b83f8ccb6b0c39309 Cr-Commit-Position: refs/heads/master@{#407592} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/645052fc36e185431edffa4b83f8ccb6b0c39309 Cr-Commit-Position: refs/heads/master@{#407592} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
