Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780473002:
Fix UT failure by temporarily uncommenting (Closed)
|
Created:
3 years, 9 months ago by elad.alon Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix UT failure by temporarily uncommenting
BUG=webrtc:7322, webrtc:7405
Review-Url: https://codereview.webrtc.org/2780473002
Cr-Commit-Position: refs/heads/master@{#17393}
Committed: https://chromium.googlesource.com/external/webrtc/+/4e7645118ed4dcd43b2856f85720e72cd248771c
Patch Set 1 #Patch Set 2 : Added issue number to TODOs. #
Total comments: 2
Patch Set 3 : . #
Messages
Total messages: 28 (10 generated)
elad.alon@webrtc.org changed reviewers: + minyue@webrtc.org, sprang@webrtc.org
Odd that it would pass trybots and fail for Erik locally, but here's a quick fix while we look more into it, so that other people won't be blocked.
Description was changed from ========== Fix UT failure by temporarily uncommenting BUG=None ========== to ========== Fix UT failure by temporarily uncommenting BUG=webrtc:7322 ==========
On 2017/03/27 09:52:42, elad.alon wrote: > Odd that it would pass trybots and fail for Erik locally, but here's a quick fix > while we look more into it, so that other people won't be blocked. Is it safe to just remove the DCHECK()?
On 2017/03/27 09:55:20, minyue-webrtc wrote: > On 2017/03/27 09:52:42, elad.alon wrote: > > Odd that it would pass trybots and fail for Erik locally, but here's a quick > fix > > while we look more into it, so that other people won't be blocked. > > Is it safe to just remove the DCHECK()? I think so. See OnRecoverableUplinkPacketLossRate() for a similar issue.
would you file a bug and add the bug link to the comment? then lgtm
On 2017/03/27 10:02:05, minyue-webrtc wrote: > would you file a bug and add the bug link to the comment? then lgtm Done. Adding owners.
elad.alon@webrtc.org changed reviewers: + ossu@webrtc.org, solenberg@webrtc.org
Owners (Fredrick/Oskar), could you PTAL?
Question, btw - can/should I add more than one BUG to a CL?
On 2017/03/27 10:14:12, elad.alon wrote: > Question, btw - can/should I add more than one BUG to a CL? Yes, use "," to separate. But do we have two in this case?
On 2017/03/27 10:49:35, minyue-webrtc wrote: > On 2017/03/27 10:14:12, elad.alon wrote: > > Question, btw - can/should I add more than one BUG to a CL? > > Yes, use "," to separate. But do we have two in this case? We have the one Erik has mentioned, and the one you've asked me to file.
Description was changed from ========== Fix UT failure by temporarily uncommenting BUG=webrtc:7322 ========== to ========== Fix UT failure by temporarily uncommenting BUG=webrtc:7322, webrtc:7405 ==========
lgtm, but I'm not an owner
Is this to unbreak bots? Why not fix the test, or revert the CL causing it? https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:277: // TODO(elad.alon): This fails in UT; fix and uncomment. (WebRTC issue 7405.) Super nit: Referring to issues in this format is not so common in the code base (3 occurencies). It is more common to add a link to the bug tracker issue, i.e.: See https://bugs.chromium.org/p/webrtc/issues/detail?id=7405.
On 2017/03/27 14:30:29, the sun wrote: > Is this to unbreak bots? Why not fix the test, or revert the CL causing it? > > https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... > webrtc/audio/audio_send_stream.cc:277: // TODO(elad.alon): This fails in UT; fix > and uncomment. (WebRTC issue 7405.) > Super nit: Referring to issues in this format is not so common in the code base > (3 occurencies). It is more common to add a link to the bug tracker issue, i.e.: > See https://bugs.chromium.org/p/webrtc/issues/detail?id=7405. The CL doesn't need to get reverted, IMHO, because the thread-check was just piggy-backed on top of it. I don't think fixing the test would be completely straight-forward, so I'd prefer not to switch to working on that at the moment, schedule-wise. Wdys?
https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2780473002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:277: // TODO(elad.alon): This fails in UT; fix and uncomment. (WebRTC issue 7405.) On 2017/03/27 14:30:29, the sun wrote: > Super nit: Referring to issues in this format is not so common in the code base > (3 occurencies). It is more common to add a link to the bug tracker issue, i.e.: > See https://bugs.chromium.org/p/webrtc/issues/detail?id=7405. Will do.
lgtm
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, solenberg@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2780473002/#ps40001 (title: ".")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by elad.alon@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": 40001, "attempt_start_ts": 1490629389780180,
"parent_rev": "d701dfdeef231185b262bc9fe7024b6211ae96bf", "commit_rev":
"4e7645118ed4dcd43b2856f85720e72cd248771c"}
Message was sent while issue was closed.
Description was changed from ========== Fix UT failure by temporarily uncommenting BUG=webrtc:7322, webrtc:7405 ========== to ========== Fix UT failure by temporarily uncommenting BUG=webrtc:7322, webrtc:7405 Review-Url: https://codereview.webrtc.org/2780473002 Cr-Commit-Position: refs/heads/master@{#17393} Committed: https://chromium.googlesource.com/external/webrtc/+/4e7645118ed4dcd43b2856f85... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/4e7645118ed4dcd43b2856f85... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
