Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441543002:
RTCStats equality operator added (Closed)
|
Created:
4 years, 2 months ago by hbos Modified:
4 years, 1 month ago Reviewers:
hta-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRTCStats equality operator added.
This will be helpful in unittests to EXPECT_EQ reports. It should be a
useful operator to have outside of testing as well.
BUG=chromium:627816
NOTRY=True
Committed: https://crrev.com/67c8bc4bf2cc10669e5cb7fd461469f30edcaa23
Cr-Commit-Position: refs/heads/master@{#14767}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : Addressed comments and rebase with master #
Messages
Total messages: 23 (11 generated)
hbos@webrtc.org changed reviewers: + hta@webrtc.org
Please take a look, hta.
I doubt that this is valuable outside testing. I'd suggest to surround them with #ifdef IS_IN_UNIT_TEST or whatever the correct invocation is to avoid having them used outside tests for now. You need to decide whether ID and timestamp are significant for equality or not. https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h File webrtc/api/stats/rtcstats.h (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:68: // must correspond to the same member. This is a reflection of the fact that the Members() function is just an utility over a struct-like object, where each member is always present in Members() even if its is_present() is false. Suggests rephrasing as: For a given type, Members() always returns the same objects in the same order. https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:218: // Type and value comparator (can be true even if name and type isn't). Even if name and type isn't what? https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:275: } Didn't the presubmit check yell at you over having an inline function this long? git cl presubmit https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats.cc#ne... webrtc/stats/rtcstats.cc:54: std::vector<const RTCStatsMemberInterface*> members = Members(); What about the id and the timestamp? Are they significant for equality, or are they not? If they aren't, this needs to be prominently explained in the .h file. https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats_unitt... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats_unitt... webrtc/stats/rtcstats_unittest.cc:161: } Add a check for the case where types are different. Once you've decided whether timestamp and ID are significant for equality, add tests for those too.
hbos@chromium.org changed reviewers: + hbos@chromium.org
On 2016/10/21 08:56:01, hta-webrtc wrote: > I doubt that this is valuable outside testing. I'd suggest to surround them with > #ifdef IS_IN_UNIT_TEST or whatever the correct invocation is to avoid having > them used outside tests for now. Doing #if defined(UNIT_TEST) doesn't work if I put the implementation in the .cc file because the .cc file isn't in the unittest target. I'd have to inline it in the header file? And the ForTesting suffix doesn't work because it's an operator. https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h File webrtc/api/stats/rtcstats.h (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:68: // must correspond to the same member. On 2016/10/21 08:56:01, hta-webrtc wrote: > This is a reflection of the fact that the Members() function is just an utility > over a struct-like object, where each member is always present in Members() even > if its is_present() is false. > > Suggests rephrasing as: > > For a given type, Members() always returns the same objects in the same order. Done. https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:218: // Type and value comparator (can be true even if name and type isn't). On 2016/10/21 08:56:01, hta-webrtc wrote: > Even if name and type isn't what? That was a bit nonsensey. Changed to "Type and value comparator. The names are not compared." https://codereview.chromium.org/2441543002/diff/1/webrtc/api/stats/rtcstats.h... webrtc/api/stats/rtcstats.h:275: } On 2016/10/21 08:56:01, hta-webrtc wrote: > Didn't the presubmit check yell at you over having an inline function this long? > > git cl presubmit It does not, this is a template class. https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats.cc#ne... webrtc/stats/rtcstats.cc:54: std::vector<const RTCStatsMemberInterface*> members = Members(); On 2016/10/21 08:56:01, hta-webrtc wrote: > What about the id and the timestamp? > > Are they significant for equality, or are they not? > If they aren't, this needs to be prominently explained in the .h file. They should also be compared too, fixed that. Done. https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats_unitt... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.chromium.org/2441543002/diff/1/webrtc/stats/rtcstats_unitt... webrtc/stats/rtcstats_unittest.cc:161: } On 2016/10/21 08:56:01, hta-webrtc wrote: > Add a check for the case where types are different. Once you've decided whether > timestamp and ID are significant for equality, add tests for those too. Done.
PTAL hta.
lgtm % naming issue. I'm going to LGTM this, but I'd really prefer to add a comment in the .h file that the == operator is exposed for testing, even if we can't enforce it by #ifdefs. https://codereview.webrtc.org/2441543002/diff/20001/webrtc/stats/rtcstats_uni... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2441543002/diff/20001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:164: RTCTestStats undefined_diff_id("testId2", 123); "undefined" is a very poor name, because it looks like a reserved word (is so in many other languages). what about "undefined" -> "empty_stats" and "defined" -> "stats_with_all_values"? also _diff_ -> _different_ for clarity.
Description was changed from ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 ========== to ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 ==========
hbos@webrtc.org changed reviewers: - hbos@chromium.org
On 2016/10/25 10:30:30, hta-webrtc wrote: > I'm going to LGTM this, but I'd really prefer to add a comment in the .h file > that the == operator is exposed for testing, even if we can't enforce it by > #ifdefs. Done. https://codereview.webrtc.org/2441543002/diff/20001/webrtc/stats/rtcstats_uni... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2441543002/diff/20001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:164: RTCTestStats undefined_diff_id("testId2", 123); On 2016/10/25 10:30:30, hta-webrtc wrote: > "undefined" is a very poor name, because it looks like a reserved word (is so in > many other languages). > what about "undefined" -> "empty_stats" and "defined" -> > "stats_with_all_values"? > > also _diff_ -> _different_ for clarity. Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2441543002/#ps40001 (title: "Addressed comments and rebase with master")
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
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/18783)
Only failures are unrelated flaky test, landing with NOTRY.
Description was changed from ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 ========== to ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 NOTRY=True ==========
The CQ bit was checked by hbos@webrtc.org
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 ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 NOTRY=True ========== to ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 NOTRY=True ========== to ========== RTCStats equality operator added. This will be helpful in unittests to EXPECT_EQ reports. It should be a useful operator to have outside of testing as well. BUG=chromium:627816 NOTRY=True Committed: https://crrev.com/67c8bc4bf2cc10669e5cb7fd461469f30edcaa23 Cr-Commit-Position: refs/heads/master@{#14767} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/67c8bc4bf2cc10669e5cb7fd461469f30edcaa23 Cr-Commit-Position: refs/heads/master@{#14767} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
