[macOS] TestWebKitAPI.WebKit.HTTPReferer is a flaky failure

[Ryan Haddad]

TestWebKitAPI.WebKit.HTTPReferer is a flaky failure on macOS bots

    TestWebKitAPI.WebKit.HTTPReferer
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:63
        Value of: strstr(request.data(), expectedHeaderField.utf8().data())
          Actual: false
        Expected: true
        

https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebKit.HTTPReferer

[Radar WebKit Bug Importer]

<rdar://problem/63470619>

[Ryan Haddad]

This test was added with https://trac.webkit.org/changeset/261402/webkit

[Ryan Haddad]

It looks like it has been flaky since the beginning.

[Alex Christensen]

Created attachment 399966 [details]
Patch

[Chris Dumez]

Comment on attachment 399966 [details]
Patch

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

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:53
> +    NSURLRequest *request(const String& path = "/") const;

"/"_str

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:184
> +        responseBuilder.append("HTTP/1.1 ");

These things could use appendLiteral()

[Darin Adler]

Comment on attachment 399966 [details]
Patch

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

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:70
> +    Connection(const Connection&) = default;
> +    Connection(Connection&&) = default;
> +    Connection& operator=(const Connection&) = default;
> +    Connection& operator=(Connection&&) = default;

I think you can just leave all of this out and it should still work the same. The compiler should generate all of this unless we tell it not to.

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:190
> +        responseBuilder.append("HTTP/1.1 ");
> +        responseBuilder.appendNumber(response.statusCode);
> +        responseBuilder.append(' ');
> +        responseBuilder.append(statusText(response.statusCode));
> +        responseBuilder.append("\r\nContent-Length: ");
> +        responseBuilder.appendNumber(response.body.length());
> +        responseBuilder.append("\r\n");

Possible drive-by improvement. Variadic append is possibly easier to read, and happens to be more efficient as well:

    responseBuilder.append("HTTP/1.1 ", response.statusCode, ' ', statusText(response.statusCode), "\r\n"
        "Content-Length: ", response.body.length(), "\r\n");

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:195
> +            responseBuilder.append(pair.key);
> +            responseBuilder.append(": ");
> +            responseBuilder.append(pair.value);
>              responseBuilder.append("\r\n");

responseBuilder.append(pair.key, ": ", pair.value, "\r\n");

[Alex Christensen]

I like the variadic append.  I'm going to separate the lines of the HTTP header like this:

StringBuilder responseBuilder;
responseBuilder.append("HTTP/1.1 ", response.statusCode, ' ', statusText(response.statusCode), "\r\n");
responseBuilder.append("Content-Length: ", response.body.length(), "\r\n");
for (auto& pair : response.headerFields)
    responseBuilder.append(pair.key, ": ", pair.value, "\r\n");
responseBuilder.append("\r\n");
responseBuilder.append(response.body);

[Alex Christensen]

http://trac.webkit.org/r262018