Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393923002:
Add unit tests for webrtc_media_stream_adapter.cc (Closed)
|
Created:
4 years, 2 months ago by hta - Chromium Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit tests for webrtc_media_stream_adapter.cc
Also adds "googNoiseReduction" to the set of permitted
video constraints (needed for mandatory to be allowed).
BUG=645907
Committed: https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495
Cr-Commit-Position: refs/heads/master@{#424252}
Patch Set 1 : Completed tests for denoising constraint #
Total comments: 10
Patch Set 2 : Addressed review comments #
Total comments: 4
Patch Set 3 : Renamed test function #Messages
Total messages: 30 (17 generated)
Description was changed from ========== Add unit tests for webrtc_media_stream_adapter.cc NOT WORKING: The command ninja -C out/Default -j 12 content_unittests returns that the constructor and destructor of the implementation under test are not found. BUG=645907 ========== to ========== Add unit tests for webrtc_media_stream_adapter.cc Also adds "googNoiseReduction" to the set of permitted video constraints (needed for mandatory to be allowed). BUG=645907 ==========
hta@chromium.org changed reviewers: + pbos@chromium.org, tommi@chromium.org
The CQ bit was checked by hta@chromium.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.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Tommi and Peter, please review. There is an unsolved problem wrt what happens if track generation fails (see TODO in test), but in production, I'm assuming that we check tracks before we get far enough to encounter that issue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Peter hasn't gotten any mail from my previous attempt, so trying again.
lgtm % trivial comments https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:31: "googNoiseReduction"}; should there be a TODO here to remove the 'goog' prefix? Maybe there's already a bug filed for this. https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); SourceNeedsDenoisingForTesting() Should it also be const? Actually, instead of adding this method, would it work to inherit from MediaStreamVideoWebRtcSink in a test only class (in the cc file of the test that needs it) and have this method only there? https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc:36: std::unique_ptr<MockPeerConnectionDependencyFactory> dependency_factory_; nit: what about MockPeerConnectionDependencyFactory dependency_factory_; ?
Addressed Tommi's comments (two "done", one partial pushback). https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:31: "googNoiseReduction"}; On 2016/10/10 09:53:35, tommi (chrömium) wrote: > should there be a TODO here to remove the 'goog' prefix? > Maybe there's already a bug filed for this. crbug.com/605673 - this covers all the goog constraints. I added reference to it in MediaConstraintsImpl.cpp (where the names are). https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); On 2016/10/10 09:53:35, tommi (chrömium) wrote: > SourceNeedsDenoisingForTesting() > > Should it also be const? > > Actually, instead of adding this method, would it work to inherit from > MediaStreamVideoWebRtcSink in a test only class (in the cc file of the test that > needs it) and have this method only there? I don't like having derived classes as the implementation under test in the tests - it's a bit too easy to introduce behavior in the derived class that doesn't match 100% the behavior of the class we are trying to test. Renamed it to SourceNeedsDenoisingTestHelper. https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc:36: std::unique_ptr<MockPeerConnectionDependencyFactory> dependency_factory_; On 2016/10/10 09:53:35, tommi (chrömium) wrote: > nit: what about MockPeerConnectionDependencyFactory dependency_factory_; ? Done.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2393923002/#ps60001 (title: "Addressed review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); On 2016/10/10 13:15:02, hta - Chromium wrote: > On 2016/10/10 09:53:35, tommi (chrömium) wrote: > > SourceNeedsDenoisingForTesting() > > > > Should it also be const? > > > > Actually, instead of adding this method, would it work to inherit from > > MediaStreamVideoWebRtcSink in a test only class (in the cc file of the test > that > > needs it) and have this method only there? > > I don't like having derived classes as the implementation under test in the > tests - it's a bit too easy to introduce behavior in the derived class that > doesn't match 100% the behavior of the class we are trying to test. > > Renamed it to SourceNeedsDenoisingTestHelper. The convention is to have the suffix "ForTesting", so I suggest we stick to that if we need the functionality here. The counter argument for the second comment is that we don't want to ship test code. There's a balance to strike there and from what I've seen, we use ForTesting methods only in classes where it's inhibitively complex to test an implementation without modifying it directly.
The CQ bit was unchecked by tommi@chromium.org
lgtm, FWIW I think ForTesting sounds better than TestHelper as a suffix https://codereview.chromium.org/2393923002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc (right): https://codereview.chromium.org/2393923002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc:42: TEST_F(MediaStreamVideoWebRtcSinkTest, ObjectConstructor) { Can you put something descriptive of the test, such as DoesNotSetDenoisingByDefault instead of ObjectConstruction? Feel free to think of a better name though. https://codereview.chromium.org/2393923002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc:51: TEST_F(MediaStreamVideoWebRtcSinkTest, NoiseReductionConstraintPassThrough) { Pref naming this test PassesThroughNoiseReductionConstraint https://codereview.chromium.org/2393923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/2393923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120: // These need to be made standard or deleted. crbug.com/605673 this should be in TODO(hta) format
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); On 2016/10/10 13:33:37, tommi (chrömium) wrote: > On 2016/10/10 13:15:02, hta - Chromium wrote: > > On 2016/10/10 09:53:35, tommi (chrömium) wrote: > > > SourceNeedsDenoisingForTesting() > > > > > > Should it also be const? > > > > > > Actually, instead of adding this method, would it work to inherit from > > > MediaStreamVideoWebRtcSink in a test only class (in the cc file of the test > > that > > > needs it) and have this method only there? > > > > I don't like having derived classes as the implementation under test in the > > tests - it's a bit too easy to introduce behavior in the derived class that > > doesn't match 100% the behavior of the class we are trying to test. > > > > Renamed it to SourceNeedsDenoisingTestHelper. > > The convention is to have the suffix "ForTesting", so I suggest we stick to that > if we need the functionality here. On that: "Functions used only for testing should be restricted to test-only scenarios either by #ifdefing them appropriately (e.g. #if defined(UNIT_TEST)) or by naming them with a ForTesting suffix. The latter will be checked at presubmit time to ensure they're only called by test files." https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > The counter argument for the second comment is that we don't want to ship test > code. There's a balance to strike there and from what I've seen, we use > ForTesting methods only in classes where it's inhibitively complex to test an > implementation without modifying it directly.
mcasas@chromium.org changed reviewers: - pbos@chromium.org
Looks better now? https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); On 2016/10/10 15:58:22, mcasas wrote: > On 2016/10/10 13:33:37, tommi (chrömium) wrote: > > On 2016/10/10 13:15:02, hta - Chromium wrote: > > > On 2016/10/10 09:53:35, tommi (chrömium) wrote: > > > > SourceNeedsDenoisingForTesting() > > > > > > > > Should it also be const? > > > > > > > > Actually, instead of adding this method, would it work to inherit from > > > > MediaStreamVideoWebRtcSink in a test only class (in the cc file of the > test > > > that > > > > needs it) and have this method only there? > > > > > > I don't like having derived classes as the implementation under test in the > > > tests - it's a bit too easy to introduce behavior in the derived class that > > > doesn't match 100% the behavior of the class we are trying to test. > > > > > > Renamed it to SourceNeedsDenoisingTestHelper. > > > > The convention is to have the suffix "ForTesting", so I suggest we stick to > that > > if we need the functionality here. > > On that: > "Functions used only for testing should be restricted > to test-only scenarios either by #ifdefing them > appropriately (e.g. #if defined(UNIT_TEST)) or by > naming them with a ForTesting suffix. The latter will > be checked at presubmit time to ensure they're only > called by test files." > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > > > > > The counter argument for the second comment is that we don't want to ship test > > code. There's a balance to strike there and from what I've seen, we use > > ForTesting methods only in classes where it's inhibitively complex to test an > > implementation without modifying it directly. > Ack - if the "ForTesting suffix is magical, it makes sense to use it all the time. The accessor is an accessor that calls an accessor to a sub-object that is not exposed in the .h file at all - I count exposing the sub-object in the .h file just so that we can write a derived class that accesses it as "complex to test an implementation without modifying it". (I also looked at modifying the class under test to use an injected class instead of the WebRTC class, but it's a class hierarchy, not an encapsulation, so this would involve rewriting the entire logic of the class.) https://codereview.chromium.org/2393923002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/2393923002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:120: // These need to be made standard or deleted. crbug.com/605673 On 2016/10/10 15:43:33, pbos wrote: > this should be in TODO(hta) format Done.
https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/media_stream_video_webrtc_sink.h (right): https://codereview.chromium.org/2393923002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/media_stream_video_webrtc_sink.h:41: rtc::Optional<bool> SourceNeedsDenoising(); On 2016/10/10 18:59:01, hta - Chromium wrote: > On 2016/10/10 15:58:22, mcasas wrote: > > On 2016/10/10 13:33:37, tommi (chrömium) wrote: > > > On 2016/10/10 13:15:02, hta - Chromium wrote: > > > > On 2016/10/10 09:53:35, tommi (chrömium) wrote: > > > > > SourceNeedsDenoisingForTesting() > > > > > > > > > > Should it also be const? > > > > > > > > > > Actually, instead of adding this method, would it work to inherit from > > > > > MediaStreamVideoWebRtcSink in a test only class (in the cc file of the > > test > > > > that > > > > > needs it) and have this method only there? > > > > > > > > I don't like having derived classes as the implementation under test in > the > > > > tests - it's a bit too easy to introduce behavior in the derived class > that > > > > doesn't match 100% the behavior of the class we are trying to test. > > > > > > > > Renamed it to SourceNeedsDenoisingTestHelper. > > > > > > The convention is to have the suffix "ForTesting", so I suggest we stick to > > that > > > if we need the functionality here. > > > > On that: > > "Functions used only for testing should be restricted > > to test-only scenarios either by #ifdefing them > > appropriately (e.g. #if defined(UNIT_TEST)) or by > > naming them with a ForTesting suffix. The latter will > > be checked at presubmit time to ensure they're only > > called by test files." > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > > > > > > > > > The counter argument for the second comment is that we don't want to ship > test > > > code. There's a balance to strike there and from what I've seen, we use > > > ForTesting methods only in classes where it's inhibitively complex to test > an > > > implementation without modifying it directly. > > > > Ack - if the "ForTesting suffix is magical, it makes sense to use it all the > time. The accessor is an accessor that calls an accessor to a sub-object that is > not exposed in the .h file at all - I count exposing the sub-object in the .h > file just so that we can write a derived class that accesses it as "complex to > test an implementation without modifying it". > > (I also looked at modifying the class under test to use an injected class > instead of the WebRTC class, but it's a class hierarchy, not an encapsulation, > so this would involve rewriting the entire logic of the class.) > That's OK with me. Just to clarify my comment, that is not what I mean. If the complexity is to make a member variable protected instead of private, that does not qualify as complex. :) What I mean is more inline with e.g. the startup path of the browser, whereby mocking the entire browser is too complex and we instead add "ForTesting" methods to that harness that allow for testing. However, for this CL, the implementation is trivial and if it's inline, will not cost us bytes of shipping dead code.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, pbos@chromium.org Link to the patchset: https://codereview.chromium.org/2393923002/#ps80001 (title: "Renamed test function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for webrtc_media_stream_adapter.cc Also adds "googNoiseReduction" to the set of permitted video constraints (needed for mandatory to be allowed). BUG=645907 ========== to ========== Add unit tests for webrtc_media_stream_adapter.cc Also adds "googNoiseReduction" to the set of permitted video constraints (needed for mandatory to be allowed). BUG=645907 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for webrtc_media_stream_adapter.cc Also adds "googNoiseReduction" to the set of permitted video constraints (needed for mandatory to be allowed). BUG=645907 ========== to ========== Add unit tests for webrtc_media_stream_adapter.cc Also adds "googNoiseReduction" to the set of permitted video constraints (needed for mandatory to be allowed). BUG=645907 Committed: https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495 Cr-Commit-Position: refs/heads/master@{#424252} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/38db686694408c72809cf99a0d664a6d2b6c7495 Cr-Commit-Position: refs/heads/master@{#424252} |
