[webkit-reviews] review granted: [Bug 183744] Add some tests for lldb_webkit.py : [Attachment 341210] Patch and unit tests that run fast

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 183744: Add some tests for lldb_webkit.py
https://bugs.webkit.org/show_bug.cgi?id=183744

Attachment 341210: Patch and unit tests that run fast

https://bugs.webkit.org/attachment.cgi?id=341210&action=review




--- Comment #24 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 341210
  --> https://bugs.webkit.org/attachment.cgi?id=341210
Patch and unit tests that run fast

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

> Tools/Scripts/webkitpy/common/system/systemhost.py:55
> +	   return
self.filesystem.abspath(self.filesystem.join(xcode_developer_directory, '..',
'SharedFrameworks', 'LLDB.framework', 'Versions', 'A', 'Resources', 'Python'))

Is it documented anywhere that LLDB.framework can be used from this location?
https://lldb.llvm.org says that the framework is exposed as a public on on
macOS, but I don't see it in /S/L/F. And
https://lldb.llvm.org/python-reference.html says that it's in /S/L/PF, which
doesn't seem correct either.

> Tools/Scripts/webkitpy/test/main.py:60
> +	   _log.info("Skipping lldb_webkit tests; could not find
'{}'.".format(lldb_webkit_tester_executable))

This seems fairly dangerous, given that only "make" builds build the tool. So
one may run the tests and not realize that they are not testing lldb.

Do you intend to add it to all other build scenarios?

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:1
> +// Copyright (C) 2015-2018 Apple Inc. All rights reserved.

It may be useful to compare config files to some project that's more actively
maintained (perhaps WebCore). I don't see anything that is an obvious problem,
but there aren't a lot of xcconfig problems that I can readily notice.

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:68
> +HEADER_SEARCH_PATHS = ${BUILT_PRODUCTS_DIR}/usr/local/include;

What are we including from here?

> Tools/lldb/lldbWebKitTester/main.cpp:28
> +#include <iostream>
> +#include <wtf/text/StringBuilder.h>
> +#include <wtf/text/WTFString.h>

This is an unusual mix of headers. Do we have to use iostream here?

> Tools/lldb/lldbWebKitTester/main.cpp:33
> +// From Tools/TestWebKitAPI/WTFStringUtilities.h

I'm not sure what useful information this line adds. Is it a FIXME: Find a way
to share implementation with TestWebKitAPI?

> Tools/lldb/lldb_webkit_unittest.py:2
> +# -*- coding: utf-8 -*-

I see that we have this line in a few source files, but that's rare, and last
it was discussed on webkit-dev, I think that the consensus was that we don't
add editor specific markup of any kind.

> Tools/lldb/lldb_webkit_unittest.py:85
> +	   cls.sbProcess.Kill()

Is it not a child process that will be killed automatically?

> Tools/lldb/lldb_webkit_unittest.py:86
> +	   cls.sbTarget.DeleteAllBreakpoints()

Given that this is only called on exit, not sure what benefit this provides.