Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845013003:
NetEq tests: BUILD target reorg (Closed)
|
Created:
3 years, 7 months ago by hlundin-webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, minyue-webrtc, the sun, aluebs-webrtc, bjornv1, mbonadei Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNetEq tests: BUILD target reorg
In this CL, the neteq_unittest_tools target is split in two separate
targets. One still called neteq_tools which does not set
testonly=true and that includes code related to audio input,
replacement audio and fake decoding. The other target called
neteq_test_tools contains the remaining files, and is
still under testonly=true.
Other renames:
neteq_test_tools -> neteq_test_tools_deprecated
neteq_test_minimal -> neteq_tools_minimal
Cyclic dependencies were also cleaned up.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_compile_rel_ng,linux_chromium_compile_dbg_ng
BUG=webrtc:7467, webrtc:6828
Review-Url: https://codereview.webrtc.org/2845013003
Cr-Commit-Position: refs/heads/master@{#17921}
Committed: https://chromium.googlesource.com/external/webrtc/+/b637a94b63e45be2c2e6f599ea9b14293f3fd321
Patch Set 1 #
Total comments: 4
Patch Set 2 : Renaming targets #
Total comments: 1
Patch Set 3 : Fixing win compile errors #
Depends on Patchset: Messages
Total messages: 29 (12 generated)
Patchset #1 (id:1) has been deleted
henrik.lundin@webrtc.org changed reviewers: + kjellander@webrtc.org
kjellander@, PTAL.
Drive-by comment: A build target called "neteq_unittest_tools" should probably not be included in any non-tests, so there should be no reason to mark it testonly=true. Conversely, if it needs to be included in non-tests, it should probably not be called "neteq_unittest_tools".
On 2017/04/27 09:30:39, kwiberg-webrtc wrote: > Drive-by comment: A build target called "neteq_unittest_tools" should probably > not be included in any non-tests, so there should be no reason to mark it > testonly=true. Conversely, if it needs to be included in non-tests, it should > probably not be called "neteq_unittest_tools". neteq_unittest_tools will be used in tools that do not strictly count as tests; event_log_visualizer_utils in particular. I can rename it to neteq_tools if that makes you happier. Should probably also rename neteq_test_minimal to neteq_tools_minmal then. WDYT?
On 2017/04/27 09:38:57, hlundin-webrtc wrote: > On 2017/04/27 09:30:39, kwiberg-webrtc wrote: > > Drive-by comment: A build target called "neteq_unittest_tools" should probably > > not be included in any non-tests, so there should be no reason to mark it > > testonly=true. Conversely, if it needs to be included in non-tests, it should > > probably not be called "neteq_unittest_tools". > > neteq_unittest_tools will be used in tools that do not strictly count as tests; > event_log_visualizer_utils in particular. I can rename it to neteq_tools if that > makes you happier. Should probably also rename neteq_test_minimal to > neteq_tools_minmal then. WDYT? sgtm
https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1563: rtc_source_set("neteq_unittest_tools") { I'd like to avoid having a target with "unittest" in the name doesn't have testonly=true set, especially as Mirko and I have been looking at these particular targets yesterday in our attempts to enable GN check for webrtc/test without breaking Chromium (the fuzzers are making it hard). How about naming this one something like neteq_input_tools or something like that? https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1595: rtc_source_set("neteq_unittest_tools_extended") { This name doesn't say much about what's left in here. Can you come up with a better name? Or just use neteq_test_tools (I prefer the word 'test' over 'unittests').
On 2017/04/27 10:14:24, kwiberg-webrtc wrote: > On 2017/04/27 09:38:57, hlundin-webrtc wrote: > > On 2017/04/27 09:30:39, kwiberg-webrtc wrote: > > > Drive-by comment: A build target called "neteq_unittest_tools" should > probably > > > not be included in any non-tests, so there should be no reason to mark it > > > testonly=true. Conversely, if it needs to be included in non-tests, it > should > > > probably not be called "neteq_unittest_tools". > > > > neteq_unittest_tools will be used in tools that do not strictly count as > tests; > > event_log_visualizer_utils in particular. I can rename it to neteq_tools if > that > > makes you happier. Should probably also rename neteq_test_minimal to > > neteq_tools_minmal then. WDYT? > > sgtm FYI: I didn't see these messages before I wrote my comments, but they have the same concern. I think it's fine for any target to have testonly=true as long as it's not production code. It's actually something I'd encourage for all our "tools". The fuzzer tests have testonly=true set, as the fuzzer_test template is wrapping the test target type (https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzer_test.gni?rcl=b4...).
Attempt #2. WDYT? https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1563: rtc_source_set("neteq_unittest_tools") { On 2017/04/27 10:34:52, kjellander_webrtc wrote: > I'd like to avoid having a target with "unittest" in the name doesn't have > testonly=true set, especially as Mirko and I have been looking at these > particular targets yesterday in our attempts to enable GN check for webrtc/test > without breaking Chromium (the fuzzers are making it hard). > > How about naming this one something like neteq_input_tools or something like > that? It is not the only target that contains "input" things. I Changed it to neteq_tools instead. https://codereview.webrtc.org/2845013003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1595: rtc_source_set("neteq_unittest_tools_extended") { On 2017/04/27 10:34:52, kjellander_webrtc wrote: > This name doesn't say much about what's left in here. Can you come up with a > better name? > Or just use neteq_test_tools (I prefer the word 'test' over 'unittests'). I went for neteq_test_tools. Only problem is that it was already taken by another target, but that is mainly old cruft, so I renamed that one to *_deprecated.
On 2017/04/27 10:37:56, kjellander_webrtc wrote: > On 2017/04/27 10:14:24, kwiberg-webrtc wrote: > > On 2017/04/27 09:38:57, hlundin-webrtc wrote: > > > On 2017/04/27 09:30:39, kwiberg-webrtc wrote: > > > > Drive-by comment: A build target called "neteq_unittest_tools" should > > probably > > > > not be included in any non-tests, so there should be no reason to mark it > > > > testonly=true. Conversely, if it needs to be included in non-tests, it > > should > > > > probably not be called "neteq_unittest_tools". > > > > > > neteq_unittest_tools will be used in tools that do not strictly count as > > tests; > > > event_log_visualizer_utils in particular. I can rename it to neteq_tools if > > that > > > makes you happier. Should probably also rename neteq_test_minimal to > > > neteq_tools_minmal then. WDYT? > > > > sgtm > > FYI: I didn't see these messages before I wrote my comments, but they have the > same concern. > I think it's fine for any target to have testonly=true as long as it's not > production code. It's actually something I'd encourage for all our "tools". > The fuzzer tests have testonly=true set, as the fuzzer_test template is wrapping > the test target type > (https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzer_test.gni?rcl=b4...). I agree with kwiberg@ and kjellander@ about naming, it is just a name but if you have no context you can think it is somehow related to testing even if it is just used by one fuzzer. Yes, we were looking at these targets yesterday because //webrtc/test/fuzzers:neteq_rtp_fuzzer should explicitly (in Gn) depend on //webrtc/modules/audio_coding:neteq_unittest_tools. neteq_unittest_tools is inside a condition on rtc_include_tests and since chromium uses our fuzzers but builds with rtc_enable_tests=false I had to move the target out of the condition. This caused another issue with //third_party/gflags which is not strictly related to this CL. I will wait for this to land before continuing my work on enabling 'gn check'.
On 2017/04/27 11:53:35, mbonadei wrote: > On 2017/04/27 10:37:56, kjellander_webrtc wrote: > > On 2017/04/27 10:14:24, kwiberg-webrtc wrote: > > > On 2017/04/27 09:38:57, hlundin-webrtc wrote: > > > > On 2017/04/27 09:30:39, kwiberg-webrtc wrote: > > > > > Drive-by comment: A build target called "neteq_unittest_tools" should > > > probably > > > > > not be included in any non-tests, so there should be no reason to mark > it > > > > > testonly=true. Conversely, if it needs to be included in non-tests, it > > > should > > > > > probably not be called "neteq_unittest_tools". > > > > > > > > neteq_unittest_tools will be used in tools that do not strictly count as > > > tests; > > > > event_log_visualizer_utils in particular. I can rename it to neteq_tools > if > > > that > > > > makes you happier. Should probably also rename neteq_test_minimal to > > > > neteq_tools_minmal then. WDYT? > > > > > > sgtm > > > > FYI: I didn't see these messages before I wrote my comments, but they have the > > same concern. > > I think it's fine for any target to have testonly=true as long as it's not > > production code. It's actually something I'd encourage for all our "tools". > > The fuzzer tests have testonly=true set, as the fuzzer_test template is > wrapping > > the test target type > > > (https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzer_test.gni?rcl=b4...). > > I agree with kwiberg@ and kjellander@ about naming, it is just a name but > if you have no context you can think it is somehow related to testing > even if it is just used by one fuzzer. It will be used by more than just the fuzzer in the near future. That is why I'm doing this now. > > Yes, we were looking at these targets yesterday because > //webrtc/test/fuzzers:neteq_rtp_fuzzer should explicitly (in Gn) depend on > //webrtc/modules/audio_coding:neteq_unittest_tools. > > neteq_unittest_tools is inside a condition on rtc_include_tests and since > chromium uses our fuzzers but builds with rtc_enable_tests=false I had to > move the target out of the condition. > This caused another issue with //third_party/gflags which is not strictly > related to this CL. > > I will wait for this to land before continuing my work on enabling > 'gn check'.
I like it if the trybots do! lgtm
Description was changed from ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_unittest_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_unittest_tools_extended contains the remaining files, and is still under testonly=true. Cyclic dependencies were also cleaned up. BUG=webrtc:7467,webrtc:6828 ========== to ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_unittest_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_unittest_tools_extended contains the remaining files, and is still under testonly=true. Cyclic dependencies were also cleaned up. BUG=webrtc:7467,webrtc:6828 ==========
On 2017/04/27 12:37:22, kjellander_webrtc wrote: > I like it if the trybots do! > > lgtm CL description needs an update too.
Description was changed from ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_unittest_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_unittest_tools_extended contains the remaining files, and is still under testonly=true. Cyclic dependencies were also cleaned up. BUG=webrtc:7467,webrtc:6828 ========== to ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_test_tools contains the remaining files, and is still under testonly=true. Other renames: neteq_test_tools -> neteq_test_tools_deprecated neteq_test_minimal -> neteq_tools_minimal Cyclic dependencies were also cleaned up. BUG=webrtc:7467,webrtc:6828 ==========
Description was changed from ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_test_tools contains the remaining files, and is still under testonly=true. Other renames: neteq_test_tools -> neteq_test_tools_deprecated neteq_test_minimal -> neteq_tools_minimal Cyclic dependencies were also cleaned up. BUG=webrtc:7467,webrtc:6828 ========== to ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_test_tools contains the remaining files, and is still under testonly=true. Other renames: neteq_test_tools -> neteq_test_tools_deprecated neteq_test_minimal -> neteq_tools_minimal Cyclic dependencies were also cleaned up. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_compile_rel_ng,linux_chromium_compile_dbg_ng BUG=webrtc:7467,webrtc:6828 ==========
The CQ bit was checked by henrik.lundin@webrtc.org
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: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8592)
kjellander@google.com changed reviewers: + kjellander@google.com
Commented regarding trybot failure. It would be nice to get this landed to unblock Mirko's work. https://codereview.webrtc.org/2845013003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2845013003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1565: "neteq/tools/fake_decode_from_file.cc", I can't explain why, but it seems you need to either fix the warnings or add: if (is_win) { configs += [ "//build/config/compiler:no_size_t_to_int_warning" ] } to this target now, due to fake_decode_from_file.cc.
On 2017/04/28 06:56:18, kjellander (google.com) wrote: > Commented regarding trybot failure. It would be nice to get this landed to > unblock Mirko's work. > > https://codereview.webrtc.org/2845013003/diff/40001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/BUILD.gn (right): > > https://codereview.webrtc.org/2845013003/diff/40001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/BUILD.gn:1565: > "neteq/tools/fake_decode_from_file.cc", > I can't explain why, but it seems you need to either fix the warnings or add: > > if (is_win) { > configs += [ "//build/config/compiler:no_size_t_to_int_warning" ] > } > > to this target now, due to fake_decode_from_file.cc. Fixing this in a new patch set. Waiting for bots now.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2845013003/#ps60001 (title: "Fixing win compile errors")
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": 1493364054310630,
"parent_rev": "777cb7b4c64712140cfae000ccf877907e6a2454", "commit_rev":
"b637a94b63e45be2c2e6f599ea9b14293f3fd321"}
Message was sent while issue was closed.
Description was changed from ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_test_tools contains the remaining files, and is still under testonly=true. Other renames: neteq_test_tools -> neteq_test_tools_deprecated neteq_test_minimal -> neteq_tools_minimal Cyclic dependencies were also cleaned up. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_compile_rel_ng,linux_chromium_compile_dbg_ng BUG=webrtc:7467,webrtc:6828 ========== to ========== NetEq tests: BUILD target reorg In this CL, the neteq_unittest_tools target is split in two separate targets. One still called neteq_tools which does not set testonly=true and that includes code related to audio input, replacement audio and fake decoding. The other target called neteq_test_tools contains the remaining files, and is still under testonly=true. Other renames: neteq_test_tools -> neteq_test_tools_deprecated neteq_test_minimal -> neteq_tools_minimal Cyclic dependencies were also cleaned up. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_compile_rel_ng,linux_chromium_compile_dbg_ng BUG=webrtc:7467,webrtc:6828 Review-Url: https://codereview.webrtc.org/2845013003 Cr-Commit-Position: refs/heads/master@{#17921} Committed: https://chromium.googlesource.com/external/webrtc/+/b637a94b63e45be2c2e6f599e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/b637a94b63e45be2c2e6f599e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
