Skip the http tests when no httpd could be found on the system instead of crashing at the very end of a frustatingly long testing session.
Created attachment 95773 [details] proposed patch
Is this a configuration we would ever desire to run the tests in? Perhaps run-webkit-tests should just refuse to run instead?
Created attachment 95892 [details] proposed patch v2
Comment on attachment 95892 [details] proposed patch v2 Great fix, r=me
Comment on attachment 95892 [details] proposed patch v2 Rejecting attachment 95892 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2 Last 500 characters of output: /VCSUtils_unittest/runPatchCommand.....................................ok Tools/Scripts/webkitperl/VCSUtils_unittest/setChangeLogDateAndReviewer.........................ok All tests successful. Files=19, Tests=363, 2 wallclock secs ( 1.03 cusr + 0.26 csys = 1.29 CPU) syntax error at Tools/Scripts/old-run-webkit-tests line 528, near "else if" syntax error at Tools/Scripts/old-run-webkit-tests line 540, near "}" Execution of Tools/Scripts/old-run-webkit-tests aborted due to compilation errors. Full output: http://queues.webkit.org/results/8757760
Created attachment 95907 [details] Archive of layout-test-results from cr-jail-7 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-7 Port: Mac Platform: Mac OS X 10.6.7
Comment on attachment 95892 [details] proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=95892&action=review > Tools/Scripts/old-run-webkit-tests:528 > +} else if (!hasHTTPD()) { Please s/else if/elsif before landing. ;)
(In reply to comment #7) > (From update of attachment 95892 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95892&action=review > > > Tools/Scripts/old-run-webkit-tests:528 > > +} else if (!hasHTTPD()) { > > Please s/else if/elsif before landing. ;) D'oh! My hugger-mugger trivial patches can make the whole tree red. Don't trust me blindly ;)
Why not use the commit-queue? And why not CC anyone who has worked on this perl before?
(In reply to comment #9) > Why not use the commit-queue? And why not CC anyone who has worked on this perl before? Sorry for the noise, my fault.
Looking good. Do we need the same change for new-run-webkit-tests?
Yeah, looks like new-run-webkit-tests will want something similar.
For completeness, this was committed in <http://trac.webkit.org/changeset/88012>.
Comment on attachment 95892 [details] proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=95892&action=review I know this patch was already reviewed and landed. I have some remarks. > Tools/ChangeLog:9 > + * Scripts/webkitperl/httpd.pm: It is convention that we either list or add a remark here about the function(s) that we added/modified for Perl changes since prepare-ChangeLog doesn't know how to generate the list of added and modified functions for Perl scripts. We should fix this. > Tools/Scripts/old-run-webkit-tests:529 > + print "\nNo httpd found. Cannot run http tests.\nPlease use --no-http if you do not want to run http tests.\n" You're missing a semicolon ';' at the end of this line. I assume you tested this script with your changes. I'm not sure why this wouldn't generate a syntax error. ah, you caught this and fixed it in <http://trac.webkit.org/changeset/88013>. > Tools/Scripts/webkitperl/httpd.pm:93 > + return system(@command) == 0; This should read: return exitStatus(system(@command)) == 0; By definition of system(), "The return value [of system()] is the exit status of the program as returned by the wait call. To get the actual exit value, shift right by eight". We have a convenience function, called exitStatus(), that does this in a portable way. See <http://perldoc.perl.org/functions/system.html> for more details.