Move push event dispatching out of ServiceWorkerVersion.

content/browser/service_worker/service_worker_version_unittest.cc

Index: content/browser/service_worker/service_worker_version_unittest.cc
diff --git a/content/browser/service_worker/service_worker_version_unittest.cc b/content/browser/service_worker/service_worker_version_unittest.cc
index 3895f55d6a95f6db2522f1e4cb5517c194bfd23d..489018f7e858d0afdd5447250be8c134b1a69928 100644
--- a/content/browser/service_worker/service_worker_version_unittest.cc
+++ b/content/browser/service_worker/service_worker_version_unittest.cc
@@ -238,6 +238,28 @@ class ServiceWorkerVersionTest : public testing::Test {
     helper_.reset();
   }
 
+  void SimulateDispatchEvent() {
+    ServiceWorkerStatusCode status =
+        SERVICE_WORKER_ERROR_NETWORK;  // dummy value

[nhiroki, 2016/01/15 01:03:42]

nit: SERVICE_WORKER_ERROR_MAX_VALUE would be more suitable for a dummy value. I
use it these days.

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

Done

+
+    // Make sure worker is running.
+    version_->StartWorker(CreateReceiverOnCurrentThread(&status));
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(SERVICE_WORKER_OK, status);
+    EXPECT_EQ(ServiceWorkerVersion::RUNNING, version_->running_status());
+
+    // Start request, as if an event is being dispatched.
+    int request_id =
+        version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,
+                               CreateReceiverOnCurrentThread(&status));
+    base::RunLoop().RunUntilIdle();
+
+    // And finish request, as if a response to the event was received.
+    EXPECT_TRUE(version_->FinishRequest(request_id));

[johnme, 2016/01/18 19:06:19]

The previous test was actually dispatching an event, rather than merely calling
StartRequest and FinishRequest. Is this still testing the same thing?

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

Since StartRequest and FinishRequest are the only parts of dispatching an event
that effect ServiceWorkerVersion, yes, this should still be the same. But I must
admit that I'm not entirely sure what some of these tests are testing.

Other unit tests ensure that DispatchEvent (and the equivalent mojo codepath)
work correctly, so it should be safe for tests that just "dispatch an event" to
not actually have any mock ipc/mojo going on.

[johnme, 2016/01/20 12:01:10]

Ok, since nhiroki is happy that's probably fine then :)

[nhiroki, 2016/01/20 14:55:54]

For these StaleUpdate_* tests, StartWorker is the most important part and
actually we don't have to call StartRequest/FinishRequest.

StartWorker calls MarkIfStale in the middle of the sequence, and it updates
|stale_time_| if registration's last update check was done over 24 hours ago. If
|stale_time_| is updated, StopTimeoutTimer called from StoppedWorker starts
|update_timer_|. These tests check the behavior.

+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(SERVICE_WORKER_OK, status);
+  }
+
   TestBrowserThreadBundle thread_bundle_;
   scoped_ptr<MessageReceiver> helper_;
   scoped_refptr<ServiceWorkerRegistration> registration_;
@@ -700,16 +722,11 @@ TEST_F(ServiceWorkerVersionTest, StoppingBeforeDestruct) {
 
 // Test that update isn't triggered for a non-stale worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_FreshWorker) {
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(base::Time::Now());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
+  SimulateDispatchEvent();
 
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 }
@@ -737,10 +754,7 @@ TEST_F(ServiceWorkerVersionTest, StaleUpdate_StartWorker) {
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(GetYesterday());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  SimulateDispatchEvent();
   EXPECT_FALSE(version_->stale_time_.is_null());
   EXPECT_FALSE(version_->update_timer_.IsRunning());
 
@@ -754,16 +768,11 @@ TEST_F(ServiceWorkerVersionTest, StaleUpdate_StartWorker) {
 
 // Test that staleness is detected on a running worker.
 TEST_F(ServiceWorkerVersionTest, StaleUpdate_RunningWorker) {
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-
   // Start a fresh worker.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetActiveVersion(version_);
   registration_->set_last_update_check(base::Time::Now());
-  version_->DispatchPushEvent(CreateReceiverOnCurrentThread(&status),
-                              std::string());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  SimulateDispatchEvent();
   EXPECT_TRUE(version_->stale_time_.is_null());
 
   // Simulate it running for a day. It will be marked stale.
@@ -795,11 +804,16 @@ TEST_F(ServiceWorkerVersionTest, StaleUpdate_DoNotDeferTimer) {
           ServiceWorkerVersion::kStartNewWorkerTimeoutMinutes + 1);
   version_->stale_time_ = stale_time;
 
+  // Make sure worker is running.
+  version_->StartWorker(base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(ServiceWorkerVersion::RUNNING, version_->running_status());
+
   // Stale time is not deferred.
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
+  version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,

[johnme, 2016/01/18 19:06:19]

Ditto is this testing the same thing?

[Marijn Kruisselbrink, 2016/01/20 01:06:31]

As I'm not entirely sure what this test is testing I'm not 100% sure, but I
believe that yes, this is testing the same thing, and is doing so arguably
better by only actually executing the code that matters and not also including
unrelated mock IPC stuff.
In particular I'm not sure if the FinishRequest calls in the second half of this
test are needed. And if they are maybe the test should inspect some state before
them as well (which wasn't really possible in the old test since
DispatchPushEvent always got a reply more or less immediately).

[johnme, 2016/01/20 12:01:10]

Ok, since nhiroki is happy that's probably fine then :)

[nhiroki, 2016/01/20 14:55:54]

Hmmm... sorry, I completely overlooked this. As I explained in my other comment,
StartWorker() is the most important part. This test makes sure that these start
worker requests don't update |stale_time_| in MarkIfStale because it is already
non-null. Probably we should call StartWorker instead of StartRequest.

[Marijn Kruisselbrink, 2016/01/21 23:38:21]

I changed these to call RunAfterStartWorker isntead of StartRequest.

+                         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  version_->StartRequest(ServiceWorkerMetrics::EventType::PUSH,
+                         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));

[nhiroki, 2016/01/20 14:55:54]

Can you call RunUntilIdle here?

(Actually this should not be necessary because there is a ""live"" registration
for the version and StartRequest can synchronously call MarkIfStale via
FindRegistrationForId, but I think test code should not depend on such an
internal behavior.)

[Marijn Kruisselbrink, 2016/01/21 23:38:21]

Done. Yeah, I was confused for a while how this could possibly be testing the
right thing without actually waiting for possibly asynchronous stuff to
complete.

   EXPECT_EQ(stale_time, version_->stale_time_);
 
   // Timeout triggers the update.
@@ -809,12 +823,18 @@ TEST_F(ServiceWorkerVersionTest, StaleUpdate_DoNotDeferTimer) {
 
   // Update timer is not deferred.
   base::TimeTicks run_time = version_->update_timer_.desired_run_time();
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
-  version_->DispatchPushEvent(
-      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback), std::string());
+  int request_id1 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  int request_id2 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  int request_id3 = version_->StartRequest(
+      ServiceWorkerMetrics::EventType::PUSH,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  EXPECT_TRUE(version_->FinishRequest(request_id1));
+  EXPECT_TRUE(version_->FinishRequest(request_id2));
+  EXPECT_TRUE(version_->FinishRequest(request_id3));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(version_->stale_time_.is_null());
   EXPECT_EQ(run_time, version_->update_timer_.desired_run_time());