Move AudioDecoder and AudioDecoderFactory mocks to webrtc/test/

Move AudioDecoder and AudioDecoderFactory mocks to webrtc/test/

AudioDecoder and AudioDecoderFactory are in webrtc/api/ now, so move
their mocks to someplace central where tests from all over WebRTC are
allowed to #include them.

BUG=webrtc:5805
Review-Url: https://codereview.webrtc.org/2798063004
Cr-Commit-Position: refs/heads/master@{#17619}
Committed: https://chromium.googlesource.com/external/webrtc/+/37e99fd3fa4d49b87a58a58e9605af01cdb45d16

[kwiberg-webrtc, 2017-04-06 12:29:38]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 12:29:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-04-06 12:33:01]

kwiberg@webrtc.org changed reviewers:
+ kjellander@webrtc.org, ossu@webrtc.org

[kwiberg-webrtc, 2017-04-06 12:33:02]

We first considered moving the mocks to someplace under webrtc/api/, but decided
that reducing API surface was worth more than sharing the mocks with any
out-of-tree tests. This could be revisited if someone felt like cleaning up the
mocks.

[kjellander_webrtc, 2017-04-06 12:48:20]

I thought we were going to keep mocks like these in
https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?

Is there any advantage by moving them here?

[kjellander_webrtc, 2017-04-06 12:51:12]

Sorry, I apparently only read the CL description and missed your message. This
is fine with me. 

Can you notify aleloi@ for moving mock_audio_mixer.h and hbos@ for moving
mock_rtpreceiver.h + mock_rtpsender.h?
For fakeconstraints.h I don't know.

lgtm

[the sun, 2017-04-06 12:53:18]

On 2017/04/06 12:48:20, kjellander_webrtc wrote:
> I thought we were going to keep mocks like these in
> https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?

That's what I thought as well. The mocks are potentially useful for API clients
so we might as well provide them.

> Is there any advantage by moving them here?

One disadvantage is that we'll ultimately have a mock of everything in
webrtc/test/, and to not slow down work it will need to be owned by everyone as
well. I already find the latter a problem - whenever I change an interface+mock
I need to get approval from a test/ owner, so I'm tempted to sign up as owner,
but would rather not...

[commit-bot: I haz the power, 2017-04-06 12:53:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 12:53:33]

Dry run: Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/23648)

[ossu, 2017-04-06 13:03:29]

On 2017/04/06 12:53:18, the sun wrote:
> On 2017/04/06 12:48:20, kjellander_webrtc wrote:
> > I thought we were going to keep mocks like these in
> > https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?
> 
> That's what I thought as well. The mocks are potentially useful for API
clients
> so we might as well provide them.
> 

kwiberg@ and I discussed this earlier today and came to the conclusion that, as
it stands right now, the mocks contain too much stuff that are tailored to our
own unit tests. Were they "clean" mocks, they'd be able to go into
api/<something>/mocks/ directly.

[kwiberg-webrtc, 2017-04-06 13:12:37]

On 2017/04/06 12:53:18, the sun wrote:
> On 2017/04/06 12:48:20, kjellander_webrtc wrote:
> > I thought we were going to keep mocks like these in
> > https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?
> 
> That's what I thought as well. The mocks are potentially useful for API
clients
> so we might as well provide them.

That's what I figured too, initially, but then I had a look at the two files
moved in this CL. MockAudioDecoder has a Die method called by the destructor
(useful for testing lifetime management), MockAudioDecoderFactory has a method
with a move-only return value that needs special handling and two factory
functions that create mocks with specific sets of expectations. In short, our
mocks are polluted with implementation details from our tests. I wouldn't want
to put these in the api/ directory without cleaning them up first---and then our
current tests would need additional utilities to be able to use them. It would
be a bunch of extra complexity (mostly in the form of indirections) for very
little gain, since we don't actually know of any API clients that would use
them.

I think it's a much better compromise to simply not expose our mocks. A clean
mock is trivial to write (~1 line per method, pretty much completely determined
by the method signature), so we should keep our mocks to ourselves, let our
users write their own mocks if they need them, and enjoy the resulting reduced
API surface.

> > Is there any advantage by moving them here?
> 
> One disadvantage is that we'll ultimately have a mock of everything in
> webrtc/test/, and to not slow down work it will need to be owned by everyone
as
> well. I already find the latter a problem - whenever I change an
interface+mock
> I need to get approval from a test/ owner, so I'm tempted to sign up as owner,
> but would rather not...

I can make subdirectories with different OWNERS if you want. Copy the directory
structure of webrtc/api/, or something else?

[kjellander_webrtc, 2017-04-06 13:44:54]

On 2017/04/06 13:12:37, kwiberg-webrtc wrote:
> On 2017/04/06 12:53:18, the sun wrote:
> > On 2017/04/06 12:48:20, kjellander_webrtc wrote:
> > > I thought we were going to keep mocks like these in
> > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?
> > 
> > That's what I thought as well. The mocks are potentially useful for API
> clients
> > so we might as well provide them.
> 
> That's what I figured too, initially, but then I had a look at the two files
> moved in this CL. MockAudioDecoder has a Die method called by the destructor
> (useful for testing lifetime management), MockAudioDecoderFactory has a method
> with a move-only return value that needs special handling and two factory
> functions that create mocks with specific sets of expectations. In short, our
> mocks are polluted with implementation details from our tests. I wouldn't want
> to put these in the api/ directory without cleaning them up first---and then
our
> current tests would need additional utilities to be able to use them. It would
> be a bunch of extra complexity (mostly in the form of indirections) for very
> little gain, since we don't actually know of any API clients that would use
> them.
> 
> I think it's a much better compromise to simply not expose our mocks. A clean
> mock is trivial to write (~1 line per method, pretty much completely
determined
> by the method signature), so we should keep our mocks to ourselves, let our
> users write their own mocks if they need them, and enjoy the resulting reduced
> API surface.
> 
> > > Is there any advantage by moving them here?
> > 
> > One disadvantage is that we'll ultimately have a mock of everything in
> > webrtc/test/, and to not slow down work it will need to be owned by everyone
> as
> > well. I already find the latter a problem - whenever I change an
> interface+mock
> > I need to get approval from a test/ owner, so I'm tempted to sign up as
owner,
> > but would rather not...
> 
> I can make subdirectories with different OWNERS if you want. Copy the
directory
> structure of webrtc/api/, or something else?

Please not too many subdirectories. We can also have per-file entries in OWNERS,
which isn't too hard unless the files change a lot

[kwiberg-webrtc, 2017-04-06 16:46:54]

On 2017/04/06 13:44:54, kjellander_webrtc wrote:
> On 2017/04/06 13:12:37, kwiberg-webrtc wrote:
> > On 2017/04/06 12:53:18, the sun wrote:
> > > On 2017/04/06 12:48:20, kjellander_webrtc wrote:
> > > > I thought we were going to keep mocks like these in
> > > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/test/ ?
> > > 
> > > That's what I thought as well. The mocks are potentially useful for API
> > clients
> > > so we might as well provide them.
> > 
> > That's what I figured too, initially, but then I had a look at the two files
> > moved in this CL. MockAudioDecoder has a Die method called by the destructor
> > (useful for testing lifetime management), MockAudioDecoderFactory has a
method
> > with a move-only return value that needs special handling and two factory
> > functions that create mocks with specific sets of expectations. In short,
our
> > mocks are polluted with implementation details from our tests. I wouldn't
want
> > to put these in the api/ directory without cleaning them up first---and then
> our
> > current tests would need additional utilities to be able to use them. It
would
> > be a bunch of extra complexity (mostly in the form of indirections) for very
> > little gain, since we don't actually know of any API clients that would use
> > them.
> > 
> > I think it's a much better compromise to simply not expose our mocks. A
clean
> > mock is trivial to write (~1 line per method, pretty much completely
> determined
> > by the method signature), so we should keep our mocks to ourselves, let our
> > users write their own mocks if they need them, and enjoy the resulting
reduced
> > API surface.
> > 
> > > > Is there any advantage by moving them here?
> > > 
> > > One disadvantage is that we'll ultimately have a mock of everything in
> > > webrtc/test/, and to not slow down work it will need to be owned by
everyone
> > as
> > > well. I already find the latter a problem - whenever I change an
> > interface+mock
> > > I need to get approval from a test/ owner, so I'm tempted to sign up as
> owner,
> > > but would rather not...
> > 
> > I can make subdirectories with different OWNERS if you want. Copy the
> directory
> > structure of webrtc/api/, or something else?
> 
> Please not too many subdirectories. We can also have per-file entries in
OWNERS,
> which isn't too hard unless the files change a lot

OK, then I'll just land this as-is, because this CL increases the number of
mock_*.h files in webrtc/test/ from 3 to 5---nowhere near the level where
subdirectories are called for. Since these aren't part of the API, it's easy to
rearrange them once they do grow too numerous. (This was my original motivation
for not making subdirectories.)

Fredrik, are you OK with that? Is the peer pressure strong enough? :-)

[kwiberg-webrtc, 2017-04-10 11:07:49]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-04-10 11:07:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-10 12:15:50]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491822469039370, "parent_rev":
"2fa97fdbe55d7046505332e71fd5b49559b307a1", "commit_rev":
"37e99fd3fa4d49b87a58a58e9605af01cdb45d16"}

[commit-bot: I haz the power, 2017-04-10 12:15:54]

Description was changed from

==========
Move AudioDecoder and AudioDecoderFactory mocks to webrtc/test/

AudioDecoder and AudioDecoderFactory are in webrtc/api/ now, so move
their mocks to someplace central where tests from all over WebRTC are
allowed to #include them.

BUG=webrtc:5805
==========

to

==========
Move AudioDecoder and AudioDecoderFactory mocks to webrtc/test/

AudioDecoder and AudioDecoderFactory are in webrtc/api/ now, so move
their mocks to someplace central where tests from all over WebRTC are
allowed to #include them.

BUG=webrtc:5805

Review-Url: https://codereview.webrtc.org/2798063004
Cr-Commit-Position: refs/heads/master@{#17619}
Committed:
https://chromium.googlesource.com/external/webrtc/+/37e99fd3fa4d49b87a58a58e9...
==========

[commit-bot: I haz the power, 2017-04-10 12:15:55]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/37e99fd3fa4d49b87a58a58e9...