Add UploadProgressTracker unittest

content/browser/loader/upload_progress_tracker_unittest.cc

Index: content/browser/loader/upload_progress_tracker_unittest.cc
diff --git a/content/browser/loader/upload_progress_tracker_unittest.cc b/content/browser/loader/upload_progress_tracker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6410b314ac694760321992d4a7272cedb8a6f76b
--- /dev/null
+++ b/content/browser/loader/upload_progress_tracker_unittest.cc
@@ -0,0 +1,218 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/loader/upload_progress_tracker.h"
+
+#include "base/test/test_simple_task_runner.h"
+#include "net/base/upload_progress.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+namespace {
+
+class FakeUploadProgressTrackerClient : public UploadProgressTracker::Client {
+ public:
+  FakeUploadProgressTrackerClient() {}
+  ~FakeUploadProgressTrackerClient() override {}
+
+  net::UploadProgress GetUploadProgress() override {
+    return net::UploadProgress(current_position_, total_size_);
+  }
+
+  void ReportUploadProgress(int64_t current_position,
+                            int64_t total_size) override {
+    ++report_count_;
+    current_position_ = current_position;
+    total_size_ = total_size;

[mmenke, 2017/01/11 19:31:13]

Both of these two lines seems wrong.

Seems like we should either do:
EXPECT_EQ(current_position_, current_position);
EXPECT_EQ(total_size_, total_size);

And/or we should store them in separate values, that are not the same as the
ones returned by GetUploadProgress.

[tzik, 2017/01/12 13:00:57]

Oh... Right, it's wrong.

+  }
+
+  int report_count() const { return report_count_; }
+  int64_t current_position() const { return current_position_; }
+  int64_t total_size() const { return total_size_; }
+
+  void set_current_position(int64_t current_position) {
+    current_position_ = current_position;
+  }
+
+  void set_total_size(int64_t total_size) { total_size_ = total_size; }
+
+ private:
+  int report_count_ = 0;
+  int64_t current_position_ = 0;
+  int64_t total_size_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeUploadProgressTrackerClient);
+};
+
+class TestingUploadProgressTracker : public UploadProgressTracker {
+ public:
+  TestingUploadProgressTracker(
+      const tracked_objects::Location& location,
+      Client* client,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : UploadProgressTracker(location, client, std::move(task_runner)) {}
+
+  void set_current_time(const base::TimeTicks& current_time) {
+    current_time_ = current_time;
+  }
+
+ protected:
+  base::TimeTicks GetCurrentTime() override { return current_time_; }

[mmenke, 2017/01/11 19:31:14]

Can be private

[tzik, 2017/01/12 13:00:57]

Done.

+
+  base::TimeTicks current_time_;

[mmenke, 2017/01/11 19:31:14]

Should be private.

[tzik, 2017/01/12 13:00:57]

Done.

+};

[mmenke, 2017/01/11 19:31:14]

DISALLOW_COPY_AND_ASSIGN

[tzik, 2017/01/12 13:00:57]

Done.

+
+}  // namespace
+
+class UploadProgressTrackerTest : public ::testing::Test {
+ public:
+  UploadProgressTrackerTest()
+      : task_runner_(new base::TestSimpleTaskRunner),
+        upload_progress_tracker_(FROM_HERE, &tracker_client_, task_runner_) {}

[mmenke, 2017/01/11 19:31:14]

include base/location.h for FROM_HERE

[tzik, 2017/01/12 13:00:57]

Done.

+
+ protected:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  FakeUploadProgressTrackerClient tracker_client_;
+  TestingUploadProgressTracker upload_progress_tracker_;
+};
+
+TEST_F(UploadProgressTrackerTest, NoACK) {
+  tracker_client_.set_current_position(50);
+  tracker_client_.set_total_size(100);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(50, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+
+  tracker_client_.set_current_position(75);
+
+  // The second timer task does nothing, since the first task didn't send ACK.
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+}
+
+TEST_F(UploadProgressTrackerTest, NoUpload) {
+  tracker_client_.set_current_position(0);
+  tracker_client_.set_total_size(0);
+
+  // UploadProgressTracker does nothing on the empty upload content.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(0, tracker_client_.report_count());
+}
+
+TEST_F(UploadProgressTrackerTest, NoProgress) {
+  tracker_client_.set_current_position(50);
+  tracker_client_.set_total_size(100);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(50, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+
+  upload_progress_tracker_.OnAckReceived();
+
+  // The second time doesn't call ReportUploadProgress since there's no
+  // progress.
+  EXPECT_EQ(1, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+}
+
+TEST_F(UploadProgressTrackerTest, Finished) {
+  tracker_client_.set_current_position(999);
+  tracker_client_.set_total_size(1000);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(999, tracker_client_.current_position());
+  EXPECT_EQ(1000, tracker_client_.total_size());
+
+  upload_progress_tracker_.OnAckReceived();
+  tracker_client_.set_current_position(1000);
+
+  // The second timer task calls ReportUploadProgress for reporting the
+  // completion.
+  EXPECT_EQ(1, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(2, tracker_client_.report_count());
+  EXPECT_EQ(1000, tracker_client_.current_position());
+  EXPECT_EQ(1000, tracker_client_.total_size());
+}
+
+TEST_F(UploadProgressTrackerTest, Progress) {
+  tracker_client_.set_current_position(50);
+  tracker_client_.set_total_size(100);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(50, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+
+  upload_progress_tracker_.OnAckReceived();
+  tracker_client_.set_current_position(75);
+
+  // The second timer task calls ReportUploadProgress since the progress is
+  // big enough to report.
+  EXPECT_EQ(1, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(2, tracker_client_.report_count());
+  EXPECT_EQ(75, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+}
+
+TEST_F(UploadProgressTrackerTest, TimePassed) {

[mmenke, 2017/01/11 19:31:13]

Can this just be a copy of NoProgress, with a change from 50->51 (Plus time
update).  Just suggesting making tests more similar when possible so it's easier
to compare them.

[tzik, 2017/01/12 13:00:57]

Done.

+  tracker_client_.set_current_position(500);
+  tracker_client_.set_total_size(1000);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(500, tracker_client_.current_position());
+  EXPECT_EQ(1000, tracker_client_.total_size());

[mmenke, 2017/01/11 19:31:14]

Suggest using 500 / 1000 instead of 50/100 on all of these tests, just to make
them easier to compare.

[tzik, 2017/01/12 13:00:57]

Done.

+
+  upload_progress_tracker_.OnAckReceived();
+  tracker_client_.set_current_position(501);
+  upload_progress_tracker_.set_current_time(base::TimeTicks::Now());

[mmenke, 2017/01/11 19:31:14]

Relying on returning a time of 0 by default seems a bit weird.  Suggest
defaulting time to base::TimeTicks::Now() in the UploadProgressTracker
constructor, and incrementing it by whatever is necessary here, to make it
trigger.

[tzik, 2017/01/12 13:00:57]

Done.

+
+  // The second timer task calls ReportUploadProgress since it's been long time
+  // from the last report.
+  EXPECT_EQ(1, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(2, tracker_client_.report_count());
+  EXPECT_EQ(501, tracker_client_.current_position());
+  EXPECT_EQ(1000, tracker_client_.total_size());

[mmenke, 2017/01/11 19:31:13]

Also suggest a copy of this test where progress goes backwards (500->499) (Also
with the time delta).  Progress can be rewound in two cases:  Redirects (Entire
body was uploaded, but we don't guarantee a progress update at 100% in that
case, and even if we did, we don't destroy the UploadProgressTracker).  Also,
sometimes we retry the request after we've uploaded it.  The retry is mostly
invisible to consumers, but can see progress go backwards, and tracker shouldn't
report that.

[mmenke, 2017/01/11 19:31:14]

Suggest another copy of this test, without the time delta, which should not send
an update.

[tzik, 2017/01/12 13:00:57]

Done.

+}
+
+TEST_F(UploadProgressTrackerTest, Completed) {
+  tracker_client_.set_current_position(50);
+  tracker_client_.set_total_size(100);
+
+  // The first timer task calls ReportUploadProgress.
+  EXPECT_EQ(0, tracker_client_.report_count());
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, tracker_client_.report_count());
+  EXPECT_EQ(50, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+
+  tracker_client_.set_current_position(100);
+
+  // OnUploadCompleted runs ReportUploadProgress even without Ack nor timer.
+  upload_progress_tracker_.OnUploadCompleted();
+  EXPECT_EQ(2, tracker_client_.report_count());
+  EXPECT_EQ(100, tracker_client_.current_position());
+  EXPECT_EQ(100, tracker_client_.total_size());
+}

[mmenke, 2017/01/11 19:31:14]

It's not exciting, but maybe a test with an upload of unknown size (-1).

[tzik, 2017/01/12 13:00:57]

IIUC, request bodies always have fixed sizes that are known when the request was
issued.
The request body is from RequestInit#body, and its BodyInit is a union of types
that have known sizes, unlike ResponseBodyInit.
https://fetch.spec.whatwg.org/#request-class
https://fetch.spec.whatwg.org/#bodyinit

[mmenke, 2017/01/12 16:06:54]

That's not true when talking about Chrome in general (Since we do
browser-initiated chunked uploads to Google services), but for requests that
currently go through this code, you're right.

+
+}  // namespace context