Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779953003:
Add documentation to the ThresholdCurve unit tests (Closed)
|
Created:
3 years, 8 months ago by elad.alon Modified:
3 years, 8 months ago Reviewers:
minyue-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd documentation to the ThresholdCurve unit tests
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2779953003
Cr-Commit-Position: refs/heads/master@{#17454}
Committed: https://chromium.googlesource.com/external/webrtc/+/2d699acf6441e0262fef5955b63d1e21816c5672
Patch Set 1 #
Total comments: 33
Patch Set 2 : . #Patch Set 3 : s/hoizontal/horizontal #Messages
Total messages: 15 (6 generated)
Description was changed from ========== Add documentation to the ThresholdCurve unit tests BUG=webrtc:7058 ========== to ========== Add documentation to the ThresholdCurve unit tests BUG=webrtc:7058 ==========
elad.alon@webrtc.org changed reviewers: + minyue@webrtc.org
PTAL
https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:18: // the curve is normally defined by two distinct points (with one edge case why is it called "one edge case"? what is an edge? I think you may remove (xxx) and remove the word "distinct" https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:19: // when the points aren't distinct). Those points, P1 and P2, are ordered so A and B, or change the figure https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:36: // *-------------------------> // You don't seem to use the definitions here in the test names https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:54: // when the curve is a "general" one - P1 and P2 are different in both their general -> normal https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:56: TEST(ThresholdCurveTest, PointPositionGeneralCurve) { PointPositionToCommonCurve https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:147: TEST(ThresholdCurveTest, PointPositionCurvePointsHaveSameValueY) { PointPositionToCurveWithHorizaontalSegment https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:212: TEST(ThresholdCurveTest, PointPositionCurvePointsHaveSameValueX) { PointPositionToCurveWithVerticalSegment https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:283: TEST(ThresholdCurveTest, PointPositionCurvePointsAreIdentical) { PointPositionCurveWithNoSegment https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:326: TEST(ThresholdCurveTest, TwoCurvesSegmentHasSameProjectionAxisX) { TwoCurvesWithTouchingVerticalRay https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:355: TEST(ThresholdCurveTest, TwoCurvesSegmentOfHigherSubsetProjectionAxisX) { TwoCurvesWithoutTouching https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:387: TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower) { Looks the same as TwoCurvesSegmentHasSameProjectionAxisX https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:418: TEST(ThresholdCurveTest, TwoCurvesPointsOfHigherOnRaysOfLower) { TwoCurvesWithTouchingRays https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:502: // the second curve's segment intersects the first curve's segment. TwoCurvesWithCrossingSegments https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:566: TEST(ThresholdCurveTest, NearlyIdenticalCurvesSecondContinuesOnOtherLeftSide) { TwoCurvesWithTouchingSegmentAndHorizontalRay https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:595: TEST(ThresholdCurveTest, NearlyIdenticalCurvesSecondContinuesOnOtherRightSide) { TwoCurvesWithTouchingSegmentAndVerticalRay
https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:18: // the curve is normally defined by two distinct points (with one edge case On 2017/03/29 18:13:39, minyue-webrtc wrote: > why is it called "one edge case"? what is an edge? > > I think you may remove (xxx) and remove the word "distinct" Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:19: // when the points aren't distinct). Those points, P1 and P2, are ordered so On 2017/03/29 18:13:39, minyue-webrtc wrote: > A and B, or change the figure Changed the figure. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:36: // *-------------------------> // On 2017/03/29 18:13:39, minyue-webrtc wrote: > You don't seem to use the definitions here in the test names What do you mean? Here are some test names that use those definitions: * segment - TwoCurvesSegmentHasSameProjectionAxisX * horizontal ray - TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower These are just two examples. There are more. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:54: // when the curve is a "general" one - P1 and P2 are different in both their On 2017/03/29 18:13:39, minyue-webrtc wrote: > general -> normal Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:56: TEST(ThresholdCurveTest, PointPositionGeneralCurve) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > PointPositionToCommonCurve Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:147: TEST(ThresholdCurveTest, PointPositionCurvePointsHaveSameValueY) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > PointPositionToCurveWithHorizaontalSegment Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:212: TEST(ThresholdCurveTest, PointPositionCurvePointsHaveSameValueX) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > PointPositionToCurveWithVerticalSegment Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:283: TEST(ThresholdCurveTest, PointPositionCurvePointsAreIdentical) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > PointPositionCurveWithNoSegment I'll use "null segment" instead; I think it's clearer. If someone is confused, they can check the comment. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:326: TEST(ThresholdCurveTest, TwoCurvesSegmentHasSameProjectionAxisX) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > TwoCurvesWithTouchingVerticalRay That's not the focus here. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:355: TEST(ThresholdCurveTest, TwoCurvesSegmentOfHigherSubsetProjectionAxisX) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > TwoCurvesWithoutTouching That's not the focus here. This name would not distinguish this test from TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower, which also exhibits the property of the curves not touching. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:387: TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > Looks the same as TwoCurvesSegmentHasSameProjectionAxisX It's not. Please check the comment, the name and/or the code. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:418: TEST(ThresholdCurveTest, TwoCurvesPointsOfHigherOnRaysOfLower) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > TwoCurvesWithTouchingRays Touching rays would be true for for identical curves, too. This name helps to distinguish this test from IdenticalCurves. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:502: // the second curve's segment intersects the first curve's segment. On 2017/03/29 18:13:39, minyue-webrtc wrote: > TwoCurvesWithCrossingSegments Done. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:566: TEST(ThresholdCurveTest, NearlyIdenticalCurvesSecondContinuesOnOtherLeftSide) { I don't think that name is good. The segment is not "touching", it merged. "Touching" would imply intersection. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:595: TEST(ThresholdCurveTest, NearlyIdenticalCurvesSecondContinuesOnOtherRightSide) { On 2017/03/29 18:13:39, minyue-webrtc wrote: > TwoCurvesWithTouchingSegmentAndVerticalRay Similarly.
https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:36: // *-------------------------> // On 2017/03/29 18:50:13, elad.alon wrote: > On 2017/03/29 18:13:39, minyue-webrtc wrote: > > You don't seem to use the definitions here in the test names > > What do you mean? Here are some test names that use those definitions: > * segment - TwoCurvesSegmentHasSameProjectionAxisX > * horizontal ray - TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower > These are just two examples. There are more. I meant that you did not use "only" the definitions. You use terms "Projection" "ValueX" etc that are not defined here.
https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:36: // *-------------------------> // On 2017/03/29 18:55:52, minyue-webrtc wrote: > On 2017/03/29 18:50:13, elad.alon wrote: > > On 2017/03/29 18:13:39, minyue-webrtc wrote: > > > You don't seem to use the definitions here in the test names > > > > What do you mean? Here are some test names that use those definitions: > > * segment - TwoCurvesSegmentHasSameProjectionAxisX > > * horizontal ray - TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower > > These are just two examples. There are more. > > I meant that you did not use "only" the definitions. You use terms "Projection" > "ValueX" etc that are not defined here. Do you suggest to add an explanation about what "x-value" means? Or do you suggest to avoid the use of "x-value" and "projection"? I think those terms are too basic to be avoided (without losing clarity and/or focus). But I'm open for more suggestions.
(Uploaded)
lgtm Names are always hard. It may be still not perfect. But it is clearly improvements. https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2779953003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:387: TwoCurvesRightPointOfHigherCurveAboveHoizontalRayOfLower) { On 2017/03/29 18:50:12, elad.alon wrote: > On 2017/03/29 18:13:39, minyue-webrtc wrote: > > Looks the same as TwoCurvesSegmentHasSameProjectionAxisX > > It's not. Please check the comment, the name and/or the code. I see.
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2779953003/#ps40001 (title: "s/hoizontal/horizontal")
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": 40001, "attempt_start_ts": 1490818404743370,
"parent_rev": "3ff7a955525ffeff32f53c9d72fc6edcee66637a", "commit_rev":
"2d699acf6441e0262fef5955b63d1e21816c5672"}
Message was sent while issue was closed.
Description was changed from ========== Add documentation to the ThresholdCurve unit tests BUG=webrtc:7058 ========== to ========== Add documentation to the ThresholdCurve unit tests BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2779953003 Cr-Commit-Position: refs/heads/master@{#17454} Committed: https://chromium.googlesource.com/external/webrtc/+/2d699acf6441e0262fef5955b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/2d699acf6441e0262fef5955b... |
