Replace voe_auto_test

Remove voe_auto_test and add new tests to cover the missing cases.

BUG=webrtc:4690
Review-Url: https://codereview.webrtc.org/3007383002
Cr-Commit-Position: refs/heads/master@{#19865}
Committed: https://webrtc.googlesource.com/src/+/18f5427e4c21abb3fe74fbee158f3cba5c1b0127

[the sun, 2017-09-12 12:25:50]

Description was changed from

==========
voe_auto_test cleanup (part 1)

BUG=webrtc:4690
==========

to

==========
Remove voe_auto_test and add new tests to cover the missing cases.

BUG=webrtc:4690
==========

[the sun, 2017-09-12 12:25:50]

solenberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[the sun, 2017-09-13 14:16:30]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[the sun, 2017-09-13 14:16:58]

On 2017/09/13 14:15:54, the sun wrote:

Added kwiberg@ for webrtc/test/ and BUILD.gn.

[kwiberg-webrtc, 2017-09-13 14:25:35]

On 2017/09/13 14:16:58, the sun wrote:
> On 2017/09/13 14:15:54, the sun wrote:
> 
> Added kwiberg@ for webrtc/test/ and BUILD.gn.

lgtm for webrtc/test/ and webrtc/BUILD.gn.

[ossu, 2017-09-14 11:51:58]

Nice work! Looks like we're getting rid of quite a lot of fixtures that were
used for just a few tests.

I've a few questions, though...

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:41: CNameObserver() : AudioSendTest() {}
Will CNameObserver() = default; do?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:83:
EXPECT_TRUE(header.extension.hasAudioLevel);
The old tests included one that checked that no audio level extension was
present if it hadn't been enabled. Is it worth adding that test here as well, or
is that implementation to straightforward to warrant it?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:88: LOG(LS_WARNING) << "Got a packet
with zero audioLevel - waiting"
When can this happen? Where does the audio come from?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:111: TEST_F(AudioSendStreamCallTest,
SupportsTransportWideSequenceNumbers) {
Is this test new? It looks like there are some seeds for testing hasAbsoluteSend
time in the old code, but nothing more. (and this one tests the opposite)

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:153: static const int kDtmfEventFirst =
12;
It looks like this differs from the ranges tested before. Does that matter? Or
are the input ranges to the functions different?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:158: DtmfObserver() : AudioSendTest() {}
= default here as well?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:168: if (expected_dtmf_event_ == event)
{
Will you want to ensure no other DTMF events are being sent as well? To ensure
they're not mistakenly sent twice, for example.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
File webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc (left):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc:32: TEST_F(DtmfTest,
TestTwoNonDtmfEvents) {
Is this no longer supported? Should it be tested for as well? Am I just not
seeing it? :)

[the sun, 2017-09-14 12:36:40]

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:41: CNameObserver() : AudioSendTest() {}
On 2017/09/14 11:51:58, ossu wrote:
> Will CNameObserver() = default; do?

Done.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:83:
EXPECT_TRUE(header.extension.hasAudioLevel);
On 2017/09/14 11:51:58, ossu wrote:
> The old tests included one that checked that no audio level extension was
> present if it hadn't been enabled. Is it worth adding that test here as well,
or
> is that implementation to straightforward to warrant it?

Done.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:88: LOG(LS_WARNING) << "Got a packet
with zero audioLevel - waiting"
On 2017/09/14 11:51:58, ossu wrote:
> When can this happen? Where does the audio come from?

The test signal is a periodic noise burst.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:111: TEST_F(AudioSendStreamCallTest,
SupportsTransportWideSequenceNumbers) {
On 2017/09/14 11:51:58, ossu wrote:
> Is this test new? It looks like there are some seeds for testing
hasAbsoluteSend
> time in the old code, but nothing more. (and this one tests the opposite)

AbsoluteSendTime is no longer used. There was a leftover in the old test, but
the test only looked at audioLevel. TransportSequenceNumbers is used though, and
we were lacking a test for it.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:153: static const int kDtmfEventFirst =
12;
On 2017/09/14 11:51:58, ossu wrote:
> It looks like this differs from the ranges tested before. Does that matter? Or
> are the input ranges to the functions different?

It doesn't matter. The point is to test that events are queued up and then sent
out.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:158: DtmfObserver() : AudioSendTest() {}
On 2017/09/14 11:51:57, ossu wrote:
> = default here as well?

Done.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:168: if (expected_dtmf_event_ == event)
{
On 2017/09/14 11:51:58, ossu wrote:
> Will you want to ensure no other DTMF events are being sent as well? To ensure
> they're not mistakenly sent twice, for example. 

In part, we're already testing this since for each packet we expect a different
event. I've increased the event length to slightly increase the likelihood that
there could be such a problem.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
File webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc (left):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc:32: TEST_F(DtmfTest,
TestTwoNonDtmfEvents) {
On 2017/09/14 11:51:57, ossu wrote:
> Is this no longer supported? Should it be tested for as well? Am I just not
> seeing it? :)

Anything >16 counts as "non-DTMF", so in practice it is already tested.

[ossu, 2017-09-14 12:51:32]

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:88: LOG(LS_WARNING) << "Got a packet
with zero audioLevel - waiting"
On 2017/09/14 12:36:39, the sun wrote:
> On 2017/09/14 11:51:58, ossu wrote:
> > When can this happen? Where does the audio come from?
> 
> The test signal is a periodic noise burst.

Alright. I was thinking if this would often get logged, then maybe it's benign
enough to not log at all and if it wouldn't, then it should maybe be more than
just a log message. I'm not going to insist either way.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:111: TEST_F(AudioSendStreamCallTest,
SupportsTransportWideSequenceNumbers) {
On 2017/09/14 12:36:39, the sun wrote:
> On 2017/09/14 11:51:58, ossu wrote:
> > Is this test new? It looks like there are some seeds for testing
> hasAbsoluteSend
> > time in the old code, but nothing more. (and this one tests the opposite)
> 
> AbsoluteSendTime is no longer used. There was a leftover in the old test, but
> the test only looked at audioLevel. TransportSequenceNumbers is used though,
and
> we were lacking a test for it.

Acknowledged.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:153: static const int kDtmfEventFirst =
12;
On 2017/09/14 12:36:39, the sun wrote:
> On 2017/09/14 11:51:58, ossu wrote:
> > It looks like this differs from the ranges tested before. Does that matter?
Or
> > are the input ranges to the functions different?
> 
> It doesn't matter. The point is to test that events are queued up and then
sent
> out.

Acknowledged.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:168: if (expected_dtmf_event_ == event)
{
On 2017/09/14 12:36:40, the sun wrote:
> On 2017/09/14 11:51:58, ossu wrote:
> > Will you want to ensure no other DTMF events are being sent as well? To
ensure
> > they're not mistakenly sent twice, for example. 
> 
> In part, we're already testing this since for each packet we expect a
different
> event. I've increased the event length to slightly increase the likelihood
that
> there could be such a problem.

Why not EXPECT_EQ(expected_dtmf_event_, event)? Right now it'll silently ignore
any "extra" events that may be generated, but perhaps that's intended?

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
File webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc (left):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/voice_engine/test/...
webrtc/voice_engine/test/auto_test/standard/dtmf_test.cc:32: TEST_F(DtmfTest,
TestTwoNonDtmfEvents) {
On 2017/09/14 12:36:39, the sun wrote:
> On 2017/09/14 11:51:57, ossu wrote:
> > Is this no longer supported? Should it be tested for as well? Am I just not
> > seeing it? :)
> 
> Anything >16 counts as "non-DTMF", so in practice it is already tested.

Ah, alright.

[the sun, 2017-09-14 12:53:04]

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

[commit-bot: I haz the power, 2017-09-14 12:53:10]

Dry run: 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-09-14 14:48:21]

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

[commit-bot: I haz the power, 2017-09-14 14:48:22]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-09-14 20:05:06]

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:88: LOG(LS_WARNING) << "Got a packet
with zero audioLevel - waiting"
On 2017/09/14 12:51:32, ossu wrote:
> On 2017/09/14 12:36:39, the sun wrote:
> > On 2017/09/14 11:51:58, ossu wrote:
> > > When can this happen? Where does the audio come from?
> > 
> > The test signal is a periodic noise burst.
> 
> Alright. I was thinking if this would often get logged, then maybe it's benign
> enough to not log at all and if it wouldn't, then it should maybe be more than
> just a log message. I'm not going to insist either way.

It is benign as long as the test eventually completes. If it times out, that
means we got no packages with >0 level, and then the log lines will be helpful.

https://codereview.webrtc.org/3007383002/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_tests.cc:168: if (expected_dtmf_event_ == event)
{
On 2017/09/14 12:51:32, ossu wrote:
> On 2017/09/14 12:36:40, the sun wrote:
> > On 2017/09/14 11:51:58, ossu wrote:
> > > Will you want to ensure no other DTMF events are being sent as well? To
> ensure
> > > they're not mistakenly sent twice, for example. 
> > 
> > In part, we're already testing this since for each packet we expect a
> different
> > event. I've increased the event length to slightly increase the likelihood
> that
> > there could be such a problem.
> 
> Why not EXPECT_EQ(expected_dtmf_event_, event)? Right now it'll silently
ignore
> any "extra" events that may be generated, but perhaps that's intended?

Well, AFAIU (RFC 4733) extra events are perfectly fine for the sender to
generate - the event duration is relative to the RTP timestamp of the packet and
may even increase for ongoing events (it is still regarded as a single event if
the start timestamp is the same). The RtpSenderAudio implementation periodically
sends events for the given duration of 50ms, presumably as a measure against
packet loss.

I've changed the test to better reflect these expectations.

[the sun, 2017-09-15 11:38:59]

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

[commit-bot: I haz the power, 2017-09-15 11:39:02]

Dry run: 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-09-15 12:05:52]

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

[commit-bot: I haz the power, 2017-09-15 12:05:54]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-09-15 12:20:22]

https://codereview.webrtc.org/3007383002/diff/120001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/120001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream_tests.cc:208: } else {
So, now we only advance the expected dtmf event if we're no longer getting the
one we previously expected. This vibes with your explanation of the RFC (I
didn't know it could do that).

However, now if we get kDtmfEventLast only once, we'll never trigger
observation_complete_. Perhaps invert the check:
if(event != expected_dtmf_event_) {
/* advance and EXPECT_EQ */
}
if (event == kDtmfEventLast) observation_complete_.Set();

[the sun, 2017-09-15 13:08:26]

https://codereview.webrtc.org/3007383002/diff/120001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream_tests.cc (right):

https://codereview.webrtc.org/3007383002/diff/120001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream_tests.cc:208: } else {
On 2017/09/15 12:20:21, ossu wrote:
> So, now we only advance the expected dtmf event if we're no longer getting the
> one we previously expected. This vibes with your explanation of the RFC (I
> didn't know it could do that).
> 
> However, now if we get kDtmfEventLast only once, we'll never trigger
> observation_complete_. Perhaps invert the check:
> if(event != expected_dtmf_event_) {
> /* advance and EXPECT_EQ */
> }
> if (event == kDtmfEventLast) observation_complete_.Set();

Yes. Good catch!

[ossu, 2017-09-15 13:09:23]

lgtm!

[the sun, 2017-09-15 13:09:56]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-09-15 13:09:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3007383002/#ps170001 (title:
"reviewer comment")

[commit-bot: I haz the power, 2017-09-15 13:10:03]

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-09-15 15:10:32]

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

[commit-bot: I haz the power, 2017-09-15 15:10:33]

Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))

[the sun, 2017-09-15 16:53:06]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-09-15 16:53:14]

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-09-15 16:56:09]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1505494386988010,
"parent_rev": "8b891cecf793f34c186e35872ba85f19388d201b", "commit_rev":
"18f5427e4c21abb3fe74fbee158f3cba5c1b0127"}

[commit-bot: I haz the power, 2017-09-15 16:56:15]

Description was changed from

==========
Remove voe_auto_test and add new tests to cover the missing cases.

BUG=webrtc:4690
==========

to

==========
Remove voe_auto_test and add new tests to cover the missing cases.

BUG=webrtc:4690

Review-Url: https://codereview.webrtc.org/3007383002
Cr-Commit-Position: refs/heads/master@{#19865}
Committed:
https://webrtc.googlesource.com/src/+/18f5427e4c21abb3fe74fbee158f3cba5c1b0127
==========

[commit-bot: I haz the power, 2017-09-15 16:56:17]

Committed patchset #10 (id:170001) as
https://webrtc.googlesource.com/src/+/18f5427e4c21abb3fe74fbee158f3cba5c1b0127