Bug 232736 - [macOS] run-benchmark should take diagnostic screenshots upon test timeout
Summary: [macOS] run-benchmark should take diagnostic screenshots upon test timeout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Mac (Apple Silicon) macOS 11
: P2 Normal
Assignee: W.D. Xiong
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-04 16:36 PDT by W.D. Xiong
Modified: 2021-12-01 11:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.11 KB, patch)
2021-11-04 17:14 PDT, W.D. Xiong
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2021-11-09 15:18 PST, W.D. Xiong
no flags Details | Formatted Diff | Diff
Patch (5.57 KB, patch)
2021-11-09 16:40 PST, W.D. Xiong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description W.D. Xiong 2021-11-04 16:36:28 PDT
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.
Comment 1 W.D. Xiong 2021-11-04 17:14:00 PDT
Created attachment 443358 [details]
Patch
Comment 2 Stephanie Lewis 2021-11-05 11:47:50 PDT
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 3 dewei_zhu 2021-11-05 13:49:10 PDT
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.
Comment 4 W.D. Xiong 2021-11-09 15:18:11 PST
Created attachment 443736 [details]
Patch
Comment 5 Stephanie Lewis 2021-11-09 15:34:32 PST
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 6 dewei_zhu 2021-11-09 15:36:44 PST
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?
Comment 7 W.D. Xiong 2021-11-09 15:54:51 PST
(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.
Comment 8 dewei_zhu 2021-11-09 16:00:10 PST
(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.
Comment 9 W.D. Xiong 2021-11-09 16:40:05 PST
Created attachment 443756 [details]
Patch
Comment 10 dewei_zhu 2021-11-10 12:20:51 PST
Comment on attachment 443756 [details]
Patch

r=me
Comment 11 Radar WebKit Bug Importer 2021-11-11 15:37:27 PST
<rdar://problem/85319097>
Comment 12 EWS 2021-12-01 11:23:39 PST
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].