Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632323003:
Script to start stubbed loopback video test with Espresso (Closed)
DescriptionScript to start stubbed loopback video test with Espresso
BUG=webrtc:7034
Review-Url: https://codereview.webrtc.org/2632323003
Cr-Commit-Position: refs/heads/master@{#16216}
Committed: https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc97525e17bbf3d1354c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Using os.path.join for path concatenation instead of + #
Total comments: 31
Patch Set 3 : Improved path handling and other review fixes #
Total comments: 6
Patch Set 4 : Fixed review comments #
Total comments: 9
Patch Set 5 : Style fixes and sys.executable used to execute python scripts #Messages
Total messages: 24 (8 generated)
mandermo@webrtc.org changed reviewers: + ehmaldonado@webrtc.org, jansson@webrtc.org, kjellander@webrtc.org
The script that will run in the CI environment to start the loopback test with stubbed video and do the video quality comparison.
https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:48: if not build_dir_x86: Why not do the same with build_dir_android? https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:49: build_dir_x86 = temp_dir + '/LocalBuild' Maybe use rather os.path.join() here and below to join paths? https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:107: shutil.rmtree(temp_dir) Just curious: Why request a temp_dir as a flag if the results won't be saved?
https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:48: if not build_dir_x86: On 2017/01/17 16:02:36, ehmaldonado_webrtc wrote: > Why not do the same with build_dir_android? I got the impression that in the CI the android version is already built, while the x86 version is not. https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:49: build_dir_x86 = temp_dir + '/LocalBuild' On 2017/01/17 16:02:36, ehmaldonado_webrtc wrote: > Maybe use rather os.path.join() here and below to join paths? Done. https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:107: shutil.rmtree(temp_dir) On 2017/01/17 16:02:36, ehmaldonado_webrtc wrote: > Just curious: Why request a temp_dir as a flag if the results won't be saved? Maybe there is no need actually, I thought maybe that some CI environment needed exact control of where to save temporaries. We can check with kjellander@ what he thinks.
https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/1/webrtc/tools/test_stubbed_loo... webrtc/tools/test_stubbed_loopback_video.py:107: shutil.rmtree(temp_dir) On 2017/01/18 10:21:24, mandermo wrote: > On 2017/01/17 16:02:36, ehmaldonado_webrtc wrote: > > Just curious: Why request a temp_dir as a flag if the results won't be saved? > > Maybe there is no need actually, I thought maybe that some CI environment needed > exact control of where to save temporaries. We can check with kjellander@ what > he thinks. I thought it could be useful to pass ${ISOLATED_OUTDIR} as in https://cs.chromium.org/chromium/src/tools/mb/mb.py?rcl=0&l=1100 for things like this, to get the stats files archived (and also in this case the built frame analyzer binary). We might want to avoid archiving the reference_video_640x360_30fps.yuv for each build though, since it would be a lot of waste archiving an identical file each time. The test_video.y4m is interesting though. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:10: import argparse Please add a module docstring that briefly describes what this script does and what its expectations are on the environment and flags passed. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: WEBRTC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '../../')) The way to write this is: os.path.normpath(os.path.join(SCRIPT_DIR, os.pardir, os.pardir)) https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: WEBRTC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '../../')) Call this SRC_DIR to avoid confusion (there's a webrtc/ dir inside the src/ dir). https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:23: parser.add_argument('--source_dir', This flag doesn't seem to be needed if we just use SRC_DIR above (it's the same dir). That however requires a full checkout on the machine that's executing the test, which is true today but might not be in the future (but let's work on isolating this in a follow-up CL when/if needed in the future instead). So just remove this flag and use SRC_DIR instead. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:32: args = parser.parse_args() Please add parser.error() messages to error out in case a mandatory flag is missing (I guess all should be mandatory?) https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:34: +1 blank line before top-level functions. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:54: download_script = os.path.join(source_dir, Please introduce a constant at the top of the file: TOOLCHAIN_DIR = os.path.join(SRC_DIR, 'tools-webrtc', 'video_quality_toolchain') and use it below to make the code more readable. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:55: 'tools-webrtc/video_quality_toolchain/download.py') Avoid all / and just use multiple string arguments to os.path.join instead. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:65: subprocess.check_call(['adb', 'pull', '/sdcard/output.y4m', test_video]) Please pass the filename (output.y4m) as a flag, we want to minimize hardcoded duplicated names in this script. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:75: # Convert the test video. drop this comment, it doesn't add any value. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:79: 'resources/reference_video_640x360_30fps.y4m') Please pass this string as a flag instead of hardcoding a name here which will break if the file is renamed/moved. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:84: # Convert the reference video. This comment doesn't tell anything the code isn't. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:111: +1 blank line before top-level functions and statements.
https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:10: import argparse On 2017/01/19 13:55:34, kjellander_webrtc wrote: > Please add a module docstring that briefly describes what this script does and > what its expectations are on the environment and flags passed. Have added a comment now. Anything other format or content I need in it? https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: WEBRTC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '../../')) On 2017/01/19 13:55:34, kjellander_webrtc wrote: > Call this SRC_DIR to avoid confusion (there's a webrtc/ dir inside the src/ > dir). Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: WEBRTC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '../../')) On 2017/01/19 13:55:34, kjellander_webrtc wrote: > The way to write this is: > os.path.normpath(os.path.join(SCRIPT_DIR, os.pardir, os.pardir)) Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:23: parser.add_argument('--source_dir', On 2017/01/19 13:55:34, kjellander_webrtc wrote: > This flag doesn't seem to be needed if we just use SRC_DIR above (it's the same > dir). That however requires a full checkout on the machine that's executing the > test, which is true today but might not be in the future (but let's work on > isolating this in a follow-up CL when/if needed in the future instead). > > So just remove this flag and use SRC_DIR instead. This is an optional argument. It was useful when I was developing to be able to put the file somewhere else and to be able to inject the the path to the source dir, which is allowed now. If it is not given then it will use the default which is give an as an argument to the add_argument() call. Should I remove the argument and use SRC_DIR directly instead? https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:32: args = parser.parse_args() On 2017/01/19 13:55:34, kjellander_webrtc wrote: > Please add parser.error() messages to error out in case a mandatory flag is > missing (I guess all should be mandatory?) Why should they all be mandatory? Only build_dir_android is currently mandatory and is a positional argument, maybe it would be more consistent to add it to add_argument() as "--build_dir_android" instead of as "build_dir_android" and verify it later and raise an error if it is provided. If it is not given this happens: mandermo@mandermo:~/code/webrtc/src$ webrtc/tools/test_stubbed_loopback_video.py usage: test_stubbed_loopback_video.py [-h] [--source_dir SOURCE_DIR] [--build_dir_x86 BUILD_DIR_X86] [--temp_dir TEMP_DIR] build_dir_android test_stubbed_loopback_video.py: error: too few arguments https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:34: On 2017/01/19 13:55:35, kjellander_webrtc wrote: > +1 blank line before top-level functions. Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:54: download_script = os.path.join(source_dir, On 2017/01/19 13:55:35, kjellander_webrtc wrote: > Please introduce a constant at the top of the file: > TOOLCHAIN_DIR = os.path.join(SRC_DIR, 'tools-webrtc', 'video_quality_toolchain') > and use it below to make the code more readable. Have introduced a local variable toolchain_dir. If remove the --source_dir argument, then I can make it global. Should I? https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:55: 'tools-webrtc/video_quality_toolchain/download.py') On 2017/01/19 13:55:35, kjellander_webrtc wrote: > Avoid all / and just use multiple string arguments to os.path.join instead. Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:65: subprocess.check_call(['adb', 'pull', '/sdcard/output.y4m', test_video]) On 2017/01/19 13:55:35, kjellander_webrtc wrote: > Please pass the filename (output.y4m) as a flag, we want to minimize hardcoded > duplicated names in this script. As a flag to the script? This path is already hard coded in https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests.... Should we communicate that path to ConnectActivityStubbedInputOutputTest? I thought we did not want that. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:75: # Convert the test video. On 2017/01/19 13:55:35, kjellander_webrtc wrote: > drop this comment, it doesn't add any value. Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:79: 'resources/reference_video_640x360_30fps.y4m') On 2017/01/19 13:55:35, kjellander_webrtc wrote: > Please pass this string as a flag instead of hardcoding a name here which will > break if the file is renamed/moved. As flag to this script? Can be good with flexibility. But thought we decided against flexibility with regards to the reference video, since we wanted to hard code reference_video_640x360_30fps.y4m into several places already, like https://cs.chromium.org/chromium/src/third_party/webrtc/examples/BUILD.gn?q=r... and https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests.... https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:84: # Convert the reference video. On 2017/01/19 13:55:35, kjellander_webrtc wrote: > This comment doesn't tell anything the code isn't. Done. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:111: On 2017/01/19 13:55:34, kjellander_webrtc wrote: > +1 blank line before top-level functions and statements. Done.
https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:23: parser.add_argument('--source_dir', On 2017/01/19 16:46:42, mandermo wrote: > On 2017/01/19 13:55:34, kjellander_webrtc wrote: > > This flag doesn't seem to be needed if we just use SRC_DIR above (it's the > same > > dir). That however requires a full checkout on the machine that's executing > the > > test, which is true today but might not be in the future (but let's work on > > isolating this in a follow-up CL when/if needed in the future instead). > > > > So just remove this flag and use SRC_DIR instead. > > This is an optional argument. It was useful when I was developing to be able to > put the file somewhere else and to be able to inject the the path to the source > dir, which is allowed now. If it is not given then it will use the default which > is give an as an argument to the add_argument() call. Should I remove the > argument and use SRC_DIR directly instead? As it's now defaulting to the SRC_DIR, it's fine to keep it. You could add "Default: %default." to the help text though, to make it obvious. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:54: download_script = os.path.join(source_dir, On 2017/01/19 16:46:42, mandermo wrote: > On 2017/01/19 13:55:35, kjellander_webrtc wrote: > > Please introduce a constant at the top of the file: > > TOOLCHAIN_DIR = os.path.join(SRC_DIR, 'tools-webrtc', > 'video_quality_toolchain') > > and use it below to make the code more readable. > > Have introduced a local variable toolchain_dir. If remove the --source_dir > argument, then I can make it global. Should I? This is fine. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:65: subprocess.check_call(['adb', 'pull', '/sdcard/output.y4m', test_video]) On 2017/01/19 16:46:42, mandermo wrote: > On 2017/01/19 13:55:35, kjellander_webrtc wrote: > > Please pass the filename (output.y4m) as a flag, we want to minimize hardcoded > > duplicated names in this script. > > As a flag to the script? This path is already hard coded in > https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests.... > Should we communicate that path to ConnectActivityStubbedInputOutputTest? I > thought we did not want that. Argh, you're right. I forgot about that. Hopefully it won't change in the near future. https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:79: 'resources/reference_video_640x360_30fps.y4m') On 2017/01/19 16:46:42, mandermo wrote: > On 2017/01/19 13:55:35, kjellander_webrtc wrote: > > Please pass this string as a flag instead of hardcoding a name here which will > > break if the file is renamed/moved. > > As flag to this script? > > Can be good with flexibility. But thought we decided against flexibility with > regards to the reference video, since we wanted to hard code > reference_video_640x360_30fps.y4m into several places already, like > https://cs.chromium.org/chromium/src/third_party/webrtc/examples/BUILD.gn?q=r... > and > https://cs.chromium.org/chromium/src/third_party/webrtc/examples/androidtests.... You're right, my bad. https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: AppRTCMobile.apk and AppRTCMobileTestStubbedVideoIO.apk on the device. These don't need to be installed, it will be taken care of by the run_AppRTCMobileTestStubbedVideoIO script. https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:64: subprocess.check_call(['gn', 'gen', build_dir_x86]) It'll be hard to debug what's going on if a command fails since subprocess.check_call doesn't print what command it invoked. Can you wrap the check_call invocations in a helper function and utilize the logging module like: def _RunCommand(argv, **kwargs): logging.info('Running %r', argv) subprocess.check_call(argv, **kwargs) Then initialize logging like: logging.basicConfig(level=logging.INFO)
https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/20001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:23: parser.add_argument('--source_dir', On 2017/01/20 08:18:56, kjellander_webrtc wrote: > On 2017/01/19 16:46:42, mandermo wrote: > > On 2017/01/19 13:55:34, kjellander_webrtc wrote: > > > This flag doesn't seem to be needed if we just use SRC_DIR above (it's the > > same > > > dir). That however requires a full checkout on the machine that's executing > > the > > > test, which is true today but might not be in the future (but let's work on > > > isolating this in a follow-up CL when/if needed in the future instead). > > > > > > So just remove this flag and use SRC_DIR instead. > > > > This is an optional argument. It was useful when I was developing to be able > to > > put the file somewhere else and to be able to inject the the path to the > source > > dir, which is allowed now. If it is not given then it will use the default > which > > is give an as an argument to the add_argument() call. Should I remove the > > argument and use SRC_DIR directly instead? > > As it's now defaulting to the SRC_DIR, it's fine to keep it. You could add > "Default: %default." to the help text though, to make it obvious. Done. Added "." to the end of the help texts also. https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:19: AppRTCMobile.apk and AppRTCMobileTestStubbedVideoIO.apk on the device. On 2017/01/20 08:18:56, kjellander_webrtc wrote: > These don't need to be installed, it will be taken care of by the > run_AppRTCMobileTestStubbedVideoIO script. Ok, have removed that from the help text. https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:64: subprocess.check_call(['gn', 'gen', build_dir_x86]) On 2017/01/20 08:18:56, kjellander_webrtc wrote: > It'll be hard to debug what's going on if a command fails since > subprocess.check_call doesn't print what command it invoked. > > Can you wrap the check_call invocations in a helper function and utilize the > logging module like: > def _RunCommand(argv, **kwargs): > logging.info('Running %r', argv) > subprocess.check_call(argv, **kwargs) > > Then initialize logging like: > logging.basicConfig(level=logging.INFO) Done. Where is best to call logging.basicConfig(level=logging.INFO)? Now I put it outside a function in before the function declaration have started. Is it better in main()?
lgtm with comments addressed. https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:64: subprocess.check_call(['gn', 'gen', build_dir_x86]) On 2017/01/23 09:09:30, mandermo wrote: > On 2017/01/20 08:18:56, kjellander_webrtc wrote: > > It'll be hard to debug what's going on if a command fails since > > subprocess.check_call doesn't print what command it invoked. > > > > Can you wrap the check_call invocations in a helper function and utilize the > > logging module like: > > def _RunCommand(argv, **kwargs): > > logging.info('Running %r', argv) > > subprocess.check_call(argv, **kwargs) > > > > Then initialize logging like: > > logging.basicConfig(level=logging.INFO) > > Done. Where is best to call logging.basicConfig(level=logging.INFO)? Now I put > it outside a function in before the function declaration have started. Is it > better in main()? You can do it anywhere as long as it executes before logging.info actually executes. I suggest in the beginning of main() rather than the global scope. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:30: Remove blank line 30 and add another blank line above 29 (top-level statements are separated by 2 blank lines according to the style guide, and yes that's a bit silly) https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:79: _RunCommand([download_script]) Add sys.executable as the first argument in the list. Systems like Windows don't usually have a .py as a registered executable file type so they'll need to know what program to use to execute the script. sys.executable will be the Python interpreter executable that is currently executing this very script. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:82: espresso_target = os.path.join(build_dir_android, Rename to test_script. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:84: _RunCommand([espresso_target]) Add sys.executable https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:123: compare_script, '--ref_video', reference_video_yuv, Add sys.executable
https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/40001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:64: subprocess.check_call(['gn', 'gen', build_dir_x86]) On 2017/01/23 10:31:47, kjellander_webrtc wrote: > On 2017/01/23 09:09:30, mandermo wrote: > > On 2017/01/20 08:18:56, kjellander_webrtc wrote: > > > It'll be hard to debug what's going on if a command fails since > > > subprocess.check_call doesn't print what command it invoked. > > > > > > Can you wrap the check_call invocations in a helper function and utilize the > > > logging module like: > > > def _RunCommand(argv, **kwargs): > > > logging.info('Running %r', argv) > > > subprocess.check_call(argv, **kwargs) > > > > > > Then initialize logging like: > > > logging.basicConfig(level=logging.INFO) > > > > Done. Where is best to call logging.basicConfig(level=logging.INFO)? Now I put > > it outside a function in before the function declaration have started. Is it > > better in main()? > > You can do it anywhere as long as it executes before logging.info actually > executes. I suggest in the beginning of main() rather than the global scope. Moved to top of main(). https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... File webrtc/tools/test_stubbed_loopback_video.py (right): https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:30: On 2017/01/23 10:31:47, kjellander_webrtc wrote: > Remove blank line 30 and add another blank line above 29 (top-level statements > are separated by 2 blank lines according to the style guide, and yes that's a > bit silly) Done. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:79: _RunCommand([download_script]) On 2017/01/23 10:31:47, kjellander_webrtc wrote: > Add sys.executable as the first argument in the list. Systems like Windows don't > usually have a .py as a registered executable file type so they'll need to know > what program to use to execute the script. > > sys.executable will be the Python interpreter executable that is currently > executing this very script. Done. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:82: espresso_target = os.path.join(build_dir_android, On 2017/01/23 10:31:47, kjellander_webrtc wrote: > Rename to test_script. Done. https://codereview.webrtc.org/2632323003/diff/60001/webrtc/tools/test_stubbed... webrtc/tools/test_stubbed_loopback_video.py:84: _RunCommand([espresso_target]) On 2017/01/23 10:31:47, kjellander_webrtc wrote: > Add sys.executable Done.
Description was changed from ========== Script to start stubbed loopback video test with Espresso BUG= ========== to ========== Script to start stubbed loopback video test with Espresso BUG=webrtc:7034 ==========
The CQ bit was checked by mandermo@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/2632323003/#ps80001 (title: "Style fixes and sys.executable used to execute python scripts")
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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17904) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10088) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5509) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21911)
The CQ bit was checked by mandermo@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": 80001, "attempt_start_ts": 1485184764729980,
"parent_rev": "0ebdf2757c585f04eba18c9b4e7dd68910c42e35", "commit_rev":
"ed582f7e36773c50d359fc97525e17bbf3d1354c"}
Message was sent while issue was closed.
Description was changed from ========== Script to start stubbed loopback video test with Espresso BUG=webrtc:7034 ========== to ========== Script to start stubbed loopback video test with Espresso BUG=webrtc:7034 Review-Url: https://codereview.webrtc.org/2632323003 Cr-Commit-Position: refs/heads/master@{#16216} Committed: https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc975... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc975...
Message was sent while issue was closed.
On 2017/01/23 15:55:47, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as > https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc975... Would you mind renaming the script to video_quality_loopback_test.py and move it into tools-webrtc/android/ instead?
Message was sent while issue was closed.
On 2017/02/08 07:55:29, kjellander_webrtc wrote: > On 2017/01/23 15:55:47, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > > https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc975... > > Would you mind renaming the script to video_quality_loopback_test.py and move it > into tools-webrtc/android/ instead? I'll take care of this. I found a small bug as well.
Message was sent while issue was closed.
On 2017/02/08 07:55:29, kjellander_webrtc wrote: > On 2017/01/23 15:55:47, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > > https://chromium.googlesource.com/external/webrtc/+/ed582f7e36773c50d359fc975... > > Would you mind renaming the script to video_quality_loopback_test.py and move it > into tools-webrtc/android/ instead? I'll take care of this. I found a small bug as well. |
