Bug 164344 – Add TCP client to DumpRenderTree and WebKitTestRunner for on device testing

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Jonathan Bedard]

In order to test on device, a TCP connection will be opened between device and host.  This adds the TCP client which will run on the device.  This TCP client redirects standard out and standard error to the TCP connection and sends it's received data to standard in.  If the server is not properly initialized, the normal handles for standard out, standard error and standard in will be used.

[Jonathan Bedard]

https://bugs.webkit.org/show_bug.cgi?id=164338 contains the server associated with this client.

Note that this patch does not use the TCP client, it simply implements it.

[Jonathan Bedard]

Created attachment 293713 [details]
Patch

[Jonathan Bedard]

Created attachment 293767 [details]
Patch

[Jonathan Bedard]

Created attachment 293772 [details]
Patch

[WebKit Commit Bot]

Attachment 293772 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/CMakeLists.txt:38:  No trailing spaces  [whitespace/trailing] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 293772 [details]
Patch

Attachment 293772 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2456149

New failing tests:
inspector/sampling-profiler/call-frame-with-dom-functions.html

[Build Bot]

Created attachment 293795 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jonathan Bedard]

Created attachment 293803 [details]
Patch

[Jonathan Bedard]

Created attachment 293815 [details]
Patch

[Jonathan Bedard]

Created attachment 293891 [details]
Patch

[Jonathan Bedard]

Created attachment 293898 [details]
Patch

[Jonathan Bedard]

Created attachment 293930 [details]
Patch

[WebKit Commit Bot]

Attachment 293930 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/PipingServer.cpp:35:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 294062 [details]
Patch

[WebKit Commit Bot]

Attachment 294062 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/PipingServer.cpp:34:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 294065 [details]
Patch

[WebKit Commit Bot]

Attachment 294065 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/PipingServer.cpp:34:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 294067 [details]
Patch

[WebKit Commit Bot]

Attachment 294067 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/PipingServer.cpp:34:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Comment on attachment 294067 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:-1087
> -

Typo here has been noted

> Tools/WebKitTestRunner/TestController.cpp:-1115
> -

Typo here has been noted

[Radar WebKit Bug Importer]

<rdar://problem/29206941>

[Daniel Bates]

Comment on attachment 294067 [details]
Patch

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

> Tools/TestRunnerShared/PipingServer.cpp:235
> +static LoggingTCPPipe* singlePtr(nullptr);
> +
> +LoggingTCPPipe& LoggingTCPPipe::singleton()
> +{
> +    if (!isInitialized())
> +        initialize();
> +    return *singlePtr;
> +}

This is weird. I suggest that DRT/WTR manage the singleton LoggingTCPPipe.

> Tools/TestRunnerShared/PipingServer.cpp:239
> +#if OS(UNIX)

Can we separate out the UNIX and Windows implementations into separate files similar to what we do in WebCore/platform?

> Tools/TestRunnerShared/PipingServer.cpp:257
> +    int portVal = 5000 + getpid() % 5000;

Can you explain this? How did you pick the number 5000?  Maybe a better name is basePort?

> Tools/TestRunnerShared/PipingServer.cpp:297
> +    memset(m_threads, 0, sizeof(std::thread*)*3);

Can we use std::array and remove the need for zeroing out this memory?

> Tools/TestRunnerShared/PipingServer.cpp:310
> +    delete m_stdout;
> +    delete m_stderr;
> +    delete m_stdin;
> +    delete m_stdoutSocket;
> +    delete m_stderrSocket;
> +    delete m_stdinSocket;

Can we use std::unique_ptr and remove the need for these deletes?

> Tools/TestRunnerShared/PipingServer.cpp:337
> +    m_threads[s_stdoutThread] = new std::thread(LoggingTCPPipe::stdoutLoop);
> +    m_threads[s_stderrThread] = new std::thread(LoggingTCPPipe::stderrLoop);

Can we wrap these raw pointers in std::unique_ptr?

> Tools/TestRunnerShared/PipingServer.cpp:380
> +#define TYPE_TAG 8

What is the purpose of this variable? Can we come up with a more descriptive name? How did you pick the value 8?

> Tools/TestRunnerShared/TestingSocket.cpp:133
> +    size_t len = strlen(name);
> +    m_name = new char[len + 1];
> +    memset(m_name, 0, len + 1);
> +    memcpy(m_name, name, len);

Can we use std::string and avoid this?

> Tools/TestRunnerShared/TestingSocket.cpp:138
> +    strcpy(m_address.sun_path, m_name);

strncpy?

> Tools/TestRunnerShared/TestingSocket.h:59
> +    inline FILE* file() { return m_target; }
> +    inline FILE* original() { return m_original; }
> +    inline bool isEnabled() const { return m_enabled; }

The keyword inline is not necessary. Inline C++ member functions are inlined by definition.

> Tools/TestRunnerShared/TestingSocket.h:68
> +class TCPTestingSocket : public TestingSocket {

Can we come up with a more descriptive name for this? I am unclear of the purpose of this class. From talking with you this class is only used for Windows. Maybe we should state that in its name?

> Tools/TestRunnerShared/TestingSocket.h:88
> +class UnixTestingSocket : public TestingSocket {

Can we come up with a more descriptive name for this? I am unclear of the purpose of this class.

> Tools/TestRunnerShared/TestingSocket.h:96
> +    char* m_name;

Can we use std::string?

> Tools/WebKitTestRunner/Options.cpp:47
> +    , tcpAddress("127.0.0.1")

This is OK as-is. Is there an API we can use to determine the local loopback address?

> Tools/WebKitTestRunner/Options.cpp:177
> +            

Is this intentional?

> Tools/WebKitTestRunner/Options.h:51
> +    int tcpPort;

Would it make sense to use an unsigned integer for the TCP port? How do we behave if a person passes a negative port number?

> Tools/WebKitTestRunner/TestController.cpp:1126
> +        fflush(stderr);

Normally stderr has the property that it auto flushes. Maybe this no longer makes sense when using TCP sockets.

> Tools/WebKitTestRunner/ios/mainIOS.mm:72
> +    NSString *docs_dir = [paths objectAtIndex:0];

Variables names should be written in CamelCase as opposed to using underscores to separate words in a name.

[Jonathan Bedard]

Created attachment 294731 [details]
Patch

[WebKit Commit Bot]

Attachment 294731 [details] did not pass style-queue:


ERROR: Tools/TestRunnerShared/LoggingTCPPipe.cpp:34:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestRunnerShared/InternalFileSocket.cpp:34:  socklen_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 294731 [details]
Patch

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

> Tools/ChangeLog:16
> +        (runTestingServerLoop): Added â#quitâ command to exit application gracefully, flush stdout and stderr every loop.

Quotes are not being encoded correctly at least in PrettyPatch. We should explain why we need to add a quit command given that the current setup worked without this.

> Tools/ChangeLog:21
> +        (WTR::InternalFileSocket::FileSide::FileSide): "Client side" of the local socket, which swaps the provided FILE* with a FILE* referenceing a local socket.

referenceing => referencing

> Tools/TestRunnerShared/InternalFileSocket.cpp:70
> +    memset((char*)&m_address, 0, sizeof(m_address));
> +    m_address.sun_family = AF_UNIX;

How did you decide to build up m_address here as opposed to in ::open()?

> Tools/TestRunnerShared/InternalFileSocket.cpp:71
> +    strncpy(m_address.sun_path, m_name.c_str(), 108);

108 is is the wrong size of the sun_path field on OS X per unix(4). Regardless, we should use sizeof(m_address.sun_path) to let the compiler compute the correct size of this field instead of hardcoding a size.

> Tools/TestRunnerShared/InternalFileSocket.cpp:103
> +    m_socket = socket(AF_UNIX, SOCK_STREAM, 0);

Is there a named constant that we can use for protocol 0? Or can we add a comment?

> Tools/TestRunnerShared/InternalFileSocket.cpp:152
> +    memset((char*)&m_address, 0, sizeof(m_address));
> +    m_address.sun_family = AF_UNIX;

How did you decide to build up m_address here as opposed to in ::open()?

> Tools/TestRunnerShared/InternalFileSocket.cpp:153
> +    strncpy(m_address.sun_path, m_name.c_str(), 108);

108 is is the wrong size of the sun_path field on OS X per unix(4). Regardless, we should use sizeof(m_address.sun_path) to let the compiler compute the correct size of this field instead of hardcoding a size.

[Jonathan Bedard]

Using a different approach.