Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1257583011:
Adding unittests to AudioConferenceMixer. (Closed)
|
Created:
5 years, 4 months ago by minyue-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding unittests to AudioConferenceMixer.
Unit tests around AudioConferenceMixer was severely missing. This CL is to add some tests.
BUG=
R=ajm@chromium.org, andrew@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/22c2729607964aa38d6cb4e521994453b6a271c4
Patch Set 1 : #
Total comments: 35
Patch Set 2 : addressing comments #Patch Set 3 : one more update #Patch Set 4 : after rebase #
Messages
Total messages: 16 (7 generated)
Patchset #1 (id:1) has been deleted
minyue@webrtc.org changed reviewers: + andrew@webrtc.org
Patchset #1 (id:20001) has been deleted
Hi Andrew, Would you take look at this? If you have other suggested unit tests, I will be happy to add them.
ajm@chromium.org changed reviewers: + ajm@chromium.org
Thanks Minyue. Before I start reviewing, just want to be clear: did you copy some of this from elsewhere, or is it written from scratch? https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.... webrtc/modules/modules.gyp:162: 'audio_conference_mixer/test/audio_conference_mixer_unittest.cc', nit: It's more typical to put the unit test alongside the production code.
On 2015/08/04 21:09:03, ajm wrote: > Thanks Minyue. Before I start reviewing, just want to be clear: did you copy > some of this from elsewhere, or is it written from scratch? > > https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.gyp > File webrtc/modules/modules.gyp (right): > > https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.... > webrtc/modules/modules.gyp:162: > 'audio_conference_mixer/test/audio_conference_mixer_unittest.cc', > nit: It's more typical to put the unit test alongside the production code. Hi Andrew, This was written from scratch.
https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc (right): https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:15: #include "testing/gmock/include/gmock/gmock.h" This can go with the other includes. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:36: .WillByDefault(Invoke(this, &MockMixerParticipant::FakeAudioFrame)); If you always want this behavior, don't mock it, but "fake" it. Move FakeAudioFrame into GetAudioFrame's implementation: int32_t GetAudioFrame(const int32_t id, AudioFrame& audio_frame) override { audio_frame.CopyFrom(fake_frame_); return 0; } BTW, there are so many style violations in this stuff. Do you have any interest in fixing them? Just to take this method, the interface should be: int GetAudioFrame(int id, AudioFrame* audio_frame); https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:41: AudioFrame fake_frame_; This should be private with a setter. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:49: TEST(AudioConferenceMixer, AnonymousAndUnanonymous) { Nonanonymous is a bit more natural (but also weird). https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:59: mixer.reset(AudioConferenceMixer::Create(kId)); Don't reset, assign directly: rtc::scoped_ptr<AudioConferenceMixer> mixer(AudioConferenceMixer::Create(kId)); https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:62: MockMixerParticipant anonymous[kAnonymous]; Use vectors. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:65: for (int i = 0; i < kUnanonymous; i++) { nit: ++i and below https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:67: EXPECT_EQ(0, mixer->MixabilityStatus(unanonymous[i], ret)); Is there any value in testing this with more than one participant? https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:71: for (int i = 0; i < kAnonymous; i++) { Same here, is this loop adding any value? And the other loops in this test. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:121: mixer.reset(AudioConferenceMixer::Create(kId)); As above, reset is not needed. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:126: MockMixerParticipant participants[kParticipants]; vector https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:129: participants[i].fake_frame_.UpdateFrame(i, 0, nullptr, 320, 32000, This is a rarely used method. Could you just set the members you want to change directly? https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:132: // Use 80 to escape from ramped-in window. Do you mean that you need to choose a late enough sample to avoid ramping? I think you should elaborate a bit. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:152: bool ret; Instead of ret, name this what it is: is_mixed.
andrew@webrtc.org changed reviewers: - ajm@chromium.org
Patchset #2 (id:60001) has been deleted
Hi Andrew, Thank you for your comments! I made some arguments and made a new patch set. Please take another look. Thanks! https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... File webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc (right): https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:36: .WillByDefault(Invoke(this, &MockMixerParticipant::FakeAudioFrame)); On 2015/08/18 04:03:48, Andrew MacDonald wrote: > If you always want this behavior, don't mock it, but "fake" it. Move > FakeAudioFrame into GetAudioFrame's implementation: > The reason that I mock GetAudioFrame is that I want to check whether it is called with EXPECT_CALL. > int32_t GetAudioFrame(const int32_t id, AudioFrame& audio_frame) override { > audio_frame.CopyFrom(fake_frame_); > return 0; > } > > BTW, there are so many style violations in this stuff. Do you have any interest > in fixing them? Just to take this method, the interface should be: > int GetAudioFrame(int id, AudioFrame* audio_frame); Yes, I'd like to do this. Maybe in a separate CL to be cleaner. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:41: AudioFrame fake_frame_; On 2015/08/18 04:03:48, Andrew MacDonald wrote: > This should be private with a setter. Yes, it can be nicer but the it is more complicated to change its value, you may need to write more functions to modify its data value, VAD flag, etc. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:49: TEST(AudioConferenceMixer, AnonymousAndUnanonymous) { On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Nonanonymous is a bit more natural (but also weird). How about "Named"? https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:59: mixer.reset(AudioConferenceMixer::Create(kId)); On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Don't reset, assign directly: > rtc::scoped_ptr<AudioConferenceMixer> mixer(AudioConferenceMixer::Create(kId)); Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:62: MockMixerParticipant anonymous[kAnonymous]; On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Use vectors. Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:65: for (int i = 0; i < kUnanonymous; i++) { On 2015/08/18 04:03:48, Andrew MacDonald wrote: > nit: ++i and below Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:67: EXPECT_EQ(0, mixer->MixabilityStatus(unanonymous[i], ret)); On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Is there any value in testing this with more than one participant? May be not very needed. But I made the number of "named" to be larger than kMaximumAmountOfMixedParticipants (see line 55). To see that it won't be limited by the kMaximumAmountOfMixedParticipants https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:71: for (int i = 0; i < kAnonymous; i++) { On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Same here, is this loop adding any value? And the other loops in this test. I made the number of "named" to be larger than kMaximumAmountOfMixedParticipants (see line 55). To see that it won't be limited by the kMaximumAmountOfMixedParticipants https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:121: mixer.reset(AudioConferenceMixer::Create(kId)); On 2015/08/18 04:03:48, Andrew MacDonald wrote: > As above, reset is not needed. Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:126: MockMixerParticipant participants[kParticipants]; On 2015/08/18 04:03:48, Andrew MacDonald wrote: > vector Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:129: participants[i].fake_frame_.UpdateFrame(i, 0, nullptr, 320, 32000, On 2015/08/18 04:03:48, Andrew MacDonald wrote: > This is a rarely used method. Could you just set the members you want to change > directly? I searched for its usage, and found that it is not very rarely used. I also feel that it fits here nicely, otherwise, one needs to understand the default value and modify one-by-one, making it more lines and less readible. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:132: // Use 80 to escape from ramped-in window. On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Do you mean that you need to choose a late enough sample to avoid ramping? I > think you should elaborate a bit. Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:152: bool ret; On 2015/08/18 04:03:48, Andrew MacDonald wrote: > Instead of ret, name this what it is: is_mixed. Done. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/modules.... webrtc/modules/modules.gyp:162: 'audio_conference_mixer/test/audio_conference_mixer_unittest.cc', On 2015/08/04 21:09:03, ajm wrote: > nit: It's more typical to put the unit test alongside the production code. Ok, but does that mean a big change in 'target_name': 'modules_unittests',
ajm@chromium.org changed reviewers: + ajm@chromium.org
lgtm with a few changes. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... File webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc (right): https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:36: .WillByDefault(Invoke(this, &MockMixerParticipant::FakeAudioFrame)); On 2015/08/25 15:36:12, minyue-webrtc wrote: > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > If you always want this behavior, don't mock it, but "fake" it. Move > > FakeAudioFrame into GetAudioFrame's implementation: > > > The reason that I mock GetAudioFrame is that I want to check whether it is > called with EXPECT_CALL. Ah OK. You can often define the methods actions directly via the mock, but looks like needing to use AudioFrame::CopyFrom would make that difficult. > > > int32_t GetAudioFrame(const int32_t id, AudioFrame& audio_frame) override { > > audio_frame.CopyFrom(fake_frame_); > > return 0; > > } > > > > BTW, there are so many style violations in this stuff. Do you have any > interest > > in fixing them? Just to take this method, the interface should be: > > int GetAudioFrame(int id, AudioFrame* audio_frame); > > Yes, I'd like to do this. Maybe in a separate CL to be cleaner. Yep, sounds good. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:41: AudioFrame fake_frame_; On 2015/08/25 15:36:12, minyue-webrtc wrote: > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > This should be private with a setter. > > Yes, it can be nicer but the it is more complicated to change its value, you may > need to write more functions to modify its data value, VAD flag, etc. The typical way to do this is: AudioFrame* fake_frame() { return &fake_frame_; } Since this is a test class, I won't insist that you change it, but it's odd to see a public data member. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:49: TEST(AudioConferenceMixer, AnonymousAndUnanonymous) { On 2015/08/25 15:36:12, minyue-webrtc wrote: > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > Nonanonymous is a bit more natural (but also weird). > > How about "Named"? Nice. https://codereview.chromium.org/1257583011/diff/40001/webrtc/modules/audio_co... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:129: participants[i].fake_frame_.UpdateFrame(i, 0, nullptr, 320, 32000, On 2015/08/25 15:36:12, minyue-webrtc wrote: > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > This is a rarely used method. Could you just set the members you want to > change > > directly? > > I searched for its usage, and found that it is not very rarely used. I also feel > that it fits here nicely, otherwise, one needs to understand the default value > and modify one-by-one, making it more lines and less readible. I'd argue that this is _less_ readable since you have a bunch of literals assigned to who-knows-what. Two suggestions: - Set these default values in the MockMixerParticipant constructor. - Replace 320 and 32000 literals with constants to be used anywhere else they're needed (e.g. line 139).
last comments addressed and made a rebase. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... File webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc (right): https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:41: AudioFrame fake_frame_; On 2015/08/25 16:46:03, ajm wrote: > On 2015/08/25 15:36:12, minyue-webrtc wrote: > > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > > This should be private with a setter. > > > > Yes, it can be nicer but the it is more complicated to change its value, you > may > > need to write more functions to modify its data value, VAD flag, etc. > > The typical way to do this is: > AudioFrame* fake_frame() { return &fake_frame_; } > > Since this is a test class, I won't insist that you change it, but it's odd to > see a public data member. I like the proposal, changes made accordingly. https://codereview.webrtc.org/1257583011/diff/40001/webrtc/modules/audio_conf... webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc:129: participants[i].fake_frame_.UpdateFrame(i, 0, nullptr, 320, 32000, On 2015/08/25 16:46:03, ajm wrote: > On 2015/08/25 15:36:12, minyue-webrtc wrote: > > On 2015/08/18 04:03:48, Andrew MacDonald wrote: > > > This is a rarely used method. Could you just set the members you want to > > change > > > directly? > > > > I searched for its usage, and found that it is not very rarely used. I also > feel > > that it fits here nicely, otherwise, one needs to understand the default value > > and modify one-by-one, making it more lines and less readible. > > I'd argue that this is _less_ readable since you have a bunch of literals > assigned to who-knows-what. > > Two suggestions: > - Set these default values in the MockMixerParticipant constructor. > - Replace 320 and 32000 literals with constants to be used anywhere else they're > needed (e.g. line 139). I'd take your initial suggestion.
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:120001) manually as 22c2729607964aa38d6cb4e521994453b6a271c4 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
