Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975883002:
Remove dependency on rtc::Thread and rtc_base from audio_mixer_unittests. (Closed)
|
Created:
3 years, 5 months ago by tommi Modified:
3 years, 5 months ago Reviewers:
minyue-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, aleloi, hlundin-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove dependency on rtc::Thread and rtc_base from audio_mixer_unittests.
Instead, use a TaskQueue in the only test that required it.
BUG=none
Review-Url: https://codereview.webrtc.org/2975883002
Cr-Commit-Position: refs/heads/master@{#18969}
Committed: https://chromium.googlesource.com/external/webrtc/+/c45d6d9c850b86540cfe69ade7ce36dd9b2ab8fe
Patch Set 1 #
Total comments: 4
Messages
Total messages: 15 (8 generated)
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
tommi@webrtc.org changed reviewers: + minyue@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:387: EXPECT_CALL(participant, PreferredSampleRate()) would you explain why this additional expected call?
https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:387: EXPECT_CALL(participant, PreferredSampleRate()) On 2017/07/11 11:16:20, minyue-webrtc wrote: > would you explain why this additional expected call? This call is and should always be made so I'm setting the expectation and avoiding polluting the log. That wasn't done previously. There's an ON_CALL() handler set above, but that one only sets the default handling and prints an error when it's called - which because it always happens, we get trained to ignore (which is not a good thing). Take for example a look at the "GMOCK WARNING" lines for the AudioMixer tests in these runs: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=5961e2... https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f88549... (etc - same amount of noise from most of the AudioMixer tests) Then look at the output from the ConstructFromOtherThread test when it runs normally (with this patch): https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=799c70...
lgtm + one advice https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:387: EXPECT_CALL(participant, PreferredSampleRate()) On 2017/07/11 11:58:36, tommi (wëbrtc) wrote: > On 2017/07/11 11:16:20, minyue-webrtc wrote: > > would you explain why this additional expected call? > > This call is and should always be made so I'm setting the expectation and > avoiding polluting the log. That wasn't done previously. > > There's an ON_CALL() handler set above, but that one only sets the default > handling and prints an error when it's called - which because it always happens, > we get trained to ignore (which is not a good thing). > > Take for example a look at the "GMOCK WARNING" lines for the AudioMixer tests in > these runs: > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=5961e2... > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f88549... > > (etc - same amount of noise from most of the AudioMixer tests) > > Then look at the output from the ConstructFromOtherThread test when it runs > normally (with this patch): > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=799c70... Acknowledged. I also advice using StrictMock<MockMixerAudioSource> if possible
+aleloi https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2975883002/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:387: EXPECT_CALL(participant, PreferredSampleRate()) On 2017/07/11 13:03:22, minyue-webrtc wrote: > On 2017/07/11 11:58:36, tommi (wëbrtc) wrote: > > On 2017/07/11 11:16:20, minyue-webrtc wrote: > > > would you explain why this additional expected call? > > > > This call is and should always be made so I'm setting the expectation and > > avoiding polluting the log. That wasn't done previously. > > > > There's an ON_CALL() handler set above, but that one only sets the default > > handling and prints an error when it's called - which because it always > happens, > > we get trained to ignore (which is not a good thing). > > > > Take for example a look at the "GMOCK WARNING" lines for the AudioMixer tests > in > > these runs: > > > > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=5961e2... > > > > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f88549... > > > > (etc - same amount of noise from most of the AudioMixer tests) > > > > Then look at the output from the ConstructFromOtherThread test when it runs > > normally (with this patch): > > > > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=799c70... > > Acknowledged. > > I also advice using StrictMock<MockMixerAudioSource> if possible Yes that would be an improvement. This CL is about removing use of rtc::Thread though so I think that's a separate goal. Alex - could you take a look at that at some point?
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1499778817846520, "parent_rev":
"14c11a47123bcdf833eeb1f6a62521b69df4d463", "commit_rev":
"c45d6d9c850b86540cfe69ade7ce36dd9b2ab8fe"}
Message was sent while issue was closed.
Description was changed from ========== Remove dependency on rtc::Thread and rtc_base from audio_mixer_unittests. Instead, use a TaskQueue in the only test that required it. BUG=none ========== to ========== Remove dependency on rtc::Thread and rtc_base from audio_mixer_unittests. Instead, use a TaskQueue in the only test that required it. BUG=none Review-Url: https://codereview.webrtc.org/2975883002 Cr-Commit-Position: refs/heads/master@{#18969} Committed: https://chromium.googlesource.com/external/webrtc/+/c45d6d9c850b86540cfe69ade... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/c45d6d9c850b86540cfe69ade... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
