Bug 232736 – [macOS] run-benchmark should take diagnostic screenshots upon test timeout

• Component: Tools / Tests
• Status: RESOLVED FIXED

[hysu]

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.

[hysu]

Created attachment 443358 [details]
Patch

[Stephanie Lewis]

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.

[hysu]

Created attachment 443736 [details]
Patch

[Stephanie Lewis]

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?

[hysu]

(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.

[hysu]

Created attachment 443756 [details]
Patch

Comment on attachment 443756 [details]
Patch

r=me

[Radar WebKit Bug Importer]

<rdar://problem/85319097>

[EWS]

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].