Bug 27491 – Web Sockets Test Infrastructure Part 2/3: Patch to run-webkit-tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Yuzo Fujishima]

Add test infrastructure for Web Sockets (http://www.w3.org/TR/websockets/).

This bug is Part 2/3: Patch to run-webkit-tests.

Other parts are:
Part 1/3: Server
Part 3/3: Example test
Parts must be submitted in the order Part1, Part2, then Part3.

[Yuzo Fujishima]

Links:

Part 1/3: Server: https://bugs.webkit.org/show_bug.cgi?id=27490
Part 2/3: Patch to run-webkit-tests: https://bugs.webkit.org/show_bug.cgi?id=27491
Part 3/3: Example test: https://bugs.webkit.org/show_bug.cgi?id=27492

[Yuzo Fujishima]

Created attachment 33159 [details]
Patch to run-webkit-test to start up / shut down Web Socket Server.

[Alexey Proskuryakov]

Running WebSocket tests from file:// would mean that all requests will be cross-origin. It seems that we should want to test both same- and cross-origin requests.

[Yuzo Fujishima]

Hi, Alexey,

Thank you for your review.

I'm thinking of implementing
  window.websocketTestController.setOrigin(webSocket, originString)
just like window.layoutTestController offers various test functionalities.

Do you think it feasible? Your advice would be appreciated.

Yuzo

[Alexey Proskuryakov]

I think that it may be non-trivial to add such hooks in a way that would not be distinguishable from the real thing. Also, such tests wouldn't work in browser, making debugging more complicated.

Making WebSockets tests work together with http tests would be more reliable.

[Yuzo Fujishima]

Created attachment 34055 [details]
Patch to run-webkit-test to use Web Socket Server for both http and file schemes.

This should enable both same-origin (http://) and cross-origin(file://) testing.

[Eric Seidel (no email)]

Comment on attachment 34055 [details]
Patch to run-webkit-test to use Web Socket Server for both http and file schemes.

Please abstract:
8         my $testPath = "$testDirectory/$test";
 639         if (isCygwin()) {
 640             $testPath = toWindowsPath($testPath);
 641         } else {
 642             $testPath = canonpath($testPath);
 643         }

This is bad practice:
 1474     mkdir "/tmp/WebKit";

It make it harder to run more than one copy in parallel.  Better to use a ~ relative path or at least a unique name.  I'm certain there are other examples of such in run-webkit-tests.

That's a huge amount of code to make sure the server is dead.  Don't we already have some of that for the httpd server?  Can't we share that?

r- for the needed abstraction.

[Yuzo Fujishima]

Created attachment 41492 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Yuzo Fujishima]

Can anyone review this patch?

This changes the run-webkit-test script such that Web Socket (Secure) servers
added via Bug 27490 are started and stopped for tests under LayoutTests/websocket.

Yuzo

[Eric Seidel (no email)]

Comment on attachment 41492 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

I do not understand the minus lines in run-webkit-tests.

You need a more detailed ChangeLog giving more information about what you're doing here.

[Yuzo Fujishima]

Created attachment 41540 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Yuzo Fujishima]

Hi, Eric,

Thank you for the review.

I've added explanation to WebKitTools/ChangeLog.

FYI:
The minus-ed lines has been moved to the end of the if-elsif statement.

Yuzo

[Yuzo Fujishima]

Created attachment 41547 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Yuzo Fujishima]

Created attachment 41548 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Fumitoshi Ukai]

I've one question.
If I want to write a test with script-tests that uses LayoutTests/fast/js/, what should I do?
For now, websocket server sets document root to LayoutTests/websocket/tests, so the test can't access js files in LayoutTests/fast/js/.
Should I copy LayoutTests/fast/js/ into LayoutTests/websocket/tests/js ?

(same question for LayoutTests/http/tests)

Thanks in advance.

[Yuzo Fujishima]

Created attachment 41845 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Yuzo Fujishima]

Hi,

I've changed the document root to LayoutTests.
This should address https://bugs.webkit.org/show_bug.cgi?id=27491#c15

Reviewers and committers,
please commit this after https://bugs.webkit.org/show_bug.cgi?id=30763
This script change now requires pywebsocket 0.4.1

Yuzo

[Yuzo Fujishima]

Hi, reviewers,

Now that pywebsocket has been upgraded to 0.4.1, please resume reviewing this patch. :)

Yuzo

[Yuzo Fujishima]

Ping?

[David Levin]

Comment on attachment 41845 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

Just one issue.

> diff --git a/WebKitTools/Scripts/run-webkit-tests b/WebKitTools/Scripts/run-webkit-tests
> @@ -604,6 +603,32 @@ for my $test (@tests) {
>              }
>              print OUT "$testPath$suffixExpectedHash\n";
>          }
> +    } elsif ($test =~ /^websocket\//) {
> +        openWebSocketServerIfNeeded();

It seems like this should be in the "else" below because some of these test may run without needing the web socket server started.

> +        if ($test =~ /^websocket\/tests\/local\//) {
> +            my $testPath = "$testDirectory/$test";
> +            if (isCygwin()) {
> +                $testPath = toWindowsPath($testPath);
> +            } else {
> +                $testPath = canonpath($testPath);
> +            }
> +            print OUT "$testPath\n";
> +        } else {
> +            my $path = canonpath($test);
> +            if ($test =~ /^websocket\/tests\/ssl\//) {
> +                print OUT "https://127.0.0.1:$webSocketSecurePort/$path\n";
> +            } else {
> +                print OUT "http://127.0.0.1:$webSocketPort/$path\n";
> +            }
> +        }

[Yuzo Fujishima]

Created attachment 42440 [details]
Start/Stop Web Socket and Web Socket Secure servers for layout tests.

[Yuzo Fujishima]

Hi, David,

Thank you for the review. I've changed the code as suggested.
Can you take another look?

Yuzo

[Fumitoshi Ukai]

Committed r50500: <http://trac.webkit.org/changeset/50500>