Bug 187916 – Add test-lldb-webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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.

[Alexey Proskuryakov]

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.

[Daniel Bates]

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.

[Daniel Bates]

Filed bug #189205 and bug #189206 to teach the build bot and EWS how to run the LLDB WebKit tests.

[Daniel Bates]

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

[Radar WebKit Bug Importer]

<rdar://problem/46258959>

[Jonathan Bedard]

Created attachment 355684 [details]
Patch

[Daniel Bates]

(In reply to Jonathan Bedard from comment #6)
> Created attachment 355684 [details]
> Patch

How is this different than my patch?

[Jonathan Bedard]

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

[Jonathan Bedard]

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 '_'

[Jonathan Bedard]

Created attachment 385151 [details]
Patch

[Alexey Proskuryakov]

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.

[Jonathan Bedard]

Created attachment 385159 [details]
Patch

[Stephanie Lewis]

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?

[Stephanie Lewis]

Not really familiar with the tester object so might have missed something there but I had a few comments on code readability

[Jonathan Bedard]

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.

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 385216 [details]
Patch

[Jonathan Bedard]

Created attachment 385257 [details]
Patch

[Alexey Proskuryakov]

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?

[Jonathan Bedard]

(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

[Jonathan Bedard]

Created attachment 385272 [details]
Patch

[Alexey Proskuryakov]

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

[Jonathan Bedard]

Created attachment 385289 [details]
Patch

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 385311 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385311 [details]
Patch

Clearing flags on attachment: 385311

Committed r253371: <https://trac.webkit.org/changeset/253371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.