Browser benchmarks should be able to take screenshots when tests timeout if a diagnostic directory (--diagnose-directory) is specified. This functionality already exists in iOS, and should be roughly mirrored to macOS.
Created attachment 443358 [details] Patch
Comment on attachment 443358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443358&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:42 > + def _save_screenshot_to_path(self, output_directory, filename_without_extention): typo: extention -> extension Why the choice to not add the extension to the filename originally? I've found that changing passed in filenames adds a lot of additional complexity if we ever want to do anything else with the file later on and need to recreate the filename. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:43 > + jpg_image_name = '{}.jpg'.format(filename_without_extention) ditto > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:55 > + _log.info('Diagnose directory is not specified, will skip diagnosing.') Where does diagnose directory get set? If we don't pass in the option some default should be chosen like /private/tmp/ but maybe that happens before this > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:60 > + if os.path.isdir(diagnose_directory): nit: we use this kind of code a lot. If there isn't already a clean up/recreate directory function maybe we should move this to utils and make one.
Comment on attachment 443358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443358&action=review r=me > Tools/ChangeLog:8 > + Added diagnose_test_failure method to OSXBrowserDriver which cleans the diagnostic directory and takes a screenshot if a directory is specified (--diagnose-directory). Nit: this log line is too long, let's split it.
Created attachment 443736 [details] Patch
Comment on attachment 443736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443736&action=review > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:37 > + return '/private/tmp/run-benchmark-diagnostics/' should probably add a timestamp. tmp and private/tmp are pretty much the same thing so I don't know if the difference here is meaningful
Comment on attachment 443736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443736&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:65 > + if os.path.exists(diagnose_directory): > + _log.info('Diagnose directory: "{}" already exists, cleaning it up'.format(diagnose_directory)) > + if os.path.isdir(diagnose_directory): > + if len(os.listdir(diagnose_directory)): > + shutil.rmtree(diagnose_directory) > + elif os.path.isfile(diagnose_directory): > + os.remove(diagnose_directory) > + if not os.path.exists(diagnose_directory): > + os.makedirs(diagnose_directory) Let's add some code to catch the exception here, for example, shutil.rmtree / os.remove may fail > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:55 > + parser.add_argument('--diagnose-directory', dest='diagnose_dir', default=default_diagnose_dir(), help='Directory for storing diagnose information on test failure. Defaults to %s.' % default_diagnose_dir()) Let's use str.format instead?
(In reply to dewei_zhu from comment #6) > Let's use str.format instead? I kept it as string interpolation for now to keep it consistent with the surrounding code. I agree that it'd probably be far neater as str.format, but I think that change might fit better as its own separate patch.
(In reply to W.D. Xiong from comment #7) > (In reply to dewei_zhu from comment #6) > > Let's use str.format instead? > > I kept it as string interpolation for now to keep it consistent with the > surrounding code. I agree that it'd probably be far neater as str.format, > but I think that change might fit better as its own separate patch. I think what we usually do is ensure the new code follows the preferred code style and have a separate patch to update the existing code to new style. Let's use 'str.format' here.
Created attachment 443756 [details] Patch
Comment on attachment 443756 [details] Patch r=me
<rdar://problem/85319097>
Committed r286374 (244733@main): <https://commits.webkit.org/244733@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443756 [details].