Bug 187916

Summary: Add test-lldb-webkit
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, ews-watchlist, glenn, jbedard, lforschler, simon.fraser, slewis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=187872
https://bugs.webkit.org/show_bug.cgi?id=191033
https://bugs.webkit.org/show_bug.cgi?id=205016
https://bugs.webkit.org/show_bug.cgi?id=205271
Bug Depends on:    
Bug Blocks: 189205, 189206    
Attachments:
Description Flags
Patch
none
Patch
dbates: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Daniel Bates 2018-07-23 11:38:37 PDT
We should extract the logic to build and run the lldbwebkit tests from test-webkitpy to a new script, say test-lldbwebkit, to avoid mixing the lldbwebkit tests with the webkitpy tests.
Comment 1 Alexey Proskuryakov 2018-07-27 10:19:32 PDT
One of the reasons is that we’d need to run these tests for WebCore changes, while webkitpy tests only need to run for webkitpy changes.

Running tests as part of test-webkitpy also creates difficulties in settings where we dont’t have a WebKit build in the first place.
Comment 2 Daniel Bates 2018-08-31 12:41:44 PDT
Created attachment 348663 [details]
Patch

I will teach the EWS and build.webkit.org build bots how to run test-lldb-webkit in a subsequent patch/bug.
Comment 3 Daniel Bates 2018-08-31 13:04:21 PDT
Filed bug #189205 and bug #189206 to teach the build bot and EWS how to run the LLDB WebKit tests.
Comment 4 Daniel Bates 2018-11-05 20:05:18 PST
(In reply to Daniel Bates from comment #2)
> Created attachment 348663 [details]
> Patch
> 
> I will teach the EWS and build.webkit.org build bots how to run
> test-lldb-webkit in a subsequent patch/bug.

I don’t have time at the moment to do this. Maybe someone else can?
Comment 5 Radar WebKit Bug Importer 2018-11-26 15:23:04 PST
<rdar://problem/46258959>
Comment 6 Jonathan Bedard 2018-11-26 15:26:00 PST
Created attachment 355684 [details]
Patch
Comment 7 Daniel Bates 2018-11-26 15:32:47 PST
(In reply to Jonathan Bedard from comment #6)
> Created attachment 355684 [details]
> Patch

How is this different than my patch?
Comment 8 Jonathan Bedard 2018-11-26 15:38:57 PST
(In reply to Daniel Bates from comment #7)
> (In reply to Jonathan Bedard from comment #6)
> > Created attachment 355684 [details]
> > Patch
> 
> How is this different than my patch?

I missed that, sorry. I saw comment 4 but not the patch.
Comment 9 Jonathan Bedard 2018-11-26 16:00:46 PST
Comment on attachment 348663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348663&action=review

> Tools/Scripts/webkitpy/lldb_webkit_tests/main.py:63
> +    if os.path.isdir(lldb_python_directory):

Shouldn't we use the host object here to conform to webkitpy's idioms?

> Tools/Scripts/webkitpy/lldb_webkit_tests/main.py:95
> +

Can we add a --root option here? (matches with run-webkit-tests and run-api-tests)

> Tools/Scripts/webkitpy/lldb_webkit_tests/main.py:135
> +        self.printer.write_update("Checking autoinstalled packages ...")

We shouldn't need to do this for lldb tests

> Tools/Scripts/webkitpy/test/loader.py:32
> +class _Loader(unittest.TestLoader):

I feel like this should be class Loader(...) if we're allowing this to be imported, it shouldn't start with '_'
Comment 10 Jonathan Bedard 2019-12-09 08:19:14 PST
Created attachment 385151 [details]
Patch
Comment 11 Alexey Proskuryakov 2019-12-09 08:26:05 PST
Comment on attachment 385151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385151&action=review

I have a few wording nitpicks, someone else should probably review in earnest.

> Tools/ChangeLog:8
> +        * Scripts/test-lldb: Added.

This is not a good name for the script. It does not test lldb, it tests WebKit integration with it.

> Tools/Scripts/test-lldb:55
> +        parser.add_argument('--build', '--no-build',

It’s not clear to me from the code what the default is. Is it clear from the printed help message? Also, should definitely say what would be built.
Comment 12 Jonathan Bedard 2019-12-09 09:44:25 PST
Created attachment 385159 [details]
Patch
Comment 13 Stephanie Lewis 2019-12-09 14:07:54 PST
Comment on attachment 385159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385159&action=review

> Tools/Scripts/test-lldb-webkit:49
> +

Nice. I miss perl semantics.

> Tools/Scripts/test-lldb-webkit:119
> +        tester.parse_args()

Why are args only parsed in the fail case?

> Tools/Scripts/test-lldb-webkit:120
> +        print('lldb tests are only supported on macOS')

This error doesn't make sense to me. There are other reasons a python directory might be missing. And if you want to say macOS only shouldn't you do a macOS check?

> Tools/Scripts/webkitpy/test/main.py:235
> +                    style='release',

Forget to remove?
Comment 14 Stephanie Lewis 2019-12-09 14:09:10 PST
Not really familiar with the tester object so might have missed something there but I had a few comments on code readability
Comment 15 Jonathan Bedard 2019-12-09 14:27:48 PST
Comment on attachment 385159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385159&action=review

>> Tools/Scripts/test-lldb-webkit:119
>> +        tester.parse_args()
> 
> Why are args only parsed in the fail case?

This allows the help message to still work if you aren't on Mac. We parse arguments inside the tester otherwise.

>> Tools/Scripts/test-lldb-webkit:120
>> +        print('lldb tests are only supported on macOS')
> 
> This error doesn't make sense to me. There are other reasons a python directory might be missing. And if you want to say macOS only shouldn't you do a macOS check?

If you checkout Tools/Scripts/webkitpy/common/system/systemhost.py, you can see what's actually going on here.

There are basically two reasons for this, one is that we aren't on Mac, the other is that we don't have Xcode installed.

Maybe change this to:

'lldb tests are only supported on macOS with Xcode installed'

>> Tools/Scripts/webkitpy/test/main.py:235
>> +                    style='release',
> 
> Forget to remove?

No, we always need a style when reporting results, even though style doesn't make much sense for webkitpy tests. I figured that 'release' makes the most sense.
Comment 16 Jonathan Bedard 2019-12-09 14:36:58 PST
(In reply to Stephanie Lewis from comment #14)
> Not really familiar with the tester object so might have missed something
> there but I had a few comments on code readability

It's an object designed for running Python unit tests...it's the same class that we're moving the code out of in Tools/Scripts/webkitpy/test/main.py.
Comment 17 Jonathan Bedard 2019-12-09 17:22:58 PST
Created attachment 385216 [details]
Patch
Comment 18 Jonathan Bedard 2019-12-10 07:14:28 PST
Created attachment 385257 [details]
Patch
Comment 19 Alexey Proskuryakov 2019-12-10 09:03:32 PST
Comment on attachment 385257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385257&action=review

> Tools/Scripts/test-lldb-webkit:57
> +                            default=None, help='Enable or disable building lldb test runner before running the test (enabled by default)')

Just to double check, does this only build the test runner, not any WebKit project like test-webkitpy used to do?

> Tools/Scripts/test-lldb-webkit:77
> +        parser.add_argument('--configuration',
> +                            dest='configuration', action='store',
> +                            default=None, help='Store configuration to test.')

What does "store" mean in this context? I do not understand this, so I doubt that people looking at the help will.

> Tools/Scripts/test-lldb-webkit:79
> +                            default=None, help='Name of tests to be run.')

In the help string, s/Name/Names/.

Maybe some examples would help here, I always have trouble figuring out the naming scheme. Also, is this comma separated?
Comment 20 Jonathan Bedard 2019-12-10 09:37:01 PST
(In reply to Alexey Proskuryakov from comment #19)
> Comment on attachment 385257 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385257&action=review
> 
> > Tools/Scripts/test-lldb-webkit:57
> > +                            default=None, help='Enable or disable building lldb test runner before running the test (enabled by default)')
> 
> Just to double check, does this only build the test runner, not any WebKit
> project like test-webkitpy used to do?

It runs build-lldbwebkittester, specifically. Which only builds the lldb test runner, along with anything that test runner needs (which is bmalloc and WTF, if I recall.

> 
> > Tools/Scripts/test-lldb-webkit:77
> > +        parser.add_argument('--configuration',
> > +                            dest='configuration', action='store',
> > +                            default=None, help='Store configuration to test.')
> 
> What does "store" mean in this context? I do not understand this, so I doubt
> that people looking at the help will.

I'll try and improve the help message here, but this is a pretty standard option in webkitpy scripts which build.

> 
> > Tools/Scripts/test-lldb-webkit:79
> > +                            default=None, help='Name of tests to be run.')
> 
> In the help string, s/Name/Names/.
> 
> Maybe some examples would help here, I always have trouble figuring out the
> naming scheme. Also, is this comma separated?

Examples might get out of date. It's not comma separated, it's basically the equivalent of 'args' in the old opt parser scripts, it will be separated by spaces.

These are going to be something like: lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFCompactPointerTupleProvider_empty
Comment 21 Jonathan Bedard 2019-12-10 09:38:31 PST
Created attachment 385272 [details]
Patch
Comment 22 Alexey Proskuryakov 2019-12-10 11:07:49 PST
> It runs build-lldbwebkittester, specifically. Which only builds the lldb
> test runner, along with anything that test runner needs (which is bmalloc
> and WTF, if I recall.

Please explain it in help. This is something that we were unsure about, no need to leave other people confused.

> Examples might get out of date. It's not comma separated, it's basically the
> equivalent of 'args' in the old opt parser scripts, it will be separated by
> spaces.

Please add it to the help. Even if test names change, which is super unlikely, the overall format won't.

> lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFCompactPointerTupleProvider_empty

In particular, how is one to know that it begins with lldb_webkit_unittest? It could as well be implicit.
Comment 23 Jonathan Bedard 2019-12-10 11:30:42 PST
Created attachment 385289 [details]
Patch
Comment 24 Jonathan Bedard 2019-12-10 11:32:26 PST
(In reply to Alexey Proskuryakov from comment #22)
> > It runs build-lldbwebkittester, specifically. Which only builds the lldb
> > test runner, along with anything that test runner needs (which is bmalloc
> > and WTF, if I recall.
> 
> Please explain it in help. This is something that we were unsure about, no
> need to leave other people confused.

I can, but this is another case where it's pretty easy for the help message to get out of sync with what build-lldbwebkittester actually does.

> 
> > Examples might get out of date. It's not comma separated, it's basically the
> > equivalent of 'args' in the old opt parser scripts, it will be separated by
> > spaces.
> 
> Please add it to the help. Even if test names change, which is super
> unlikely, the overall format won't.
> 
> > lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFCompactPointerTupleProvider_empty
> 
> In particular, how is one to know that it begins with lldb_webkit_unittest?
> It could as well be implicit.

It's based on the test file name. It's actually not the only one. Easiest way to get this list would be to pass -v so the test names are printed.
Comment 25 Jonathan Bedard 2019-12-10 15:16:15 PST
Created attachment 385311 [details]
Patch
Comment 26 WebKit Commit Bot 2019-12-11 07:13:22 PST
Comment on attachment 385311 [details]
Patch

Clearing flags on attachment: 385311

Committed r253371: <https://trac.webkit.org/changeset/253371>
Comment 27 WebKit Commit Bot 2019-12-11 07:13:24 PST
All reviewed patches have been landed.  Closing bug.