Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736503002:
Adds unit test for ADM on Linux (Closed)
|
Created:
3 years, 9 months ago by henrika_webrtc Modified:
3 years, 9 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds unit test for ADM on Linux
BUG=webrtc:7273
Review-Url: https://codereview.webrtc.org/2736503002
Cr-Commit-Position: refs/heads/master@{#17285}
Committed: https://chromium.googlesource.com/external/webrtc/+/f2f91fa2f942234dd5330ff7cfb617ac1c14c592
Patch Set 1 #Patch Set 2 : Added recording test #Patch Set 3 : Added full duplex test #Patch Set 4 : cleanup #Patch Set 5 : git cl format #Patch Set 6 : Added more restrictions to run these tests #
Total comments: 18
Patch Set 7 : Feedback from solenberg@ #
Total comments: 8
Patch Set 8 : Final tuning #
Messages
Total messages: 31 (22 generated)
Description was changed from ========== Adds unit test for ADM on Linux nit Adds unit test for ADM on Linux Added more tests Base for AudioDevice unit test BUG= ========== to ========== Adds unit test for ADM on Linux BUG=TODO ==========
Description was changed from ========== Adds unit test for ADM on Linux BUG=TODO ========== to ========== Adds unit test for ADM on Linux BUG=webrtc:7273 ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
PTAL
The CQ bit was checked by henrika@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/10338) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by henrika@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Good stuff! I have a few comments, but nothing really major. Thanks for adding this and sorry for taking so long to review. :/ https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/BUILD.gn:278: if (is_linux) { not possible to run on win/mac/ios? https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:38: you want to put all the local types in an anonymous namespace: namespace { ... https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:45: enum TransportType { enum class https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:46: kPlayout = 0x1, I'd actually prefer to have separate enum members: kPlay, kRecord, kPlayRecord, rather than using a bit mask. If you insist on keep the bit mask, add a comment above the enum to explain that's what it is. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:56: : event_(nullptr), prefer inline member initialization to ctor init list https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:62: virtual ~MockAudioTransport() {} override, not virtual https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:153: if (rec_mode()) brackets, please https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:233: rtc::scoped_refptr<AudioDeviceModule> CreateAudioDevice( Internal method only used once - fold into ctor.
Done. Thanks! PTAL https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/BUILD.gn:278: if (is_linux) { I have written special versions of this test for Android and iOS before (they have been used for a long period of time). These platforms contains some unique parts that are not suitable to move up to this more generic (simpler) test. Hence, no Android and iOS here. My plan was to enable Mac and Windows after trying out the Linux version for a while first so see how well it works on our bots. In short: desktop platforms only. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:38: On 2017/03/16 07:57:35, the sun wrote: > you want to put all the local types in an anonymous namespace: > > namespace { > ... Acknowledged. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:38: On 2017/03/16 07:57:35, the sun wrote: > you want to put all the local types in an anonymous namespace: > > namespace { > ... Done. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:45: enum TransportType { On 2017/03/16 07:57:35, the sun wrote: > enum class Done. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:46: kPlayout = 0x1, Agree. Will change. Hard to combine enum class and bitmask anyhow. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:56: : event_(nullptr), On 2017/03/16 07:57:35, the sun wrote: > prefer inline member initialization to ctor init list Done. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:62: virtual ~MockAudioTransport() {} On 2017/03/16 07:57:35, the sun wrote: > override, not virtual Done. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:153: if (rec_mode()) On 2017/03/16 07:57:35, the sun wrote: > brackets, please Done. https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:233: rtc::scoped_refptr<AudioDeviceModule> CreateAudioDevice( On 2017/03/16 07:57:35, the sun wrote: > Internal method only used once - fold into ctor. Done.
The CQ bit was checked by henrika@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few minor comments. LGTM https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/BUILD.gn (right): https://codereview.webrtc.org/2736503002/diff/100001/webrtc/modules/audio_dev... webrtc/modules/audio_device/BUILD.gn:278: if (is_linux) { On 2017/03/16 14:40:03, henrika_webrtc wrote: > I have written special versions of this test for Android and iOS before (they > have been used for a long period of time). These platforms contains some unique > parts that are not suitable to move up to this more generic (simpler) test. > Hence, no Android and iOS here. > > My plan was to enable Mac and Windows after trying out the Linux version for a > while first so see how well it works on our bots. > > In short: desktop platforms only. Acknowledged. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:187: TransportType type_; = kInvalid (or remove kInvalid from the enum) https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:197: class AudioDeviceTest : public ::testing::Test { I had one more comment, which appears to have been lost. I'd usually prefer to not use a fixture, but instead a test helper that you put on the stack at the beginning of each test case (see e.g. audio_send_stream_unittest.cc). That makes the code easier to follow, and more flexible, e.g. if a certain test case would require an argument to the ctor, it is a simple matter to add one. With a gtest fixture, you would end up with either a whole separate base class, a "reconfig" method, or (worst case) implementation inheritance in multiple steps. I'm fine with this as is - just wanted to mention an alternative, because doing test fixtures this way has spread across the code base as a bit of an anti-pattern IMO. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:229: // TODO(henrika): perhaps use SetUpTestCase() to share initial setup between We're still sharding test cases in separate processes, so I don't think that would do a lot for speed. In fact, couldn't you fold these into the ctor/dtor to make the code a little simpler? https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:286: bool requirements_satisfied_ = true; Make these private: and add an accessor for event_.
Please check one last time just in case. Mainly nits but you never know. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:187: TransportType type_; On 2017/03/16 15:24:14, the sun wrote: > = kInvalid > > (or remove kInvalid from the enum) Done. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:197: class AudioDeviceTest : public ::testing::Test { Thanks. I'll consider that next time. I have written some tests before and using fixtures has (in Chrome) always been the recommended method. Do see your points though. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:229: // TODO(henrika): perhaps use SetUpTestCase() to share initial setup between I removed the use of SetUp/TearDown. https://codereview.webrtc.org/2736503002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_unittest.cc:286: bool requirements_satisfied_ = true; Used raw pointer as return value.
The CQ bit was checked by henrika@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm
The CQ bit was checked by henrika@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": 140001, "attempt_start_ts": 1489749840277720,
"parent_rev": "994ebb47024e7bf7e65039384a48a9854011d283", "commit_rev":
"f2f91fa2f942234dd5330ff7cfb617ac1c14c592"}
Message was sent while issue was closed.
Description was changed from ========== Adds unit test for ADM on Linux BUG=webrtc:7273 ========== to ========== Adds unit test for ADM on Linux BUG=webrtc:7273 Review-Url: https://codereview.webrtc.org/2736503002 Cr-Commit-Position: refs/heads/master@{#17285} Committed: https://chromium.googlesource.com/external/webrtc/+/f2f91fa2f942234dd5330ff7c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/f2f91fa2f942234dd5330ff7c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
