Discussion about File Stream Reader. (Abandoned)

Discussion about File Stream Reader.

BUG=

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

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:147:
// already called (#144).
#144 -> #145.

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

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
Why you can't just EXPECT_FALSE(have_result()) instead of counting?

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
Sorry for not emphasizing this:
> Sorry, I forgot to note that inner_read_requested_events_ is the exception.
> This vector is serving for a totally different purpose while other vectors can
be easily replaced with TestCompletionCallback.
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]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/12 12:22:06, hashimoto wrote:
> Why you can't just EXPECT_FALSE(have_result()) instead of counting?

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.

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
On 2014/06/12 12:22:06, hashimoto wrote:
> Sorry for not emphasizing this:
> > Sorry, I forgot to note that inner_read_requested_events_ is the exception.
> > This vector is serving for a totally different purpose while other vectors
can
> be easily replaced with TestCompletionCallback.
> 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)

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]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/13 00:32:51, mtomasz wrote:
> On 2014/06/12 12:22:06, hashimoto wrote:
> > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> 
> 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 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.
> 
> 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
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.
> size is dynamically calculated, instead of always set to 512KB.

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
On 2014/06/13 00:32:51, mtomasz wrote:
> On 2014/06/12 12:22:06, hashimoto wrote:
> > Sorry for not emphasizing this:
> > > Sorry, I forgot to note that inner_read_requested_events_ is the
exception.
> > > This vector is serving for a totally different purpose while other vectors
> can
> > be easily replaced with TestCompletionCallback.
> > 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)
> 
> 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.
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.
> 
> 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.

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:37]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/13 06:49:56, hashimoto wrote:
> On 2014/06/13 00:32:51, mtomasz wrote:
> > On 2014/06/12 12:22:06, hashimoto wrote:
> > > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> > 
> > 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 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.

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.

> > 
> > 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
> 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.
> > size is dynamically calculated, instead of always set to 512KB.
>

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
On 2014/06/13 06:49:56, hashimoto wrote:
> On 2014/06/13 00:32:51, mtomasz wrote:
> > On 2014/06/12 12:22:06, hashimoto wrote:
> > > Sorry for not emphasizing this:
> > > > Sorry, I forgot to note that inner_read_requested_events_ is the
> exception.
> > > > This vector is serving for a totally different purpose while other
vectors
> > can
> > > be easily replaced with TestCompletionCallback.
> > > 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)
> > 
> > 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.
> 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.

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

> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/16 03:29:36, mtomasz wrote:
> On 2014/06/13 06:49:56, hashimoto wrote:
> > On 2014/06/13 00:32:51, mtomasz wrote:
> > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> > > 
> > > 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 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.
> 
> 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.
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.
> 
> 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
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.
> than it should. Since we agree on using Logger, let's just go with the logger,
> even if it is not the best solution.
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.
> 
> > > 
> > > 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
> > 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.
> > > size is dynamically calculated, instead of always set to 512KB.
> > 
>

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
On 2014/06/16 03:29:36, mtomasz wrote:
> On 2014/06/13 06:49:56, hashimoto wrote:
> > On 2014/06/13 00:32:51, mtomasz wrote:
> > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > Sorry for not emphasizing this:
> > > > > Sorry, I forgot to note that inner_read_requested_events_ is the
> > exception.
> > > > > This vector is serving for a totally different purpose while other
> vectors
> > > can
> > > > be easily replaced with TestCompletionCallback.
> > > > 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)
> > > 
> > > 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.
> > 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.
> 
> The code will be still messed up if we don't use it for the inner reader.
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)
> 
> > > 
> > > 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.
> > 
> > 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-20 01:16:20]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/19 06:45:06, hashimoto wrote:
> On 2014/06/16 03:29:36, mtomasz wrote:
> > On 2014/06/13 06:49:56, hashimoto wrote:
> > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> > > > 
> > > > 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 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.
> > 
> > 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.
> 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;
> };

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).

> 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.

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.

> > 
> > 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
> 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.

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.

> > than it should. Since we agree on using Logger, let's just go with the
logger,
> > even if it is not the best solution.
> 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.
> > 
> > > > 
> > > > 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
> > > 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.
> > > > size is dynamically calculated, instead of always set to 512KB.
> > > 
> > 
>

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:64:
FakeFileStreamReader(const net::CompletionCallback& read_called_callback,
On 2014/06/19 06:45:06, hashimoto wrote:
> On 2014/06/16 03:29:36, mtomasz wrote:
> > On 2014/06/13 06:49:56, hashimoto wrote:
> > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > Sorry for not emphasizing this:
> > > > > > Sorry, I forgot to note that inner_read_requested_events_ is the
> > > exception.
> > > > > > This vector is serving for a totally different purpose while other
> > vectors
> > > > can
> > > > > be easily replaced with TestCompletionCallback.
> > > > > 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)
> > > > 
> > > > 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.
> > > 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.
> > 
> > The code will be still messed up if we don't use it for the inner reader.
> 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)

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

> > 
> > > > 
> > > > 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.
> > > 
> > > 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-20 03:24:12]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:144:
// If the callback wasn't called, then read_result_value is undefined.
This is not true that it would be undefined. Instead WaitForResult() would run
the message loop and cause a test timeout.

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

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/20 01:16:20, mtomasz wrote:
> On 2014/06/19 06:45:06, hashimoto wrote:
> > On 2014/06/16 03:29:36, mtomasz wrote:
> > > On 2014/06/13 06:49:56, hashimoto wrote:
> > > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> > > > > 
> > > > > 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 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.
> > > 
> > > 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.
> > 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;
> > };
> 
> 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
What you need is only one template function.
> roughly and it would make your solution grow from 5 to 21 lines (while my
logger
> class is 40 lines).
> 
> > 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.
> 
> 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.
> 
> > > 
> > > 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
> > 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.
> 
> Do you suggest that we replace RoonLoop::RunUntilIdle() with RunLoop::Run() in
> all our unit tests then?
No.
I just meant that neither RunUntilIdle() nor Run() can handle wrongly behaving
classes perfectly.
> 
> 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.
> 
> > > than it should. Since we agree on using Logger, let's just go with the
> logger,
> > > even if it is not the best solution.
> > 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.
> > > 
> > > > > 
> > > > > 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
> > > > 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.
> > > > > size is dynamically calculated, instead of always set to 512KB.
> > > > 
> > > 
> > 
> 

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.

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/20 01:16:20, mtomasz wrote:
> On 2014/06/19 06:45:06, hashimoto wrote:
> > On 2014/06/16 03:29:36, mtomasz wrote:
> > > On 2014/06/13 06:49:56, hashimoto wrote:
> > > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > > Why you can't just EXPECT_FALSE(have_result()) instead of counting?
> > > > > 
> > > > > 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 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.
> > > 
> > > 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.
> > 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;
> > };
> 
> 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
What you need is only one template function.
> roughly and it would make your solution grow from 5 to 21 lines (while my
logger
> class is 40 lines).
> 
> > 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.
> 
> 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.
> 
> > > 
> > > 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
> > 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.
> 
> Do you suggest that we replace RoonLoop::RunUntilIdle() with RunLoop::Run() in
> all our unit tests then?
No.
I just meant that neither RunUntilIdle() nor Run() can handle wrongly behaving
classes perfectly.
> 
> 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.
> 
> > > than it should. Since we agree on using Logger, let's just go with the
> logger,
> > > even if it is not the best solution.
> > 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.
> > > 
> > > > > 
> > > > > 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
> > > > 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.
> > > > > size is dynamically calculated, instead of always set to 512KB.
> > > > 
> > > 
> > 
> 

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]

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
void SetResult(int result) {
On 2014/06/27 01:35:55, hashimoto wrote:
> On 2014/06/20 01:16:20, mtomasz wrote:
> > On 2014/06/19 06:45:06, hashimoto wrote:
> > > On 2014/06/16 03:29:36, mtomasz wrote:
> > > > On 2014/06/13 06:49:56, hashimoto wrote:
> > > > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > > > Why you can't just EXPECT_FALSE(have_result()) instead of
counting?
> > > > > > 
> > > > > > 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 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.
> > > > 
> > > > 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.
> > > 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;
> > > };
> > 
> > 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
> What you need is only one template function.
> > roughly and it would make your solution grow from 5 to 21 lines (while my
> logger
> > class is 40 lines).
> > 
> > > 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.
> > 
> > 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.
> > 
> > > > 
> > > > 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
> > > 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.
> > 
> > Do you suggest that we replace RoonLoop::RunUntilIdle() with RunLoop::Run()
in
> > all our unit tests then?
> No.
> I just meant that neither RunUntilIdle() nor Run() can handle wrongly behaving
> classes perfectly.
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.
> > 
> > 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.
> > 
> > > > than it should. Since we agree on using Logger, let's just go with the
> > logger,
> > > > even if it is not the best solution.
> > > 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.
> > > > 
> > > > > > 
> > > > > > 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
> > > > > 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.
> > > > > > size is dynamically calculated, instead of always set to 512KB.
> > > > > 
> > > > 
> > > 
> > 
> 
> 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.
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.
> 
> Even if TestCompletionCallback is not the right thing to be used here, the
> design of Logger here is far from the perfection.
I'm not sure if spending too much time on making code *perfect* is the best use
of our engineering time.
> 1. Logger is repeating everything 3 times for 3 vectors.
How would you like to reduce it in the Logger class?
> 2. Logger is mixing up two different types of information: arguments passed to
> the inner reader and results returned from BufferingFileStreamReader.
What's wrong about it? Logger class logs activities happened in a test case.
> 3. Logger doesn't provide a convenient function like
> TestCompletionCallback::callback() with which the test body doesn't have to
> repeat lengthy Bind() expressions.
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.
> 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.
What's wrong about it? How would you check values returned by a callback without
checking that the callback is called?
> 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.
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.
> 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]);
>   ...
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.
> 
> I recommend you to use the existing well-written class TestCompletionCallback,
> or at least polish this class's design.

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.

[mtomasz, 2014-06-27 05:29:25]

On 2014/06/27 05:27:54, mtomasz wrote:
>
https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
> File
>
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/334593003/diff/20001/chrome/browser/chromeos/...
>
chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:51:
> void SetResult(int result) {
> On 2014/06/27 01:35:55, hashimoto wrote:
> > On 2014/06/20 01:16:20, mtomasz wrote:
> > > On 2014/06/19 06:45:06, hashimoto wrote:
> > > > On 2014/06/16 03:29:36, mtomasz wrote:
> > > > > On 2014/06/13 06:49:56, hashimoto wrote:
> > > > > > On 2014/06/13 00:32:51, mtomasz wrote:
> > > > > > > On 2014/06/12 12:22:06, hashimoto wrote:
> > > > > > > > Why you can't just EXPECT_FALSE(have_result()) instead of
> counting?
> > > > > > > 
> > > > > > > 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 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.
> > > > > 
> > > > > 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.
> > > > 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;
> > > > };
> > > 
> > > 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
> > What you need is only one template function.
> > > roughly and it would make your solution grow from 5 to 21 lines (while my
> > logger
> > > class is 40 lines).
> > > 
> > > > 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.
> > > 
> > > 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.
> > > 
> > > > > 
> > > > > 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
> > > > 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.
> > > 
> > > Do you suggest that we replace RoonLoop::RunUntilIdle() with
RunLoop::Run()
> in
> > > all our unit tests then?
> > No.
> > I just meant that neither RunUntilIdle() nor Run() can handle wrongly
behaving
> > classes perfectly.
> 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.
> > > 
> > > 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.
> > > 
> > > > > than it should. Since we agree on using Logger, let's just go with the
> > > logger,
> > > > > even if it is not the best solution.
> > > > 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.
> > > > > 
> > > > > > > 
> > > > > > > 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
> > > > > > 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.
> > > > > > > size is dynamically calculated, instead of always set to 512KB.
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> > 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.
> 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.
> > 
> > Even if TestCompletionCallback is not the right thing to be used here, the
> > design of Logger here is far from the perfection.
> I'm not sure if spending too much time on making code *perfect* is the best
use
> of our engineering time.
> > 1. Logger is repeating everything 3 times for 3 vectors.
> How would you like to reduce it in the Logger class?
> > 2. Logger is mixing up two different types of information: arguments passed
to
> > the inner reader and results returned from BufferingFileStreamReader.
> What's wrong about it? Logger class logs activities happened in a test case.
> > 3. Logger doesn't provide a convenient function like
> > TestCompletionCallback::callback() with which the test body doesn't have to
> > repeat lengthy Bind() expressions.
> 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.
> > 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.
> What's wrong about it? How would you check values returned by a callback
without
> checking that the callback is called?
> > 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.
> 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.
> > 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]);
> >   ...
> 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.
> > 
> > I recommend you to use the existing well-written class
TestCompletionCallback,
> > or at least polish this class's design.
> 
> 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.

+kinaba@

[hashimoto, 2014-07-02 09:45:19]

I'm not asking you to write perfect code, but, as a paid worker, I hesitate to
give an approval for code whose maintainability is questionable.
At this point I'm not sure if it's possible to share my concern with you, but
let's have a try.

First, if you use TestCompletionCallback, there is no reason to write a new
"logger" class at all.
It can be replaced with TestCompletionCallback and std::vector<int>.
FakeFileStreamReader will take a pointer of std::vector<int> to record method
arguments.
The code should look like:

 std::vector<int> inner_read_requested_events;
 BufferingFileStreamReader reader(
     scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
         &inner_read_requested_events, GetParam(), false)), kBufferSize);
 ...
 net::TestCompletionCallback callback;
 EXPECT_EQ(kBufferSize, callback.GetResult(reader.Read(buffer, kChunkSize,
callback.callback())));
 ASSERT_EQ(1u, inner_read_requested_events().size());
 EXPECT_EQ(kBufferSize, inner_read_requested_events()[0]);


But, if you think you cannot use TestCompletionCallback here for a reason I
don't quite understand, you can have an imitation of TestCompletionCallback
which uses RunUntilIdle() and checks for multiple callback calls.
The code should look like (please forgive me if it doesn't compile):

 template<typename T>
 class MyTestCompletionCallback {
  public:
   net::CompletionCallback callback() {
     results_.clear();
     return base::Bind(&MyTestCompletionCallback::SetResult,
base::Unretained(this));
   }
   void ExpectResult(T expected_result, T result) {
     if (result != net::ERR_IO_PENDING) {
       EXPECT_EQ(expected_result, result);
       return;
     }
     base::RunLoop::RunUntilIdle();
     ASSERT_EQ(1U, results_.size());
     EXPECT_EQ(expected_result, results_[0]);
   }

  private:
   void SetResult(T result) { results_.push_back(result); }
   std::vector<T> results_;
 };

 ...

 std::vector<int> inner_read_requested_events;
 BufferingFileStreamReader reader(
     scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
         &inner_read_requested_events, GetParam(), false)), kBufferSize);
 ...
 MyTestCompletionCallback<int> callback;
 EXPECT_NO_FATAL_FAILURE(callback.ExpectResult(kBufferSize, reader.Read(buffer,
kChunkSize, callback.callback())));
 ASSERT_EQ(1u, inner_read_requested_events().size());
 EXPECT_EQ(kBufferSize, inner_read_requested_events()[0]);


This way, you don't have to repeat everything 3 times, write Bind everywhere,
nor check if result buffer's size is 1 every time.
There is also a bonus that you can replace "ASSERT_EQ(1U, results_size());" with
code which prints the vector's contents when the vector's size is different from
1.

[mtomasz, 2014-07-02 13:54:37]

On 2014/07/02 09:45:19, hashimoto wrote:
> I'm not asking you to write perfect code, but, as a paid worker, I hesitate to
> give an approval for code whose maintainability is questionable.
>
> At this point I'm not sure if it's possible to share my concern with you, but
> let's have a try.

Please don't get me wrong. I appreciate in depth code reviews and I'm always
open to listen and change my code. However I feel like you are ignoring my
arguments.
> 
> First, if you use TestCompletionCallback, there is no reason to write a new
> "logger" class at all.
> It can be replaced with TestCompletionCallback and std::vector<int>.
> FakeFileStreamReader will take a pointer of std::vector<int> to record method
> arguments.
> The code should look like:
> 
>  std::vector<int> inner_read_requested_events;
>  BufferingFileStreamReader reader(
>      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
>          &inner_read_requested_events, GetParam(), false)), kBufferSize);
>  ...
>  net::TestCompletionCallback callback;
>  EXPECT_EQ(kBufferSize, callback.GetResult(reader.Read(buffer, kChunkSize,
> callback.callback())));
>  ASSERT_EQ(1u, inner_read_requested_events().size());
>  EXPECT_EQ(kBufferSize, inner_read_requested_events()[0]);
> 
> 
> But, if you think you cannot use TestCompletionCallback here for a reason I
> don't quite understand, you can have an imitation of TestCompletionCallback
> which uses RunUntilIdle() and checks for multiple callback calls.
> The code should look like (please forgive me if it doesn't compile):
> 
>  template<typename T>
>  class MyTestCompletionCallback {
>   public:
>    net::CompletionCallback callback() {
>      results_.clear();
>      return base::Bind(&MyTestCompletionCallback::SetResult,
> base::Unretained(this));
>    }
>    void ExpectResult(T expected_result, T result) {
>      if (result != net::ERR_IO_PENDING) {
>        EXPECT_EQ(expected_result, result);
>        return;
>      }
>      base::RunLoop::RunUntilIdle();
>      ASSERT_EQ(1U, results_.size());
>      EXPECT_EQ(expected_result, results_[0]);
>    }
> 
>   private:
>    void SetResult(T result) { results_.push_back(result); }
>    std::vector<T> results_;
>  };
> 
>  ...
> 
>  std::vector<int> inner_read_requested_events;
>  BufferingFileStreamReader reader(
>      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
>          &inner_read_requested_events, GetParam(), false)), kBufferSize);
>  ...
>  MyTestCompletionCallback<int> callback;
>  EXPECT_NO_FATAL_FAILURE(callback.ExpectResult(kBufferSize,
reader.Read(buffer,
> kChunkSize, callback.callback())));
>  ASSERT_EQ(1u, inner_read_requested_events().size());
>  EXPECT_EQ(kBufferSize, inner_read_requested_events()[0]);
> 
> 
> This way, you don't have to repeat everything 3 times, write Bind everywhere,
> nor check if result buffer's size is 1 every time.
> There is also a bonus that you can replace "ASSERT_EQ(1U, results_size());"
with
> code which prints the vector's contents when the vector's size is different
from
> 1.

The code looks cool, but it also looks also complex.

Instead of a Bind, you need EXPECT_NO_FATAL_FAILURE for every check. Trivial
EXPECT_EQ, become longish ExpectResult calls. Calling callback.callback() is
confusing. Note that MyTestCompletionCallback is actually not a callback. Your
example code is shorter in lines, but look how long your ExpectResult line is.
Inner reader's values you are checking in the test body, while the outer reader
via ExpectResult. This is slightly inconsistent.

Your code doesn't catch unintentional callback calls in some cases. If Read()
returns a value != ERR_IO_PENDING, and calls the callback, then the test will
pass but it shouldn't.

Saying that, I'm really not convinced that your suggested code is more
maintainable, since it's basically more complex (templates,
EXPECT_NO_FATAL_FAILURE, ExpectResult, expects in two different places).

It seem that you prefer shorter code, and I prefer simpler code (even if it is
longer). I think there is no clean winner here. I'd like to hear @kinaba's
opinion.

[kinaba, 2014-07-02 14:43:07]

I'm sorry I took so long for catching up the discussion...

My main concern on the Logger class is that it is packing many stuff
that can be separated into one class. That is, (1) the three member
variables are orthogonal that really don't need to be in a same class
(2) logs for multiple outer Read() are logged into the same vectors that
is decreasing the readability.

Shortcoming of TestCompletionCallback is that it doesn't gracefully handle
bugs on calling back 0 or 2 or more times. (For this reason I believe you
two have already agreed that we cannot use it for inner reads.)
For outer reads, I think the freezing tests for 0-callback case is fine as
long as it always (non-flakily) freezes, but if we do want to catch faulty
multiple callback invocations, I agree we need something.

Since it is a common mistake to wrongly invoking callbacks multiple times,
if the writer (Tomasz) wants to cover such bugs in tests, I'd like to respect
the direction.


So, what about this.

* For inner reads, I think the implementation in the Hashimoto's latest comment
that just uses std::vector<int> without any callbacks looks the cleanest.

* For outer reads, adding one template function like the follwing:

template<typename T>
void LogArgument(std::vector<int>* log, T arg) {log->push_back(arg)};

{  // testing single Read() call
  std::vector<int> read_events;
  int result = reader.Read(buffer, kChunkSize, base::Bind(&LogArgument<int>,
&read_events));
  base::RunLoop().RunUntilIdle();
  EXPECT(whatever, you, want);
}

* No EXPECT/ASSERT in a common utility function; the point of failure is clear
from the log.
* No three-times duplicated code in the Logger class.
* Able to catch 0-callback or 2-callback errors as ASSERTions.
* Each {...testing single Read()...} block can have distinct |read_events| log,
which means,
 you don't need to change expected sizes/indices depending on preceding blocks.
* I hope it's simple enough to justify using 'hand-crafted' utility instead of
resorting to
 existing utilities like TestCompletionCallback.
* No fanciness/trickiness (I hope.)

[mtomasz, 2014-07-02 15:07:10]

On 2014/07/02 14:43:07, kinaba wrote:
> I'm sorry I took so long for catching up the discussion...
> 
> My main concern on the Logger class is that it is packing many stuff
> that can be separated into one class. That is, (1) the three member
> variables are orthogonal that really don't need to be in a same class
> (2) logs for multiple outer Read() are logged into the same vectors that
> is decreasing the readability.
> 
> Shortcoming of TestCompletionCallback is that it doesn't gracefully handle
> bugs on calling back 0 or 2 or more times. (For this reason I believe you
> two have already agreed that we cannot use it for inner reads.)
> For outer reads, I think the freezing tests for 0-callback case is fine as
> long as it always (non-flakily) freezes, but if we do want to catch faulty
> multiple callback invocations, I agree we need something.
> 
> Since it is a common mistake to wrongly invoking callbacks multiple times,
> if the writer (Tomasz) wants to cover such bugs in tests, I'd like to respect
> the direction.
> 
> 
> So, what about this.
> 
> * For inner reads, I think the implementation in the Hashimoto's latest
comment
> that just uses std::vector<int> without any callbacks looks the cleanest.
> 
> * For outer reads, adding one template function like the follwing:
> 
> template<typename T>
> void LogArgument(std::vector<int>* log, T arg) {log->push_back(arg)};
> 
> {  // testing single Read() call
>   std::vector<int> read_events;
>   int result = reader.Read(buffer, kChunkSize, base::Bind(&LogArgument<int>,
> &read_events));
>   base::RunLoop().RunUntilIdle();
>   EXPECT(whatever, you, want);
> }
> 
> * No EXPECT/ASSERT in a common utility function; the point of failure is clear
> from the log.
> * No three-times duplicated code in the Logger class.
> * Able to catch 0-callback or 2-callback errors as ASSERTions.
> * Each {...testing single Read()...} block can have distinct |read_events|
log,
> which means,
>  you don't need to change expected sizes/indices depending on preceding
blocks.
> * I hope it's simple enough to justify using 'hand-crafted' utility instead of
> resorting to
>  existing utilities like TestCompletionCallback.
> * No fanciness/trickiness (I hope.)

Sounds reasonable to me. If @hashimoto is fine with this plan, I'll proceed with
changes.

[hashimoto, 2014-07-03 10:31:13]

On 2014/07/02 15:07:10, mtomasz wrote:
> On 2014/07/02 14:43:07, kinaba wrote:
> > I'm sorry I took so long for catching up the discussion...
> > 
> > My main concern on the Logger class is that it is packing many stuff
> > that can be separated into one class. That is, (1) the three member
> > variables are orthogonal that really don't need to be in a same class
> > (2) logs for multiple outer Read() are logged into the same vectors that
> > is decreasing the readability.
> > 
> > Shortcoming of TestCompletionCallback is that it doesn't gracefully handle
> > bugs on calling back 0 or 2 or more times. (For this reason I believe you
> > two have already agreed that we cannot use it for inner reads.)
> > For outer reads, I think the freezing tests for 0-callback case is fine as
> > long as it always (non-flakily) freezes, but if we do want to catch faulty
> > multiple callback invocations, I agree we need something.
> > 
> > Since it is a common mistake to wrongly invoking callbacks multiple times,
> > if the writer (Tomasz) wants to cover such bugs in tests, I'd like to
respect
> > the direction.
> > 
> > 
> > So, what about this.
> > 
> > * For inner reads, I think the implementation in the Hashimoto's latest
> comment
> > that just uses std::vector<int> without any callbacks looks the cleanest.
> > 
> > * For outer reads, adding one template function like the follwing:
> > 
> > template<typename T>
> > void LogArgument(std::vector<int>* log, T arg) {log->push_back(arg)};
> > 
> > {  // testing single Read() call
> >   std::vector<int> read_events;
> >   int result = reader.Read(buffer, kChunkSize, base::Bind(&LogArgument<int>,
> > &read_events));
> >   base::RunLoop().RunUntilIdle();
> >   EXPECT(whatever, you, want);
> > }
> > 
> > * No EXPECT/ASSERT in a common utility function; the point of failure is
clear
> > from the log.
> > * No three-times duplicated code in the Logger class.
> > * Able to catch 0-callback or 2-callback errors as ASSERTions.
> > * Each {...testing single Read()...} block can have distinct |read_events|
> log,
> > which means,
> >  you don't need to change expected sizes/indices depending on preceding
> blocks.
> > * I hope it's simple enough to justify using 'hand-crafted' utility instead
of
> > resorting to
> >  existing utilities like TestCompletionCallback.
> > * No fanciness/trickiness (I hope.)
> 
> Sounds reasonable to me. If @hashimoto is fine with this plan, I'll proceed
with
> changes.

Sounds reasonable.
Glad to hear that you are now OK with the vector+function plan I proposed 4
weeks ago.

[mtomasz, 2014-07-03 11:21:21]

On 2014/07/03 10:31:13, hashimoto wrote:
> On 2014/07/02 15:07:10, mtomasz wrote:
> > On 2014/07/02 14:43:07, kinaba wrote:
> > > I'm sorry I took so long for catching up the discussion...
> > > 
> > > My main concern on the Logger class is that it is packing many stuff
> > > that can be separated into one class. That is, (1) the three member
> > > variables are orthogonal that really don't need to be in a same class
> > > (2) logs for multiple outer Read() are logged into the same vectors that
> > > is decreasing the readability.
> > > 
> > > Shortcoming of TestCompletionCallback is that it doesn't gracefully handle
> > > bugs on calling back 0 or 2 or more times. (For this reason I believe you
> > > two have already agreed that we cannot use it for inner reads.)
> > > For outer reads, I think the freezing tests for 0-callback case is fine as
> > > long as it always (non-flakily) freezes, but if we do want to catch faulty
> > > multiple callback invocations, I agree we need something.
> > > 
> > > Since it is a common mistake to wrongly invoking callbacks multiple times,
> > > if the writer (Tomasz) wants to cover such bugs in tests, I'd like to
> respect
> > > the direction.
> > > 
> > > 
> > > So, what about this.
> > > 
> > > * For inner reads, I think the implementation in the Hashimoto's latest
> > comment
> > > that just uses std::vector<int> without any callbacks looks the cleanest.
> > > 
> > > * For outer reads, adding one template function like the follwing:
> > > 
> > > template<typename T>
> > > void LogArgument(std::vector<int>* log, T arg) {log->push_back(arg)};
> > > 
> > > {  // testing single Read() call
> > >   std::vector<int> read_events;
> > >   int result = reader.Read(buffer, kChunkSize,
base::Bind(&LogArgument<int>,
> > > &read_events));
> > >   base::RunLoop().RunUntilIdle();
> > >   EXPECT(whatever, you, want);
> > > }
> > > 
> > > * No EXPECT/ASSERT in a common utility function; the point of failure is
> clear
> > > from the log.
> > > * No three-times duplicated code in the Logger class.
> > > * Able to catch 0-callback or 2-callback errors as ASSERTions.
> > > * Each {...testing single Read()...} block can have distinct |read_events|
> > log,
> > > which means,
> > >  you don't need to change expected sizes/indices depending on preceding
> > blocks.
> > > * I hope it's simple enough to justify using 'hand-crafted' utility
instead
> of
> > > resorting to
> > >  existing utilities like TestCompletionCallback.
> > > * No fanciness/trickiness (I hope.)
> > 
> > Sounds reasonable to me. If @hashimoto is fine with this plan, I'll proceed
> with
> > changes.
> 
> Sounds reasonable.
> Glad to hear that you are now OK with the vector+function plan I proposed 4
> weeks ago.

I was once fine [1], but then you started insisting on using
TestCompletionCallback. You also mentioned, that you are fine with the Logger
if I maintain it, then you changed your mind. When I asked you to clarify what
you want me to fix in the Logger, you again started forcing a custom
TestCompletionCallback solution, ignoring my questions.

That's why I am very confused what exactly you want me to do.

Anyway, I'm very happy we finally got an agreement on this CL.

[1]
> I can split the class to separate utility methods like you suggested before,
> and declare vectors locally. That would however end up on declaring the same
> vectors in several test cases. I'm not convinced it is more readable. But
> let's give it a try.

[kinaba, 2014-08-11 00:16:21]

Can this issue be closed?

[chromium-reviews, 2014-08-11 00:20:30]

Closed.


On 11 August 2014 09:16, <kinaba@chromium.org> wrote:

> Can this issue be closed?
>
> https://codereview.chromium.org/334593003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.