Discussion about File Stream Reader. (Abandoned)

chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc

Index: chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..48a8376dfa80b4871e3eddc608b6a6028f67eafc
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
@@ -0,0 +1,292 @@
+// Copyright 2014 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 <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace file_system_provider {
+namespace {
+
+// Size of the fake file in bytes.
+const int kFileSize = 1024;
+
+// Size of the preloading buffer in bytes.
+const int kBufferSize = 8;
+
+// Number of bytes requested per BufferingFileStreamReader::Read().
+const int kChunkSize = 3;
+
+// Behaviour tested by a test case.
+enum TestMode { SYNCHRONOUS, ASYNCHRONOUS };
+
+// Registers and counts completion callback calls.
+class CountedTestCompletionCallback : public net::TestCompletionCallback {
+ public:
+  CountedTestCompletionCallback() : called_times_(0) {}
+  virtual ~CountedTestCompletionCallback() {}
+
+  // Returns how many times the callback has been called.
+  int called_times() const { return called_times_; }
+
+  // GetResult() can't be used if we don't know the argument. So we need another
+  // method for such case. Note, that the result may be undefined if SetResult
+  // was never called.
+  int MaybeGetResultWithoutWaiting() const { return result_; }
+
+ protected:
+  void SetResult(int result) {

[hashimoto, 2014/06/12 12:22:06]

Why you can't just EXPECT_FALSE(have_result()) instead of counting?

[mtomasz, 2014/06/13 00:32:51]

EXPECT_FALSE(have_result()) within SetResult? That would make logs unreadable,
since we don't know exactly which instance of CountedTestCompletionCallback
failed on this check.

If you wanted to do this EXPECT_FALSE in a test case directly, then it wouldn't
work for #206. Note, that more complex tests are coming soon, when the chunk
size is dynamically calculated, instead of always set to 512KB.

[hashimoto, 2014/06/13 06:49:56]

If you really want to leave that information, you can override |result| with
ERR_IO_PENDING or -10000 or any value which is not expected by the test code to
leave the failing line number on the log.
Or, you can give a name to each CountedTestCompletionCallback instance by
passing a std::string to the ctor and emit logs on failures.
It should work as have_result_ is reset to false at the end of WaitForResult().
Also, as I said, you don't need to use TestCompletionCallback for the said
purpose.

[mtomasz, 2014/06/16 03:29:36]

I really don't think that having a class, which overrides another class, adding
a name with extra custom logging and ending up on asserts in two different
places is better than a trivial logging class. It is like trying to reuse code
at any cost, while own code would be much simpler, shorter and easier to
understand. Hence, easier to maintain.

You addressed some of my concerns, but skipped the concern about freezing tests,
when the callback is not called. Anyway, this discussion took already longer
than it should. Since we agree on using Logger, let's just go with the logger,
even if it is not the best solution.

[hashimoto, 2014/06/19 06:45:06]

The one thing I don't totally understand from the beginning is that why you
spend 40 lines for this class which is equivalent to this 5 line struct. (clang
might be upset for the lack of ctor and dtor so it may get additional couple of
lines)
struct Logger {
  std::vector<int> inner_read_requested_events;
  std::vector<int> read_result_events;
  std::vector<int64> get_length_result_events;
};
Your "logger" looks simple because it's doing nothing meaningful.
TestCompletionCallback encapsulates the whole "Run()->copy the result->Quit()"
process and it also absorbs the difference between ERR_IO_PENDING case and
results returned synchronously.
It removes the repeated pattern from the test body and makes it easy to add new
tests while this "logger" does nothing more than std::vector can do.
By "freeze", you mean Run() waiting for Quit()?
RunUntilIdle() can also "freeze" when some class has a bug and keeps posting
tasks to the message loop.
Yes, I'm OK with this thing if you like it as I said.
I wanted to share my concerns with you hoping it helps you improve the code, and
also to keep it recorded that I had these concerns.

[mtomasz, 2014/06/20 01:16:20]

This comparison is not fair. You didn't add comments to your struct, nor methods
to push a callback result to one of these vectors (+ comments to it). Note, that
you will need two callbacks - one accepting int, one accepting int64. I counted
roughly and it would make your solution grow from 5 to 21 lines (while my logger
class is 40 lines).
You're comparing Logger class to TestCompletionCallback, but I don't know why.
Of course TestCompletionCallback is much more complex. I think it is also
incorrectly written, since it may freeze tests very easily and also cause
reading uninitialized values.

The Logger class is trivial, but it groups everything related to logging in one
place. It also makes Binds simple. Having a trivial class doesn't cost. Having
more complex Binds does.
Do you suggest that we replace RoonLoop::RunUntilIdle() with RunLoop::Run() in
all our unit tests then?

We can't help if some class keeps posting tasks. But we can help if it forgets
to call a callback. If we can write precise and fast tests, why not doing that?
To avoid some lines of code? This argument doesn't make sense to me.

[hashimoto, 2014/06/27 01:35:55]

What you need is only one template function.
No.
I just meant that neither RunUntilIdle() nor Run() can handle wrongly behaving
classes perfectly.
You don't like TestCompletionCallback because it uses Run() and does not emit
test failures when it gets called multiple times.
I'm not sure why you specially care about these problems here because
TestCompletionCallback is widely used and we're doing fine.
Of course, it's not the perfect solution, but it does its work in most cases.
If you find an obvious pitfall in TestCompletionCallback, then you should
consider improving it rather than having your own alternative to benefit
everyone.

Even if TestCompletionCallback is not the right thing to be used here, the
design of Logger here is far from the perfection.
1. Logger is repeating everything 3 times for 3 vectors.
2. Logger is mixing up two different types of information: arguments passed to
the inner reader and results returned from BufferingFileStreamReader.
3. Logger doesn't provide a convenient function like
TestCompletionCallback::callback() with which the test body doesn't have to
repeat lengthy Bind() expressions.
4. Even if the result callback is expected to be called only once in every case,
you have to check the result vector's size everywhere.
5. If the result vector's size is different from the expected value, the test
aborts without printing any actual returned values.  What you can know from the
log is the size of the vector only.
6. When you change the test body, you may end up fixing expected sizes and
indices everywhere. For example, if you want to remove the <Do task A> from the
code below, you have to fix the all expected sizes and the indices used to get
the returned value.
  <Do task A> 
  ASSERT_EQ(1u, logger.read_result_events().size());
  EXPECT_EQ(kAAA, logger.read_result_events()[0]);
  <Do task B>
  ASSERT_EQ(2u, logger.read_result_events().size());
  EXPECT_EQ(kBBB, logger.read_result_events()[1]);
  <Do task C>
  ASSERT_EQ(3u, logger.read_result_events().size());
  EXPECT_EQ(kCCC, logger.read_result_events()[2]);
  ...

I recommend you to use the existing well-written class TestCompletionCallback,
or at least polish this class's design.

[hashimoto, 2014/06/27 01:35:55]

What you need is only one template function.
No.
I just meant that neither RunUntilIdle() nor Run() can handle wrongly behaving
classes perfectly.
You don't like TestCompletionCallback because it uses Run() and does not emit
test failures when it gets called multiple times.
I'm not sure why you specially care about these problems here because
TestCompletionCallback is widely used and we're doing fine.
Of course, it's not the perfect solution, but it does its work in most cases.
If you find an obvious pitfall in TestCompletionCallback, then you should
consider improving it rather than having your own alternative to benefit
everyone.

Even if TestCompletionCallback is not the right thing to be used here, the
design of Logger here is far from the perfection.
1. Logger is repeating everything 3 times for 3 vectors.
2. Logger is mixing up two different types of information: arguments passed to
the inner reader and results returned from BufferingFileStreamReader.
3. Logger doesn't provide a convenient function like
TestCompletionCallback::callback() with which the test body doesn't have to
repeat lengthy Bind() expressions.
4. Even if the result callback is expected to be called only once in every case,
you have to check the result vector's size everywhere.
5. If the result vector's size is different from the expected value, the test
aborts without printing any actual returned values.  What you can know from the
log is the size of the vector only.
6. When you change the test body, you may end up fixing expected sizes and
indices everywhere. For example, if you want to remove the <Do task A> from the
code below, you have to fix the all expected sizes and the indices used to get
the returned value.
  <Do task A> 
  ASSERT_EQ(1u, logger.read_result_events().size());
  EXPECT_EQ(kAAA, logger.read_result_events()[0]);
  <Do task B>
  ASSERT_EQ(2u, logger.read_result_events().size());
  EXPECT_EQ(kBBB, logger.read_result_events()[1]);
  <Do task C>
  ASSERT_EQ(3u, logger.read_result_events().size());
  EXPECT_EQ(kCCC, logger.read_result_events()[2]);
  ...

I recommend you to use the existing well-written class TestCompletionCallback,
or at least polish this class's design.

[mtomasz, 2014/06/27 05:27:54]

It's hard to test everything. But if we can test more (or more precisely)
*easily*, then I don't get why we wouldn't want to do that.
Yet other problem is that we need some logger anyway. For the inner reader, we
can't use TestCompletionCallback. IIUC, you want to test half with
TestCompletionCallback, and half with some kind of a logger.
I'm not sure if spending too much time on making code *perfect* is the best use
of our engineering time.
How would you like to reduce it in the Logger class?
What's wrong about it? Logger class logs activities happened in a test case.
Please clarify. The current Binds are short and easy. What's wrong about them? I
don't see much advantage in to adding ::callback() methods to the Logger class.
What's wrong about it? How would you check values returned by a callback without
checking that the callback is called?
It is much better than TestCompletionCallback, which wouldn't give us any
information.
I believe just having an assert about size, is a good balance between Logger
class complexity and amount of output details.
The more detailed tests we have, the more we have to update in case of a change
in the code. I don't see issues with the code above.
Since we need the logger class anyway (for the internal reader), and it is
already written, and it doesn't cause timeouts, and it results in detailed
tests, I'd like to go with a Logger.

Since all of your issues are related to style, please let me know exactly what
do you want me to fix.

+    net::TestCompletionCallback::SetResult(result);
+    called_times_++;
+  }
+
+ private:
+  int called_times_;
+  DISALLOW_COPY_AND_ASSIGN(CountedTestCompletionCallback);
+};
+
+// Fake internal file stream reader.
+class FakeFileStreamReader : public webkit_blob::FileStreamReader {
+ public:
+  FakeFileStreamReader(const net::CompletionCallback& read_called_callback,

[hashimoto, 2014/06/12 12:22:06]

Sorry for not emphasizing this:
As I said, running TestCompletionCallback to record arguments in this class is
totally wrong because it quits the run loop as you might find.
You can pass a pointer to a Logger which only has inner_read_requested_events_,
or just a pointer to a std::vector<int>. (both are essentially same)

[mtomasz, 2014/06/13 00:32:51]

Got it. However, still a lot of tricks remain, if we use TestCompletionCallback
for other cases.
Eg. (1) it can freeze the test, (2) we can have an uninitialized value, so we
need to check have_result always, (3) we can't use WaitForResult, and GetResult
for some cases (See 1), and hence we have to manually call RunLoop.

To sum up this long discussion, IMHO TestCompletionCallback saves some lines of
code, but in return we have a lot of hacks and difficult to maintain code. If
you think TestCompletionCallback is better here, let me know why. Otherwise, I'd
like to go with a Logger class, which is IMHO way simpler and easier to maintain
solution.

[hashimoto, 2014/06/13 06:49:56]

This code is messed up because TestCompletionCallback is wrongly used so that
Quit() is called twice during each method call.
If you stop passing TestCompletionCallback to the fake reader, things will get
much simpler.
As I said, although I don't recognize any need to implement homemade "logger"
class here, I'm OK with the proposed test code if you really like it.

[mtomasz, 2014/06/16 03:29:36]

The code will be still messed up if we don't use it for the inner reader.

[hashimoto, 2014/06/19 06:45:06]

Why don't you demonstrate it in this CL? (IIUC demonstrating how the code looks
like with TestCompletionCallback was the entire purpose of this CL)

[mtomasz, 2014/06/20 01:16:20]

It's already there. Lines #137 - #149 are very complicated, but not related to
the inner reader.

+                       TestMode mode,
+                       bool return_error)
+      : read_called_callback_(read_called_callback),
+        mode_(mode),
+        return_error_(return_error) {}
+  virtual ~FakeFileStreamReader() {}
+
+  // webkit_blob::FileStreamReader overrides.
+  virtual int Read(net::IOBuffer* buf,
+                   int buf_len,
+                   const net::CompletionCallback& callback) OVERRIDE {
+    read_called_callback_.Run(buf_len);
+
+    if (return_error_) {
+      if (mode_ == SYNCHRONOUS)
+        return net::ERR_ACCESS_DENIED;
+
+      base::MessageLoopProxy::current()->PostTask(
+          FROM_HERE, base::Bind(callback, net::ERR_ACCESS_DENIED));
+      return net::ERR_IO_PENDING;
+    }
+
+    const std::string fake_data('X', buf_len);
+    memcpy(buf->data(), fake_data.c_str(), buf_len);
+
+    if (mode_ == SYNCHRONOUS)
+      return buf_len;
+
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+                                                base::Bind(callback, buf_len));
+    return net::ERR_IO_PENDING;
+  }
+
+  virtual int64 GetLength(
+      const net::Int64CompletionCallback& callback) OVERRIDE {
+    if (mode_ == SYNCHRONOUS)
+      return kFileSize;
+
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, base::Bind(callback, kFileSize));
+    return net::ERR_IO_PENDING;
+  }
+
+ private:
+  net::CompletionCallback read_called_callback_;
+  TestMode mode_;
+  bool return_error_;
+  DISALLOW_COPY_AND_ASSIGN(FakeFileStreamReader);
+};
+
+}  // namespace
+
+class FileSystemProviderBufferingFileStreamReaderTest
+    : public testing::Test,
+      public ::testing::WithParamInterface<TestMode> {
+ protected:
+  FileSystemProviderBufferingFileStreamReaderTest() {}
+  virtual ~FileSystemProviderBufferingFileStreamReaderTest() {}
+
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, Read) {
+  CountedTestCompletionCallback inner_result;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          inner_result.callback(), GetParam(), false /* return_error */)),
+      kBufferSize);
+
+  // For the first read, the internal file stream reader is fired, as there is
+  // no data in the preloading buffer.
+  {
+    CountedTestCompletionCallback read_result;
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    const int result = reader.Read(buffer, kChunkSize, read_result.callback());
+    // Calling WaitForResult() would freeze the test if the callback is not
+    // called. It is better to assert than timeout.
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    base::RunLoop().RunUntilIdle();
+    // If the callback wasn't called, then read_result_value is undefined.

[mtomasz, 2014/06/20 03:24:11]

This is not true that it would be undefined. Instead WaitForResult() would run
the message loop and cause a test timeout.

+    ASSERT_EQ(1, read_result.called_times());
+    // GetResult() will not freeze, because we are sure that the callback is
+    // already called (#144).

[mtomasz, 2014/06/12 04:13:16]

#144 -> #145.

+    const int read_result_value = read_result.GetResult(result);
+    EXPECT_EQ(kChunkSize, read_result_value);
+
+    // Calling WaitForResult() would freeze the test if the callback is not
+    // called. It is better to assert than timeout.
+    base::RunLoop().RunUntilIdle();
+    // If the callback wasn't called, then inner_result_value is undefined.
+    ASSERT_EQ(1, inner_result.called_times());
+    // We don't know the result returned by the internal Read(), so we can't
+    // call inner_result.GetResult(), simply because we don't know the arugment.
+    const int inner_result_value = inner_result.MaybeGetResultWithoutWaiting();
+    EXPECT_EQ(kBufferSize, inner_result_value);
+  }
+
+  // Second read should return data from the preloading buffer, without calling
+  // the internal file stream reader.
+  {
+    CountedTestCompletionCallback read_result;
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    const int result = reader.Read(buffer, kChunkSize, read_result.callback());
+    // Results returned synchronously, so the callback should not be called.
+    // GetResult() may cause RunLoop::Run(), and hence freeze the test if
+    // |result| is ERR_IO_PENDING, and the callback is not called.
+    EXPECT_EQ(kChunkSize, result);
+    base::RunLoop()
+        .RunUntilIdle();  // read_result.WaitForResult() would freeze!
+    EXPECT_FALSE(read_result.have_result());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(1, inner_result.called_times());
+  }
+
+  // Third read should return partial result from the preloading buffer. It is
+  // valid to return less bytes than requested.
+  {
+    CountedTestCompletionCallback read_result;
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    const int result = reader.Read(buffer, kChunkSize, read_result.callback());
+    EXPECT_EQ(kBufferSize - 2 * kChunkSize, result);
+    base::RunLoop()
+        .RunUntilIdle();  // read_result.WaitForResult() would freeze!
+    EXPECT_FALSE(read_result.have_result());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(1, inner_result.called_times());
+  }
+
+  // The preloading buffer is now empty, so reading should invoke the internal
+  // file stream reader.
+  {
+    CountedTestCompletionCallback read_result;
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    const int result = reader.Read(buffer, kChunkSize, read_result.callback());
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    const int read_result_value = read_result.GetResult(result);
+    EXPECT_EQ(kChunkSize, read_result_value);
+    // If the callback is not called, then we will freeze. Asserting is better
+    // than freezing until the test timeouts.
+    // const int inner_result_value = inner_result.WaitForResult();
+    base::RunLoop().RunUntilIdle();
+    ASSERT_EQ(2, inner_result.called_times());
+    // We don't know the result returned by the internal Read(), so we can't
+    // call inner_result.GetResult(), simply because we don't know the arugment.
+    EXPECT_EQ(kBufferSize, inner_result.MaybeGetResultWithoutWaiting());
+  }
+}
+
+/*
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest,
+       Read_MoreThanBufferSize) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), false)),
+      kBufferSize);
+
+  // Returning less than requested number of bytes is valid, and should not
+  // fail.
+  const int chunk_size = 20;
+  ASSERT_LT(kBufferSize, chunk_size);
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(chunk_size));
+  const int result =
+      reader.Read(buffer,
+                  chunk_size,
+                  base::Bind(&Logger::OnReadResult, base::Unretained(&logger)));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, logger.inner_read_requested_events().size());
+  EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+  ASSERT_EQ(1u, logger.read_result_events().size());
+  EXPECT_EQ(kBufferSize, logger.read_result_events()[0]);
+}
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, Read_WithError) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), true)),
+      kBufferSize);
+
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+  const int result =
+      reader.Read(buffer,
+                  kChunkSize,
+                  base::Bind(&Logger::OnReadResult, base::Unretained(&logger)));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, logger.inner_read_requested_events().size());
+  EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+  ASSERT_EQ(1u, logger.read_result_events().size());
+  EXPECT_EQ(net::ERR_ACCESS_DENIED, logger.read_result_events()[0]);
+}
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, GetLength) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), false)),
+      kBufferSize);
+
+  const int64 result = reader.GetLength(
+      base::Bind(&Logger::OnGetLengthResult, base::Unretained(&logger)));
+  base::RunLoop().RunUntilIdle();
+
+  if (GetParam() == SYNCHRONOUS) {
+    EXPECT_EQ(kFileSize, result);
+    EXPECT_EQ(0u, logger.get_length_result_events().size());
+  } else {
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, logger.get_length_result_events().size());
+    EXPECT_EQ(kFileSize, logger.get_length_result_events()[0]);
+  }
+}
+*/
+
+INSTANTIATE_TEST_CASE_P(Synchronous,
+                        FileSystemProviderBufferingFileStreamReaderTest,
+                        testing::Values(SYNCHRONOUS));
+
+INSTANTIATE_TEST_CASE_P(Asynchronous,
+                        FileSystemProviderBufferingFileStreamReaderTest,
+                        testing::Values(ASYNCHRONOUS));
+
+}  // namespace file_system_provider
+}  // namespace chromeos