Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863363002:
🎱 Add test runner scripts for instrumentation tests (Closed)
|
Created:
4 years, 8 months ago by agrieve Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@add-isolates-to-build-files Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd test runner scripts for instrumentation tests
BUG=599919
NOTRY=True
TBR=perkj@webrtc.org
Patch Set 1 #
Total comments: 3
Patch Set 2 : revert talk/common.gypi #
Depends on Patchset: Messages
Total messages: 23 (10 generated)
Description was changed from ========== Add test runner scripts for instrumentation tests BUG=599919 ========== to ========== Add test runner scripts for instrumentation tests BUG=599919 ==========
agrieve@chromium.org changed reviewers: + kjellander@webrtc.org
On 2016/04/07 02:21:54, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:kjellander@webrtc.org
Thanks for doing this for us! I only got one comment https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi File talk/build/common.gypi (right): https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 talk/build/common.gypi:32: 'variables': { I don't think you need any changes in this file. It shouldn't be referenced outside the talk/ folder, which is something we're trying to get rid of entirely (as soon we have a replacement for the legacy objC APIs that still live in there). If you need these changes, they should be in webrtc/build/common.gypi. I was able to patch your CL, remove the changes in talk/build/common.gypi, generate projects for Android and build everything. With that, I got runner scripts for libjingle_peerconnection_android_unittest and AppRTCDemoTest generated in out/Release/bin
https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi File talk/build/common.gypi (right): https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 talk/build/common.gypi:32: 'variables': { On 2016/04/07 07:57:12, kjellander (webrtc) wrote: > I don't think you need any changes in this file. It shouldn't be referenced > outside the talk/ folder, which is something we're trying to get rid of entirely > (as soon we have a replacement for the legacy objC APIs that still live in > there). > > If you need these changes, they should be in webrtc/build/common.gypi. > I was able to patch your CL, remove the changes in talk/build/common.gypi, > generate projects for Android and build everything. > With that, I got runner scripts for libjingle_peerconnection_android_unittest > and AppRTCDemoTest generated in out/Release/bin Sorry, should have pointed out why I did this, as it really wasn't obvious. The issue is that webrtc/webrtc_examples.gyp includes talk/build/common.gypi rather than webrtc/build/common.gypi. Including both common.gypis resulted in a GYP error, and switching to webrtc/build/common.gypi resulted in build failures (lint warnings). I've updated it now to just set the test_runner_path on the target within webrtc_examples.gyp
looks good, but we need another fix since https://codereview.webrtc.org/1866123002/ doesn't work as expected (see comment). https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi File talk/build/common.gypi (right): https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 talk/build/common.gypi:32: 'variables': { On 2016/04/07 13:32:23, agrieve wrote: > On 2016/04/07 07:57:12, kjellander (webrtc) wrote: > > I don't think you need any changes in this file. It shouldn't be referenced > > outside the talk/ folder, which is something we're trying to get rid of > entirely > > (as soon we have a replacement for the legacy objC APIs that still live in > > there). > > > > If you need these changes, they should be in webrtc/build/common.gypi. > > I was able to patch your CL, remove the changes in talk/build/common.gypi, > > generate projects for Android and build everything. > > With that, I got runner scripts for libjingle_peerconnection_android_unittest > > and AppRTCDemoTest generated in out/Release/bin > > Sorry, should have pointed out why I did this, as it really wasn't obvious. > > The issue is that webrtc/webrtc_examples.gyp includes talk/build/common.gypi > rather than webrtc/build/common.gypi. Including both common.gypis resulted in a > GYP error, and switching to webrtc/build/common.gypi resulted in build failures > (lint warnings). Oh, I wasn't aware of this. I really should spend some time getting rid of the last parts of talk/build/common.gypi then. > I've updated it now to just set the test_runner_path on the target within > webrtc_examples.gyp This sounded like the way to go, but when I tested this (with a checkout that had https://codereview.webrtc.org/1866123002 synced) I see only see the generated script for AppRTCDemoTest get the right path: $ grep -r test_runner.py out/Release/bin/ out/Release/bin/run_audio_decoder_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_common_audio_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_test_support_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_modules_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_tools_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_webrtc_nonparallel_tests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_rtc_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_voice_engine_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_libjingle_peerconnection_android_unittest: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_system_wrappers_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_common_video_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_AppRTCDemoTest: test_runner_path = ResolvePath('../../../webrtc/build/android/test_runner.py') out/Release/bin/run_modules_tests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_webrtc_perf_tests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_video_engine_tests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_audio_codec_speed_tests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') out/Release/bin/run_peerconnection_unittests: test_runner_path = ResolvePath('../../../build/android/test_runner.py') I suspect that the variable set in https://codereview.webrtc.org/1866123002 is not in the right variable scope (classic GYP mess).
On 2016/04/07 17:56:15, kjellander (webrtc) wrote: > looks good, but we need another fix since > https://codereview.webrtc.org/1866123002/ doesn't work as expected (see > comment). > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi > File talk/build/common.gypi (right): > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 > talk/build/common.gypi:32: 'variables': { > On 2016/04/07 13:32:23, agrieve wrote: > > On 2016/04/07 07:57:12, kjellander (webrtc) wrote: > > > I don't think you need any changes in this file. It shouldn't be referenced > > > outside the talk/ folder, which is something we're trying to get rid of > > entirely > > > (as soon we have a replacement for the legacy objC APIs that still live in > > > there). > > > > > > If you need these changes, they should be in webrtc/build/common.gypi. > > > I was able to patch your CL, remove the changes in talk/build/common.gypi, > > > generate projects for Android and build everything. > > > With that, I got runner scripts for > libjingle_peerconnection_android_unittest > > > and AppRTCDemoTest generated in out/Release/bin > > > > Sorry, should have pointed out why I did this, as it really wasn't obvious. > > > > The issue is that webrtc/webrtc_examples.gyp includes talk/build/common.gypi > > rather than webrtc/build/common.gypi. Including both common.gypis resulted in > a > > GYP error, and switching to webrtc/build/common.gypi resulted in build > failures > > (lint warnings). > > Oh, I wasn't aware of this. I really should spend some time getting rid of the > last parts of talk/build/common.gypi then. > > > I've updated it now to just set the test_runner_path on the target within > > webrtc_examples.gyp > > This sounded like the way to go, but when I tested this (with a checkout that > had https://codereview.webrtc.org/1866123002 synced) I see only see the > generated script for AppRTCDemoTest get the right path: > $ grep -r test_runner.py out/Release/bin/ > out/Release/bin/run_audio_decoder_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_common_audio_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_test_support_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_modules_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_tools_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_webrtc_nonparallel_tests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_rtc_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_voice_engine_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_libjingle_peerconnection_android_unittest: test_runner_path > = ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_system_wrappers_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_common_video_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_AppRTCDemoTest: test_runner_path = > ResolvePath('../../../webrtc/build/android/test_runner.py') > out/Release/bin/run_modules_tests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_webrtc_perf_tests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_video_engine_tests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_audio_codec_speed_tests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > out/Release/bin/run_peerconnection_unittests: test_runner_path = > ResolvePath('../../../build/android/test_runner.py') > > I suspect that the variable set in https://codereview.webrtc.org/1866123002 is > not in the right variable scope (classic GYP mess). Hmm, just tried again after a sync and it's still looking good on my machine. Are you sure you aren't looking at stale run_* files by chance?
On 2016/04/07 19:52:11, agrieve wrote: > On 2016/04/07 17:56:15, kjellander (webrtc) wrote: > > looks good, but we need another fix since > > https://codereview.webrtc.org/1866123002/ doesn't work as expected (see > > comment). > > > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi > > File talk/build/common.gypi (right): > > > > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 > > talk/build/common.gypi:32: 'variables': { > > On 2016/04/07 13:32:23, agrieve wrote: > > > On 2016/04/07 07:57:12, kjellander (webrtc) wrote: > > > > I don't think you need any changes in this file. It shouldn't be > referenced > > > > outside the talk/ folder, which is something we're trying to get rid of > > > entirely > > > > (as soon we have a replacement for the legacy objC APIs that still live in > > > > there). > > > > > > > > If you need these changes, they should be in webrtc/build/common.gypi. > > > > I was able to patch your CL, remove the changes in talk/build/common.gypi, > > > > generate projects for Android and build everything. > > > > With that, I got runner scripts for > > libjingle_peerconnection_android_unittest > > > > and AppRTCDemoTest generated in out/Release/bin > > > > > > Sorry, should have pointed out why I did this, as it really wasn't obvious. > > > > > > The issue is that webrtc/webrtc_examples.gyp includes talk/build/common.gypi > > > rather than webrtc/build/common.gypi. Including both common.gypis resulted > in > > a > > > GYP error, and switching to webrtc/build/common.gypi resulted in build > > failures > > > (lint warnings). > > > > Oh, I wasn't aware of this. I really should spend some time getting rid of the > > last parts of talk/build/common.gypi then. > > > > > I've updated it now to just set the test_runner_path on the target within > > > webrtc_examples.gyp > > > > This sounded like the way to go, but when I tested this (with a checkout that > > had https://codereview.webrtc.org/1866123002 synced) I see only see the > > generated script for AppRTCDemoTest get the right path: > > $ grep -r test_runner.py out/Release/bin/ > > out/Release/bin/run_audio_decoder_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_common_audio_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_test_support_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_modules_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_tools_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_webrtc_nonparallel_tests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_rtc_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_voice_engine_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_libjingle_peerconnection_android_unittest: > test_runner_path > > = ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_system_wrappers_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_common_video_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_AppRTCDemoTest: test_runner_path = > > ResolvePath('../../../webrtc/build/android/test_runner.py') > > out/Release/bin/run_modules_tests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_webrtc_perf_tests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_video_engine_tests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_audio_codec_speed_tests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > out/Release/bin/run_peerconnection_unittests: test_runner_path = > > ResolvePath('../../../build/android/test_runner.py') > > > > I suspect that the variable set in https://codereview.webrtc.org/1866123002 is > > not in the right variable scope (classic GYP mess). > > Hmm, just tried again after a sync and it's still looking good on my machine. > Are you sure you aren't looking at stale run_* files by chance? You're right. It does work. I was only cleaning out/Release/bin, which turns out was not enough. When I cleaned all of out/Release and rebuilt, all run-scripts looks correct as you say. Sorry about that. Then this lgtm!
Description was changed from ========== Add test runner scripts for instrumentation tests BUG=599919 ========== to ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True ==========
On 2016/04/08 05:06:55, kjellander (webrtc) wrote: > On 2016/04/07 19:52:11, agrieve wrote: > > On 2016/04/07 17:56:15, kjellander (webrtc) wrote: > > > looks good, but we need another fix since > > > https://codereview.webrtc.org/1866123002/ doesn't work as expected (see > > > comment). > > > > > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi > > > File talk/build/common.gypi (right): > > > > > > > > > https://codereview.webrtc.org/1863363002/diff/1/talk/build/common.gypi#newcode32 > > > talk/build/common.gypi:32: 'variables': { > > > On 2016/04/07 13:32:23, agrieve wrote: > > > > On 2016/04/07 07:57:12, kjellander (webrtc) wrote: > > > > > I don't think you need any changes in this file. It shouldn't be > > referenced > > > > > outside the talk/ folder, which is something we're trying to get rid of > > > > entirely > > > > > (as soon we have a replacement for the legacy objC APIs that still live > in > > > > > there). > > > > > > > > > > If you need these changes, they should be in webrtc/build/common.gypi. > > > > > I was able to patch your CL, remove the changes in > talk/build/common.gypi, > > > > > generate projects for Android and build everything. > > > > > With that, I got runner scripts for > > > libjingle_peerconnection_android_unittest > > > > > and AppRTCDemoTest generated in out/Release/bin > > > > > > > > Sorry, should have pointed out why I did this, as it really wasn't > obvious. > > > > > > > > The issue is that webrtc/webrtc_examples.gyp includes > talk/build/common.gypi > > > > rather than webrtc/build/common.gypi. Including both common.gypis resulted > > in > > > a > > > > GYP error, and switching to webrtc/build/common.gypi resulted in build > > > failures > > > > (lint warnings). > > > > > > Oh, I wasn't aware of this. I really should spend some time getting rid of > the > > > last parts of talk/build/common.gypi then. > > > > > > > I've updated it now to just set the test_runner_path on the target within > > > > webrtc_examples.gyp > > > > > > This sounded like the way to go, but when I tested this (with a checkout > that > > > had https://codereview.webrtc.org/1866123002 synced) I see only see the > > > generated script for AppRTCDemoTest get the right path: > > > $ grep -r test_runner.py out/Release/bin/ > > > out/Release/bin/run_audio_decoder_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_common_audio_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_test_support_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_modules_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_tools_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_webrtc_nonparallel_tests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_rtc_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_voice_engine_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_libjingle_peerconnection_android_unittest: > > test_runner_path > > > = ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_system_wrappers_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_common_video_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_AppRTCDemoTest: test_runner_path = > > > ResolvePath('../../../webrtc/build/android/test_runner.py') > > > out/Release/bin/run_modules_tests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_webrtc_perf_tests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_video_engine_tests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_audio_codec_speed_tests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > out/Release/bin/run_peerconnection_unittests: test_runner_path = > > > ResolvePath('../../../build/android/test_runner.py') > > > > > > I suspect that the variable set in https://codereview.webrtc.org/1866123002 > is > > > not in the right variable scope (classic GYP mess). > > > > Hmm, just tried again after a sync and it's still looking good on my machine. > > Are you sure you aren't looking at stale run_* files by chance? > > You're right. It does work. I was only cleaning out/Release/bin, which turns out > was not enough. When I cleaned all of out/Release and rebuilt, all run-scripts > looks correct as you say. > Sorry about that. > > Then this lgtm! I added NOTRY=True and triggered a few trybots to save some of our scarce resources.
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4685)
Description was changed from ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True ========== to ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True TBR=perkj@webrtc.org ==========
kjellander@webrtc.org changed reviewers: + perkj@webrtc.org
On 2016/04/08 13:50:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4685) TBRing perkj@webrtc.org for owners rubberstamp for webrtc/api
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863363002/20001
Message was sent while issue was closed.
Description was changed from ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True TBR=perkj@webrtc.org ========== to ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True TBR=perkj@webrtc.org Committed: https://crrev.com/babf8ee78c87f8965f2883b09d3dede6e3e3e247 Cr-Commit-Position: refs/heads/master@{#12294} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/babf8ee78c87f8965f2883b09d3dede6e3e3e247 Cr-Commit-Position: refs/heads/master@{#12294}
Message was sent while issue was closed.
Description was changed from ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True TBR=perkj@webrtc.org Committed: https://crrev.com/babf8ee78c87f8965f2883b09d3dede6e3e3e247 Cr-Commit-Position: refs/heads/master@{#12294} ========== to ========== Add test runner scripts for instrumentation tests BUG=599919 NOTRY=True TBR=perkj@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
