Bug 54028 – http/tests/media should be served over HTTP (not from local file)

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

All tests in http/tests/media have been special cased in layout tests to be run from a local file.  They really should be tested over HTTP.  

As an example, http/tests/media/video-referer.html tests that the Referer header is set for media resources, but requests should not contain this type of information for any file:// URL for security reasons.  This test cannot be accurately tested unless served over http://.

[Eric Carlson]

(In reply to comment #0)
> All tests in http/tests/media have been special cased in layout tests to be run from a local file.  They really should be tested over HTTP.  
> 
> As an example, http/tests/media/video-referer.html tests that the Referer header is set for media resources, but requests should not contain this type of information for any file:// URL for security reasons.  This test cannot be accurately tested unless served over http://.

+1!

[Anna Cavender]

Created attachment 81684 [details]
Patch

[Anna Cavender]

Because the local server will run in http/tests/ for these tests, I had to copy 2 .js files into http/tests/media.  Feedback requested on a better solution than this.  Thanks!

[Eric Carlson]

Comment on attachment 81684 [details]
Patch

video-tests.js is out of date, you need to update your sources!

[Anna Cavender]

Created attachment 81706 [details]
Patch

[Anna Cavender]

OOPS, let's try that again.  This time with updated files.

[Eric Carlson]

Comment on attachment 81706 [details]
Patch

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

I would definitely babysit the bots when this lands, it may break tests that don't run on the port(s) you tested.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:360
>          # TODO(dpranke): remove the media reference and the SSL reference?
> -        if (port and not relative_path.startswith("local/") and
> -            not relative_path.startswith("media/")):
> +        if (port and not relative_path.startswith("local/")):

Can this TODO be changed now?

[Anna Cavender]

Comment on attachment 81706 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:360
>> +        if (port and not relative_path.startswith("local/")):
> 
> Can this TODO be changed now?

I think so (keeping the SSL reference).  I'll cc dpranke as well.

[Alexey Proskuryakov]

Hmm, I was always assuming that there was a very good reason to run these tests from local files. I guess not.

Did you check which revision this behavior was introduced in, and how it was explained?

[Eric Carlson]

(In reply to comment #9)
> Hmm, I was always assuming that there was a very good reason to run these tests from local files. I guess not.
> 
> Did you check which revision this behavior was introduced in, and how it was explained?

It was this way from when Antti added support for http media tests in r27757:

Index: Tools/Scripts/run-webkit-tests
===================================================================
--- Tools/Scripts/run-webkit-tests	(revision 27584)
+++ Tools/Scripts/run-webkit-tests	(revision 27757)
@@ -314,6 +314,7 @@
 
 if (!$testMedia) {
     $ignoredDirectories{'media'} = 1;
+    $ignoredDirectories{'http/tests/media'} = 1;
 }
 
 if ($ignoreTests) {
@@ -484,7 +485,7 @@
         print OUT "$testPath\n";
     } else {
         openHTTPDIfNeeded();
-        if ($test !~ /^http\/tests\/local\// && $test !~ /^http\/tests\/ssl\//) {
+        if ($test !~ /^http\/tests\/local\// && $test !~ /^http\/tests\/ssl\// && $test !~ /^http\/tests\/media\//) {
             my $path = canonpath($test);
             $path =~ s/^http\/tests\///;
             print OUT "http://127.0.0.1:$httpdPort/$path\n";

He doesn't remember why it was done that way.

[Antti Koivisto]

(In reply to comment #10)
> He doesn't remember why it was done that way.

I guess I just didn't consider loading the main document over http important and using local files was simpler.

[Anna Cavender]

Duplicate copies of video-test.js and media-file.js seems like a bad idea.  Could we perhaps move LayoutTests/http/tests/media into LayoutTests/media?  Is there a reason why these 15 tests are segregated from the rest of the media tests?

[Mihai Parparita]

(In reply to comment #12)
> Duplicate copies of video-test.js and media-file.js seems like a bad idea.  Could we perhaps move LayoutTests/http/tests/media into LayoutTests/media?  Is there a reason why these 15 tests are segregated from the rest of the media tests?

Presumably because they depend on some behavior that can only be tested by having an HTTP server running? Therefore I don't think moving them into LayoutTests/media is an option. Going in the other direction (moving LayoutTests/media into LayoutTests/http/tests/media) may be more feasible.

[Alexey Proskuryakov]

Generally, we only put things into http if they must be there.

[Anna Cavender]

Alright, how about we move video-test.js and media-file.js into http/tests/media and force the media/ tests to reach over and down to get them?

[Anna Cavender]

Created attachment 82355 [details]
Patch

[Anna Cavender]

This looks like a much bigger patch than it is.  Really just moving media-file.js and video-test.js into http/tests/media so that tests running from local server will be able to access them and then updating all the media tests to point to the new file locations.

[WebKit Commit Bot]

Comment on attachment 82355 [details]
Patch

Rejecting attachment 82355 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:

java/lc3/JSUndefined ..........
java/lc3/JavaArray ....
java/lc3/JavaClass ....
java/lc3/JavaObject .....................................
java/lc3/StringMethods .
java/lc3/forin .
java/lc3/instanceof .
loader ..
mathml .......
mathml/presentation ..................
media .
media/adopt-node-crash.html -> failed

Exiting early after 1 failures. 12974 tests run.
364.66s total testing time

12973 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
9 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7919518

[Mihai Parparita]

I patched this in locally, and all media tests failed. It looks like the move of media-file.js from the old to the new location isn't reflected in the patch (there's neither a delete nor an add). I'll fix up the patch and land it.

[Alexey Proskuryakov]

Sorry if I'm just stating the obvious, but please be sure to move with svn mv, so that commit history is preserved.

[Anna Cavender]

Shoot, missed that.  I moved with git mv on my git repo, not svn.  Won't forget next time! Thank everyone for your help.

[Mihai Parparita]

(In reply to comment #21)
> Shoot, missed that.  I moved with git mv on my git repo, not svn.  Won't forget next time! Thank everyone for your help.

git mv should work too. How did you upload your patch (this could be a bug with webkit-patch upload)?

[Mihai Parparita]

Committed r78778: <http://trac.webkit.org/changeset/78778>

[Anna Cavender]

Here's what I did, let me know if there is a better way!

# find the hash of the change right before my last commit (<hash>)
git log

# prep changelog from the diff between that commit and the head of this branch
Tools/Scripts/prepare-ChangeLog --git-commit <hash>..HEAD

# reset the git log, but keep all modifications
git reset --soft <hash>

# find ChangeLogs that were modified but not added to the patch (and modify)
git status

# add all modified ChangeLogs to the patch with git add
git add Path/To/Whatever/ChangeLog
(repeat for all changelogs)

# upload patch!
Tools/Scripts/webkit-patch upload

Thanks again!

[Eric Seidel (no email)]

Nah, you just ran into bug 49154.

[Eric Seidel (no email)]

This is failing on the Snow Leopard bot.

[Eric Seidel (no email)]

Actually, this has been failing since it was originally added!

SUCCESS: Build 8605 (r78741) was the first to show failures: set([u'media/controls-without-preload.html'])
Suspect revisions:
r78741:
  http://trac.webkit.org/changeset/78741
  Bug: None (None)
  Author: "Martin Robinson" <[e-mail]>
  Reviewer: None
  Committer: "Martin Robinson" <[e-mail]>
Explained all results for SnowLeopard Intel Release (WebKit2 Tests)

[Martin Robinson]

CCing Philippe, who converted the test.

[Philippe Normand]

I haven't worked on mac WebKit recently, so please bear with me :)

We did various rebaselines of this test, but depending on the bot where we took the results it started failing elsewhere. So I think it now needs different baselines for WebKit2 on mac, SL and Leopard, possibly.