Instrument serial API code to make testing easier.

Instrument serial API code to make testing easier.

After a good lunchtime discussion, I went off to implement some sort
of overriding functionality in ExtensionFunctionDispatcher, so that I
could tell it to instantiate Function' instead of Function when an
extension/app asked for function. I then discovered
ExtensionFunctionDispatcher::OverrideFunction(), which was pretty
much exactly what I wanted.

Using this facility, added a fake SerialOpenFunction that overrides
a new CreateSerialConnection method to instantiate a
FakeEchoSerialConnection. As its name implies, this class pretends
to echo bytes over a serial port.

The practical outcome of this CL is we test more of our code without
having a physical serial device attached to the machine.

BUG=135656
TEST=this lays the ground work for testing of crrev.com/10759004

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145988

[asargent_no_longer_on_chrome, 2012-07-10 23:30:49]

Cool, LGTM.

[Mihai Parparita -not on Chrome, 2012-07-11 00:45:45]

http://codereview.chromium.org/10702138/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/serial/serial_api.h (right):

http://codereview.chromium.org/10702138/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/serial/serial_api.h:53: // Overrideable for
testing.
FWIW, the delegate pattern that Antony and I mentioned would be to move these
two methods into a Delegate inner class, and to default to an implementation
that returned real connections, but to allow it to be swapped out with one that
returns mocks.

The advantage of that over this is that it's more obvious what methods need to
be implemented for testing, instead of a seemingly random subset of all the
virtual methods.

[miket_OOO, 2012-07-11 16:06:10]

> FWIW, the delegate pattern that Antony and I mentioned would be to move these
> two methods into a Delegate inner class, and to default to an implementation
> that returned real connections, but to allow it to be swapped out with one
that
> returns mocks.

Yeah, I was thinking about this on the shuttle home yesterday; I have always
preferred inheritance over composition. Delegate classes often have a higher
cognitive load ("ugh, here's a new thing I have to learn") whereas subclasses
are just the class you already understand with a little bit extra. I recognize
that that statement is entirely subjective, and your point about the "little bit
extra" being strewn through an interface, requiring you to piece together what
is and isn't required, is perfectly valid, too.

If you think the predominant approach in Chromium is the delegate class, I can
easily refactor this code to conform. What do you think?

[Mihai Parparita -not on Chrome, 2012-07-11 16:12:42]

On Wed, Jul 11, 2012 at 9:06 AM, <miket@chromium.org> wrote:

> If you think the predominant approach in Chromium is the delegate class, I
> can
> easily refactor this code to conform. What do you think?
>

Delegate classes are often used to reduce coupling between components (e.g.
between content/ and chrome/), but I see them less often in tests. I think
what you have is fine.

Mihai

[miket_OOO, 2012-07-11 16:25:02]

OK, thanks. So let's let this one lie as-is because it's so
test-centric, and to the extent this pattern gets followed in the
future, we'll make sure at review time that class documentation helps
group test-centric methods in order to address your concerns about
cohesion of functionality.

On Wed, Jul 11, 2012 at 9:12 AM, Mihai Parparita <mihaip@chromium.org> wrote:
> On Wed, Jul 11, 2012 at 9:06 AM, <miket@chromium.org> wrote:
>>
>> If you think the predominant approach in Chromium is the delegate class, I
>> can
>> easily refactor this code to conform. What do you think?
>
>
> Delegate classes are often used to reduce coupling between components (e.g.
> between content/ and chrome/), but I see them less often in tests. I think
> what you have is fine.
>
> Mihai



-- 
@sowbug