Skip to content

Commit

Permalink
Fix LockScreenNoteTakingTest.DataAvailableOnRestart
Browse files Browse the repository at this point in the history 
The test has PRE_DataAvailableOnRestart part which creates a note in
lock screen note storage. DataAvailableOnRestart then verifies that a
onDataItemsAvailable event is dispatched as the user session is started.
The event is dispatched early on during the test - before the test body
is run, which means there is no guarantee that a result catcher created
in the test body will be created before the test app runs all the tests,
and thus it might miss a test result message.

To fix the issue, update the LockScreenNoteTakingTest setup to create a
result catcher just after the browser main parts are created - this should
be early enough to catch any messages from the test app (i.e. before the
event observed by the test app is dispatched), but late enough to have
NotificationService available (without it extensions::ResultCatcher
cannot be created).

BUG=901616

Change-Id: I58b3ccda9b93bc355bab7fb1f24e2d4a769c44d8
Reviewed-on: https://chromium-review.googlesource.com/c/1324870
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606277}
  • Loading branch information
Toni Barzic authored and Commit Bot committed Nov 8, 2018
1 parent df72f9b commit a050208
Showing 1 changed file with 33 additions and 14 deletions.
47 changes: 33 additions & 14 deletions chrome/browser/chromeos/lock_screen_apps/note_taking_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,26 @@ class LockScreenNoteTakingTest : public extensions::ExtensionBrowserTest {
extensions::ExtensionBrowserTest::SetUpCommandLine(cmd_line);
}

void CreatedBrowserMainParts(
content::BrowserMainParts* browser_main_parts) override {
// Creating result catcher to be used by tests eary on to avoid flaky hangs
// in the DataAvailableOnRestart test.
// The tests expects the test app (which is installed in the tests PRE_
// part) to run in response to the
// lockScreen.data.onDataItemsAvailable event which is dispatched early on,
// during "user" session start-up, which happens before test body is run.
// This means that result catchers created in the test body might miss test
// completion notifications from the app.
result_catcher_ = std::make_unique<extensions::ResultCatcher>();
extensions::ExtensionBrowserTest::CreatedBrowserMainParts(
browser_main_parts);
}

void TearDownOnMainThread() override {
result_catcher_.reset();
extensions::ExtensionBrowserTest::TearDownOnMainThread();
}

bool EnableLockScreenAppLaunch(const std::string& app_id) {
chromeos::NoteTakingHelper::Get()->SetPreferredApp(profile(), app_id);
chromeos::NoteTakingHelper::Get()->SetPreferredAppEnabledOnLockScreen(
Expand Down Expand Up @@ -120,7 +140,6 @@ class LockScreenNoteTakingTest : public extensions::ExtensionBrowserTest {
ExtensionTestMessageListener ready_to_close("readyToClose",
true /* will_reply */);

extensions::ResultCatcher catcher;
lock_screen_apps::StateController::Get()->RequestNewLockScreenNote(
ash::mojom::LockScreenNoteOrigin::kLockScreenButtonTap);

Expand All @@ -137,8 +156,8 @@ class LockScreenNoteTakingTest : public extensions::ExtensionBrowserTest {
// wait for it to be closed
// Test runner should wait for both of those to finish (test result message
// will be sent for each set of tests).
if (!catcher.GetNextResult()) {
*error = catcher.message();
if (!result_catcher_->GetNextResult()) {
*error = result_catcher_->message();
if (ready_to_close.was_satisfied())
ready_to_close.Reply("failed");
return false;
Expand Down Expand Up @@ -166,15 +185,19 @@ class LockScreenNoteTakingTest : public extensions::ExtensionBrowserTest {
// Close the app window created by the API test.
ready_to_close.Reply("close");

if (!catcher.GetNextResult()) {
*error = catcher.message();
if (!result_catcher_->GetNextResult()) {
*error = result_catcher_->message();
return false;
}

return true;
}

extensions::ResultCatcher* result_catcher() { return result_catcher_.get(); }

private:
std::unique_ptr<extensions::ResultCatcher> result_catcher_;

DISALLOW_COPY_AND_ASSIGN(LockScreenNoteTakingTest);
};

Expand Down Expand Up @@ -203,7 +226,6 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, LaunchInNonLockScreenContext) {
ASSERT_TRUE(app);
ASSERT_TRUE(EnableLockScreenAppLaunch(app->id()));

extensions::ResultCatcher catcher;

// Get the lock screen apps state controller to the state where lock screen
// enabled app window creation is allowed (provided the window is created
Expand All @@ -226,7 +248,7 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, LaunchInNonLockScreenContext) {
apps::LaunchPlatformAppWithAction(profile(), app.get(),
std::move(action_data), base::FilePath());

ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
ASSERT_TRUE(result_catcher()->GetNextResult()) << result_catcher()->message();
}

IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, DataCreation) {
Expand All @@ -240,14 +262,13 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, DataCreation) {
EXPECT_EQ(ash::mojom::TrayActionState::kAvailable,
lock_screen_apps::StateController::Get()->GetLockScreenNoteState());

extensions::ResultCatcher catcher;
session_manager::SessionManager::Get()->SetSessionState(
session_manager::SessionState::ACTIVE);

// Unlocking the session should trigger onDataItemsAvailable event, which
// should be catched by the background page in the main app - the event should
// start another test sequence.
ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
ASSERT_TRUE(result_catcher()->GetNextResult()) << result_catcher()->message();
}

IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, PRE_DataAvailableOnRestart) {
Expand All @@ -269,8 +290,7 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, DataAvailableOnRestart) {
// lock screen app's data storage is not empty), which should in turn run a
// sequence of API tests (in the test app background page).
// This test is intended to catch the result of these tests.
extensions::ResultCatcher catcher;
ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
ASSERT_TRUE(result_catcher()->GetNextResult()) << result_catcher()->message();
}

IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, AppLaunchActionDataParams) {
Expand All @@ -281,7 +301,6 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, AppLaunchActionDataParams) {
ASSERT_TRUE(app);
ASSERT_TRUE(EnableLockScreenAppLaunch(app->id()));

extensions::ResultCatcher catcher;

lock_screen_apps::StateController::Get()->RequestNewLockScreenNote(
ash::mojom::LockScreenNoteOrigin::kLockScreenButtonTap);
Expand All @@ -295,7 +314,7 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, AppLaunchActionDataParams) {
expected_action_data.Reply(R"({"actionType": "new_note",
"isLockScreenAction": true,
"restoreLastActionState": true})");
ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
ASSERT_TRUE(result_catcher()->GetNextResult()) << result_catcher()->message();
expected_action_data.Reset();

// Reset the lock screen app state by resetting screen lock, so the app is
Expand All @@ -316,5 +335,5 @@ IN_PROC_BROWSER_TEST_F(LockScreenNoteTakingTest, AppLaunchActionDataParams) {
expected_action_data.Reply(R"({"actionType": "new_note",
"isLockScreenAction": true,
"restoreLastActionState": false})");
ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
ASSERT_TRUE(result_catcher()->GetNextResult()) << result_catcher()->message();
}

0 comments on commit a050208

Please sign in to comment.