Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746763005:
Fixing a few tests for the upcoming Opus 1.2-alpha. (Closed)
|
Created:
3 years, 9 months ago by minyue-webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing a few tests for the upcoming Opus 1.2-alpha.
BUG=b/35415318
NOTRY=True
Review-Url: https://codereview.webrtc.org/2746763005
Cr-Commit-Position: refs/heads/master@{#17234}
Committed: https://chromium.googlesource.com/external/webrtc/+/a613eb6bff33b62fef0589b2e4e587380ae23411
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixing redefinition #
Total comments: 2
Patch Set 3 : fixing 2 #
Total comments: 1
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Fixing a few tests for the upcoming new Opus. BUG=None ========== to ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=None ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kjellander@webrtc.org
Patchset #2 (id:20001) has been deleted
PTAL this CL. https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1669: TEST_F(AcmChangeBitRateOldApi, MAYBE_Opus_48khz_20ms_10kbps) { note that MAYBE_Opus_48khz_20ms_10kbps is redefined. I hope it is ok.
Description was changed from ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=None ========== to ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 ==========
kjellander@google.com changed reviewers: + kjellander@google.com
https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1669: TEST_F(AcmChangeBitRateOldApi, MAYBE_Opus_48khz_20ms_10kbps) { On 2017/03/14 14:18:14, minyue-webrtc(OOO_until_Mar19) wrote: > note that MAYBE_Opus_48khz_20ms_10kbps is redefined. I hope it is ok. Run a few trybots to find out. I think redefinitions are considered compile errors.
On 2017/03/14 14:36:47, kjellander (google.com) wrote: > https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... > File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): > > https://codereview.webrtc.org/2746763005/diff/1/webrtc/modules/audio_coding/a... > webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1669: > TEST_F(AcmChangeBitRateOldApi, MAYBE_Opus_48khz_20ms_10kbps) { > On 2017/03/14 14:18:14, minyue-webrtc(OOO_until_Mar19) wrote: > > note that MAYBE_Opus_48khz_20ms_10kbps is redefined. I hope it is ok. > > Run a few trybots to find out. I think redefinitions are considered compile > errors. I'll let you sort out the compilation errors before reviewing.
sorted out. good time to review. https://codereview.webrtc.org/2746763005/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2746763005/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1479: #undef MAYBE_Opus_stereo_20ms I think it is generally good to undef, so I added it at all places this CL relates
kjellander@webrtc.org changed reviewers: - kjellander@google.com
https://codereview.webrtc.org/2746763005/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2746763005/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1479: #undef MAYBE_Opus_stereo_20ms On 2017/03/14 16:29:27, minyue-webrtc(OOO_until_Mar19) wrote: > I think it is generally good to undef, so I added it at all places this CL > relates I've never seen this so let's not start doing that. Can't you just rename one of the test cases so they're unique instead?
yes, of course https://codereview.webrtc.org/2746763005/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2746763005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1665: #define MAYBE_Opus_48khz_20ms_10kbps_2 DISABLED_Opus_48khz_20ms_10kbps since it is a local macro, a slightly ugly name like this may not be too bad.
Lgtm but please run some trybot to be sure
On 2017/03/14 17:40:59, kjellander_webrtc wrote: > Lgtm but please run some trybot to be sure And use NOTRY=True for landing...
Description was changed from ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 ========== to ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 NOTRY=true ==========
Description was changed from ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 NOTRY=true ========== to ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 NOTRY=True ==========
The CQ bit was checked by minyue@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": 60001, "attempt_start_ts": 1489527037851240,
"parent_rev": "275e2099abe0aea4db7b89f1abeae264574e5afa", "commit_rev":
"a613eb6bff33b62fef0589b2e4e587380ae23411"}
Message was sent while issue was closed.
Description was changed from ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 NOTRY=True ========== to ========== Fixing a few tests for the upcoming Opus 1.2-alpha. BUG=b/35415318 NOTRY=True Review-Url: https://codereview.webrtc.org/2746763005 Cr-Commit-Position: refs/heads/master@{#17234} Committed: https://chromium.googlesource.com/external/webrtc/+/a613eb6bff33b62fef0589b2e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/a613eb6bff33b62fef0589b2e...
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
