Skip to content

Commit

Permalink
Fold NotificationWatcher into WindowedNotificationObserver
Browse files Browse the repository at this point in the history 
The two helper classes are very similar and constitute unnecessary code
duplication.

BUG=NONE
TEST=All tests still pass

TBR=sky (He accidentally LGTM'd CL 15849011 instead)
TBR=nkostylev (existing_user_controller_browsertest.cc)
TBR=mnissler (device_local_account_browsertest.cc)

Review URL: https://chromiumcodereview.appspot.com/16408020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@205960 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
bartfab@chromium.org committed Jun 12, 2013
1 parent dadac57 commit bde236e
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 116 deletions.
45 changes: 3 additions & 42 deletions chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@
#include "chrome/test/base/ui_test_utils.h"
#include "chromeos/chromeos_switches.h"
#include "chromeos/dbus/fake_session_manager_client.h"
#include "content/public/browser/notification_details.h"
#include "content/public/browser/notification_service.h"
#include "content/public/test/mock_notification_observer.h"
#include "content/public/test/test_utils.h"
#include "google_apis/gaia/mock_url_fetcher_factory.h"
#include "grit/generated_resources.h"
#include "testing/gmock/include/gmock/gmock.h"
Expand Down Expand Up @@ -78,44 +77,6 @@ ACTION_P2(CreateAuthenticator, username, password) {
return new MockAuthenticator(arg0, username, password);
}

// Observes a specific notification type and quits the message loop once a
// condition holds.
class NotificationWatcher : public content::NotificationObserver {
public:
// Callback invoked on notifications. Should return true when the condition
// that the caller is waiting for is satisfied.
typedef base::Callback<bool(void)> ConditionTestCallback;

explicit NotificationWatcher(int notification_type,
const ConditionTestCallback& callback)
: type_(notification_type),
callback_(callback) {}

void Run() {
if (callback_.Run())
return;

content::NotificationRegistrar registrar;
registrar.Add(this, type_, content::NotificationService::AllSources());
run_loop_.Run();
}

// content::NotificationObserver:
virtual void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) OVERRIDE {
if (callback_.Run())
run_loop_.Quit();
}

private:
int type_;
ConditionTestCallback callback_;
base::RunLoop run_loop_;

DISALLOW_COPY_AND_ASSIGN(NotificationWatcher);
};

} // namespace

class ExistingUserControllerTest : public policy::DevicePolicyCrosBrowserTest,
Expand Down Expand Up @@ -405,11 +366,11 @@ class ExistingUserControllerPublicSessionTest

// Wait for the public session user to be created.
if (!chromeos::UserManager::Get()->IsKnownUser(public_session_user_id_)) {
NotificationWatcher(
content::WindowedNotificationObserver(
chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&chromeos::UserManager::IsKnownUser,
base::Unretained(chromeos::UserManager::Get()),
public_session_user_id_)).Run();
public_session_user_id_)).Wait();
}

// Wait for the device local account policy to be installed.
Expand Down
84 changes: 24 additions & 60 deletions chrome/browser/chromeos/policy/device_local_account_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "base/basictypes.h"
#include "base/bind.h"
#include "base/callback.h"
#include "base/command_line.h"
#include "base/file_util.h"
#include "base/files/file_path.h"
Expand Down Expand Up @@ -51,10 +50,8 @@
#include "chromeos/dbus/fake_session_manager_client.h"
#include "chromeos/dbus/mock_dbus_thread_manager_without_gmock.h"
#include "chromeos/dbus/session_manager_client.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/test_utils.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/cros_system_api/dbus/service_constants.h"

Expand All @@ -75,44 +72,6 @@ const char* kStartupURLs[] = {
"chrome://about",
};

// Observes a specific notification type and quits the message loop once a
// condition holds.
class NotificationWatcher : public content::NotificationObserver {
public:
// Callback invoked on notifications. Should return true when the condition
// that the caller is waiting for is satisfied.
typedef base::Callback<bool(void)> ConditionTestCallback;

explicit NotificationWatcher(int notification_type,
const ConditionTestCallback& callback)
: type_(notification_type),
callback_(callback) {}

void Run() {
if (callback_.Run())
return;

content::NotificationRegistrar registrar;
registrar.Add(this, type_, content::NotificationService::AllSources());
run_loop_.Run();
}

// content::NotificationObserver:
virtual void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) OVERRIDE {
if (callback_.Run())
run_loop_.Quit();
}

private:
int type_;
ConditionTestCallback callback_;
base::RunLoop run_loop_;

DISALLOW_COPY_AND_ASSIGN(NotificationWatcher);
};

} // namespace

class DeviceLocalAccountTest : public InProcessBrowserTest {
Expand Down Expand Up @@ -292,10 +251,12 @@ static bool IsKnownUser(const std::string& account_id) {
}

IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LoginScreen) {
NotificationWatcher(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_1_)).Run();
NotificationWatcher(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_2_)).Run();
content::WindowedNotificationObserver(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_1_))
.Wait();
content::WindowedNotificationObserver(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_2_))
.Wait();

CheckPublicSessionPresent(user_id_1_);
CheckPublicSessionPresent(user_id_2_);
Expand All @@ -312,19 +273,19 @@ static bool DisplayNameMatches(const std::string& account_id,
}

IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, DisplayName) {
NotificationWatcher(
content::WindowedNotificationObserver(
chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&DisplayNameMatches, user_id_1_, kDisplayName1)).Run();
base::Bind(&DisplayNameMatches, user_id_1_, kDisplayName1)).Wait();
}

IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, PolicyDownload) {
// Policy for kAccountId2 is not installed in session_manager_client, make
// sure it gets fetched from the server. Note that the test setup doesn't set
// up policy for kAccountId2, so the presence of the display name can be used
// as signal to indicate successful policy download.
NotificationWatcher(
content::WindowedNotificationObserver(
chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&DisplayNameMatches, user_id_2_, kDisplayName2)).Run();
base::Bind(&DisplayNameMatches, user_id_2_, kDisplayName2)).Wait();

// Sanity check: The policy should be present now.
ASSERT_FALSE(session_manager_client_->device_local_account_policy(
Expand All @@ -337,10 +298,12 @@ static bool IsNotKnownUser(const std::string& account_id) {

IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, DevicePolicyChange) {
// Wait until the login screen is up.
NotificationWatcher(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_1_)).Run();
NotificationWatcher(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_2_)).Run();
content::WindowedNotificationObserver(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_1_))
.Wait();
content::WindowedNotificationObserver(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsKnownUser, user_id_2_))
.Wait();

// Update policy to remove kAccountId2.
em::ChromeDeviceSettingsProto policy;
Expand All @@ -356,8 +319,9 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, DevicePolicyChange) {
g_browser_process->policy_service()->RefreshPolicies(base::Closure());

// Make sure the second device-local account disappears.
NotificationWatcher(chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsNotKnownUser, user_id_2_)).Run();
content::WindowedNotificationObserver(
chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&IsNotKnownUser, user_id_2_)).Wait();
}

static bool IsSessionStarted() {
Expand All @@ -368,9 +332,9 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, StartSession) {
// This observes the display name becoming available as this indicates
// device-local account policy is fully loaded, which is a prerequisite for
// successful login.
NotificationWatcher(
content::WindowedNotificationObserver(
chrome::NOTIFICATION_USER_LIST_CHANGED,
base::Bind(&DisplayNameMatches, user_id_1_, kDisplayName1)).Run();
base::Bind(&DisplayNameMatches, user_id_1_, kDisplayName1)).Wait();

chromeos::LoginDisplayHost* host =
chromeos::LoginDisplayHostImpl::default_host();
Expand All @@ -382,8 +346,8 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, StartSession) {
controller->LoginAsPublicAccount(user_id_1_);

// Wait for the session to start.
NotificationWatcher(chrome::NOTIFICATION_SESSION_STARTED,
base::Bind(IsSessionStarted)).Run();
content::WindowedNotificationObserver(chrome::NOTIFICATION_SESSION_STARTED,
base::Bind(IsSessionStarted)).Wait();

// Check that the startup pages specified in policy were opened.
EXPECT_EQ(1U, chrome::GetTotalBrowserCount());
Expand Down
14 changes: 12 additions & 2 deletions content/public/test/test_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,20 @@ WindowedNotificationObserver::WindowedNotificationObserver(
registrar_.Add(this, notification_type, source);
}

WindowedNotificationObserver::WindowedNotificationObserver(
int notification_type,
const ConditionTestCallback& callback)
: seen_(false),
running_(false),
callback_(callback),
source_(NotificationService::AllSources()) {
registrar_.Add(this, notification_type, source_);
}

WindowedNotificationObserver::~WindowedNotificationObserver() {}

void WindowedNotificationObserver::Wait() {
if (seen_)
if (callback_.is_null() ? seen_ : callback_.Run())
return;

running_ = true;
Expand All @@ -206,7 +216,7 @@ void WindowedNotificationObserver::Observe(
source_ = source;
details_ = details;
seen_ = true;
if (!running_)
if (!running_ || (!callback_.is_null() && !callback_.Run()))
return;

message_loop_runner_->Quit();
Expand Down
48 changes: 36 additions & 12 deletions content/public/test/test_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#ifndef CONTENT_PUBLIC_TEST_TEST_UTILS_H_
#define CONTENT_PUBLIC_TEST_TEST_UTILS_H_

#include "base/callback_forward.h"
#include "base/callback.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/run_loop.h"
Expand Down Expand Up @@ -99,30 +99,52 @@ class MessageLoopRunner : public base::RefCounted<MessageLoopRunner> {
DISALLOW_COPY_AND_ASSIGN(MessageLoopRunner);
};

// A WindowedNotificationObserver allows code to watch for a notification
// over a window of time. Typically testing code will need to do something
// like this:
// A WindowedNotificationObserver allows code to wait until a condition is met.
// Simple conditions are specified by providing a |notification_type| and a
// |source|. When a notification of the expected type from the expected source
// is received, the condition is met.
// More complex conditions can be specified by providing a |notification_type|
// and a |callback|. The callback encapsulates the logic that determines whether
// the condition has been met. If the callback returns |true|, the condition is
// met. Otherwise, the condition is not yet met and the callback will be invoked
// again every time a notification of the expected type is received until the
// callback returns |true|.
//
// This helper class exists to avoid the following common pattern in tests:
// PerformAction()
// WaitForCompletionNotification()
// This leads to flakiness as there's a window between PerformAction returning
// and the observers getting registered, where a notification will be missed.
// The pattern leads to flakiness as there is a window between PerformAction
// returning and the observers getting registered, where a notification will be
// missed.
//
// Rather, one can do this:
// WindowedNotificationObserver signal(...)
// PerformAction()
// signal.Wait()
class WindowedNotificationObserver : public NotificationObserver {
public:
// Register to listen for notifications of the given type from either a
// specific source, or from all sources if |source| is
// NotificationService::AllSources().
// Callback invoked on notifications. Should return |true| when the condition
// being waited for is met.
typedef base::Callback<bool(void)> ConditionTestCallback;

// Set up to wait for a simple condition. The condition is met when a
// notification of the given |notification_type| from the given |source| is
// received. To accept notifications from all sources, specify
// NotificationService::AllSources() as |source|.
WindowedNotificationObserver(int notification_type,
const NotificationSource& source);

// Set up to wait for a complex condition. The condition is met when
// |callback| returns |true|. The callback is invoked whenever a notification
// of |notification_type| from any source is received.
WindowedNotificationObserver(int notification_type,
const ConditionTestCallback& callback);

virtual ~WindowedNotificationObserver();

// Wait until the specified notification occurs. If the notification was
// emitted between the construction of this object and this call then it
// returns immediately.
// Wait until the specified condition is met. If the condition is already met
// (that is, the expected notification has already been received or the
// given callback returns |true| already), Wait() returns immediately.
void Wait();

// Returns NotificationService::AllSources() if we haven't observed a
Expand All @@ -145,6 +167,8 @@ class WindowedNotificationObserver : public NotificationObserver {
bool running_;
NotificationRegistrar registrar_;

ConditionTestCallback callback_;

NotificationSource source_;
NotificationDetails details_;
scoped_refptr<MessageLoopRunner> message_loop_runner_;
Expand Down

0 comments on commit bde236e

Please sign in to comment.