Bug 54028

(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!

Created attachment 81684 [details] Patch

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!

Comment on attachment 81684 [details] Patch video-tests.js is out of date, you need to update your sources!

Created attachment 81706 [details] Patch

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

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?

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.

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?

(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.

(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.

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?

(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.

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

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?

Created attachment 82355 [details] Patch

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.

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

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.

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

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

(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)?

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

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!

Nah, you just ran into bug 49154.

This is failing on the Snow Leopard bot.

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" <mrobinson@webkit.org> Reviewer: None Committer: "Martin Robinson" <mrobinson@webkit.org> Explained all results for SnowLeopard Intel Release (WebKit2 Tests)

CCing Philippe, who converted the test.

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.