Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292093002:
RtcpReceiverTest rewritten using public available interface (Closed)
|
Created:
4 years, 3 months ago by danilchap Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRtcpReceiverTest rewritten using public available interface
IncomingPacket(const uint8_t*, size_t) is used as entry point instead
of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in);
Result is validated by checking which callbacks were called instead
of checking intermediate structure PacketInformaion.
This allows to switch parsing implementation.
BUG=webrtc:5260
Committed: https://crrev.com/1e714ae7a655e8ce20a53ba4a8121b75eed9c8c1
Cr-Commit-Position: refs/heads/master@{#14074}
Patch Set 1 #Patch Set 2 : Unified SetUp and constants. #Patch Set 3 : Nits #
Total comments: 45
Patch Set 4 : Feedback #Patch Set 5 : Feedback #
Total comments: 2
Patch Set 6 : Added TODO to test with hidden expectations. #Messages
Total messages: 24 (12 generated)
Description was changed from ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called. This would allow to switch parsing implementation. BUG=webrtc:5260 ========== to ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This would allow to switch parsing implementation. BUG=webrtc:5260 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Description was changed from ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This would allow to switch parsing implementation. BUG=webrtc:5260 ========== to ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This allows to switch parsing implementation. BUG=webrtc:5260 ==========
danilchap@webrtc.org changed reviewers: + philipel@webrtc.org, sprang@webrtc.org
Rework of the https://codereview.webrtc.org/1527003002/ In addition to replacing expectation and cleanup of execution phases, this CL also rename kSourceSsrc to kReceiverSsrc and move SetSsrc() configuration into SetUp. Hope this way tests better document how rtcp_receiver is used.
Nice! Just some minor comments. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (left): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1060: EXPECT_LT(0U, candidate_set[0].bitrate_bps()); Ehm, "EXPECT_LT(0U," ??? That's one way to do it I guess... :) https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:112: : system_clock_(1335900000), Is there any meaning to this particular value? Is ever referenced? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:130: } nit: \n https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:168: rtcp::Rpsi::kPacketType, 0, 3, Is this dent in indentation intended? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:233: TEST_F(RtcpReceiverTest, InjectSrPacketFromExpectedPeer) { Maybe it makes sense to move this up and call it InjectSrPacket, and rename the current InjectSrPacket to InjectSrPacketFromUnknownSender? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:418: system_clock_.AdvanceTimeMilliseconds(500); Why 500? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:516: rb.WithLastSr(0x12344321); For readability, translate system time to ntp instead of magic constant? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:560: sdes.WithCName(kSenderSsrc, "alice@host"); Named constant for "alice@host"? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:675: sli.WithPictureId(40); Named constant https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:681: TEST_F(RtcpReceiverTest, InjectRPsiPacket) { nit: s/InjectRPsiPacket/InjectRpsiPacket https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1015: tmmbr.WithTmmbr(rtcp::TmmbItem(kReceiverSsrc, 30000, 0)); Named constant for 30000? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1096: EXPECT_LT(0U, candidate_set[0].bitrate_bps()); Please update this to check for the actual bitrate (here and below), just like you did for the previous test case. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1230: Field(&RtcpPacketTypeCounter::unique_nack_requests, 4)))); kNackListLength1 instead of 4 Derive below as well.
https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (left): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1060: EXPECT_LT(0U, candidate_set[0].bitrate_bps()); On 2016/09/05 11:05:49, språng wrote: > Ehm, "EXPECT_LT(0U," ??? > That's one way to do it I guess... :) There are only two possible values: 0 and more. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:112: : system_clock_(1335900000), On 2016/09/05 11:05:49, språng wrote: > Is there any meaning to this particular value? Is ever referenced? No, just 'random' non-zero seed for the clock. Kept untouched. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:130: } On 2016/09/05 11:05:49, språng wrote: > nit: \n Done. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:168: rtcp::Rpsi::kPacketType, 0, 3, On 2016/09/05 11:05:49, språng wrote: > Is this dent in indentation intended? Yes, to show rtcp::Rpsi::kPacketType is aligned with 2nd byte when formatting in a 4 bytes per-row. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:233: TEST_F(RtcpReceiverTest, InjectSrPacketFromExpectedPeer) { On 2016/09/05 11:05:49, språng wrote: > Maybe it makes sense to move this up and call it InjectSrPacket, and rename the > current InjectSrPacket to InjectSrPacketFromUnknownSender? It sure does, Done. Tried to minimize moving code around to give review tool chance to better align changes, but here it didn't help anyway. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:418: system_clock_.AdvanceTimeMilliseconds(500); On 2016/09/05 11:05:49, språng wrote: > Why 500? Actual value unimportant. As long as it is >0. Comment added. Having it 0 (i.e. removing line) would pass too, but will make test slightly weaker. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:516: rb.WithLastSr(0x12344321); On 2016/09/05 11:05:49, språng wrote: > For readability, translate system time to ntp instead of magic constant? Removed. Actually it is not needed, yet. Will add with comment why it should not be 0 when it will be needed. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:560: sdes.WithCName(kSenderSsrc, "alice@host"); On 2016/09/05 11:05:49, språng wrote: > Named constant for "alice@host"? Done. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:675: sli.WithPictureId(40); On 2016/09/05 11:05:49, språng wrote: > Named constant Done. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:681: TEST_F(RtcpReceiverTest, InjectRPsiPacket) { On 2016/09/05 11:05:49, språng wrote: > nit: s/InjectRPsiPacket/InjectRpsiPacket Oops, there is already InjectRpsiPacket test. Duplicate removed. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1015: tmmbr.WithTmmbr(rtcp::TmmbItem(kReceiverSsrc, 30000, 0)); On 2016/09/05 11:05:49, språng wrote: > Named constant for 30000? Done. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1096: EXPECT_LT(0U, candidate_set[0].bitrate_bps()); On 2016/09/05 11:05:49, språng wrote: > Please update this to check for the actual bitrate (here and below), just like > you did for the previous test case. Missed, thank you, Done. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1230: Field(&RtcpPacketTypeCounter::unique_nack_requests, 4)))); On 2016/09/05 11:05:49, språng wrote: > kNackListLength1 instead of 4 > Derive below as well. Oops, Done.
https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:100: constexpr uint32_t kReceiverSsrc = 0x123456; kReceiverSsrc1 https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:173: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); Not needed since |intra_frame_observer_| is strict. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:187: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); ditto https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:204: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); ditto. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:546: TEST_F(RtcpReceiverTest, InjectAppWithData) { Is there any observable difference between InjectAppWithData and InjectAppWithNoData? Do we really test anything here? https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:647: EXPECT_CALL(intra_frame_observer_, OnReceivedIntraFrameRequest(_)).Times(0); No need, |intra_frame_observer_| is strict. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:669: .Times(0); ditto https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1047: EXPECT_CALL(bandwidth_observer_, OnReceivedEstimatedBitrate(_)).Times(0); strict https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1065: EXPECT_CALL(bandwidth_observer_, OnReceivedEstimatedBitrate(_)).Times(0); strict
lgtm
https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:100: constexpr uint32_t kReceiverSsrc = 0x123456; On 2016/09/05 11:59:27, philipel wrote: > kReceiverSsrc1 Most of the tests it is the only ReceiverSsrc. Will rename this constants to kReceiverMainSsrc/kReceiverExtraSsrc so that names work better both alone and together. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:173: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > Not needed since |intra_frame_observer_| is strict. Yes, but this test want to stress this call is not expected. It more like documentation what this test is about. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:187: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > ditto Acknowledged. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:204: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > ditto. Acknowledged. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:546: TEST_F(RtcpReceiverTest, InjectAppWithData) { On 2016/09/05 11:59:27, philipel wrote: > Is there any observable difference between InjectAppWithData and > InjectAppWithNoData? Do we really test anything here? Only that it doesn't crash and doesn't cause accidental callbacks. Two tests is probably too much, so will remove one. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:647: EXPECT_CALL(intra_frame_observer_, OnReceivedIntraFrameRequest(_)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > No need, |intra_frame_observer_| is strict. Acknowledged. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:669: .Times(0); On 2016/09/05 11:59:27, philipel wrote: > ditto Acknowledged. Same reason as before: want to stress this callback shouldn't be called. (though this expectation should be stricter). https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1047: EXPECT_CALL(bandwidth_observer_, OnReceivedEstimatedBitrate(_)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > strict Acknowledged. https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1065: EXPECT_CALL(bandwidth_observer_, OnReceivedEstimatedBitrate(_)).Times(0); On 2016/09/05 11:59:27, philipel wrote: > strict Acknowledged.
https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:173: EXPECT_CALL(intra_frame_observer_, OnReceivedRPSI(_, _)).Times(0); On 2016/09/05 13:27:55, danilchap wrote: > On 2016/09/05 11:59:27, philipel wrote: > > Not needed since |intra_frame_observer_| is strict. > > Yes, but this test want to stress this call is not expected. > It more like documentation what this test is about. I guess it works like documentation, but when you know |intra_frame_observer_| is strict it might make you think you misunderstood something (like it did me :)). https://codereview.webrtc.org/2292093002/diff/170001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/170001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:162: InjectRtcpPacket(bad_packet); Maybe add EXPECT_CALL(some_object_, (params and stuff)).Times(0) to document what we are testing for?
https://codereview.webrtc.org/2292093002/diff/170001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right): https://codereview.webrtc.org/2292093002/diff/170001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:162: InjectRtcpPacket(bad_packet); On 2016/09/05 13:38:44, philipel wrote: > Maybe add EXPECT_CALL(some_object_, (params and stuff)).Times(0) to document > what we are testing for? Here it mostly tests parser won't crash or hang (It did). Added a TODO to add the only expectation that make sense for this packet (it fails now)
lgtm
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2292093002/#ps190001 (title: "Added TODO to test with hidden expectations.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This allows to switch parsing implementation. BUG=webrtc:5260 ========== to ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This allows to switch parsing implementation. BUG=webrtc:5260 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This allows to switch parsing implementation. BUG=webrtc:5260 ========== to ========== RtcpReceiverTest rewritten using public available interface IncomingPacket(const uint8_t*, size_t) is used as entry point instead of IncomingRTCPPacket(PacketInformation* out, RtcpParser* in); Result is validated by checking which callbacks were called instead of checking intermediate structure PacketInformaion. This allows to switch parsing implementation. BUG=webrtc:5260 Committed: https://crrev.com/1e714ae7a655e8ce20a53ba4a8121b75eed9c8c1 Cr-Commit-Position: refs/heads/master@{#14074} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1e714ae7a655e8ce20a53ba4a8121b75eed9c8c1 Cr-Commit-Position: refs/heads/master@{#14074} |
