Unlike old-run-webkit-tests, running "run-webkit-tests --qt" with WEBKIT_TESTFONTS unset causes a crash in the dump render tree. We should check for env variable and raise an error if needed. Patch is coming.
Created attachment 124800 [details] Patch
Comment on attachment 124800 [details] Patch Cool, it should be landed as soon as possible. ;)
Comment on attachment 124800 [details] Patch Clearing flags on attachment: 124800 Committed r106460: <http://trac.webkit.org/changeset/106460>
All reviewed patches have been landed. Closing bug.
Comment on attachment 124800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124800&action=review > Tools/Scripts/webkitpy/layout_tests/port/qt.py:140 > + if not 'WEBKIT_TESTFONTS' in os.environ: > + print "\n\nThe WEBKIT_TESTFONTS environment variable is not defined or not set properly" > + print "You must set it before running the tests." > + print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n" > + sys.exit(1) Calling sys.exit is not appropriate. This is causing test-webkitpy to abort in the middle of testing! See http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Tests%29/builds/3138/steps/webkitpy-test/logs/stdio
Reopen, because it was rolled out: http://trac.webkit.org/changeset/106466
(In reply to comment #5) > Calling sys.exit is not appropriate. This is causing test-webkitpy to abort in the middle of testing! See http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Tests%29/builds/3138/steps/webkitpy-test/logs/stdio Would it be fine to raise a RuntimeError instead ? When I run test-webkitpy locally (a linux machine) it doesn't abort, either with sys.exit or the exception. If we just return then we'll still have the crash when the var is not set. The other solution would be to check if we are running unittests and then avoid checking if the variable is set or not, but I couldn't find a way to do this inside qt.py . Thanks for helping!
Throwing an exception and catching it at an appropriate level seems like the way to go. I've CCed some more folks who might have better ideas.
Created attachment 124987 [details] Patch
Comment on attachment 124987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124987&action=review > Tools/Scripts/webkitpy/layout_tests/port/qt.py:140 > + if not 'WEBKIT_TESTFONTS' in os.environ: > + print "\n\nThe WEBKIT_TESTFONTS environment variable is not defined or not set properly" > + print "You must set it before running the tests." > + print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n" > + raise RuntimeError('WEBKIT_TESTFONTS not set') You should add tests that show that this exception is/isn't thrown in the appropriate cases. > Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:86 > + try: > + port = self.make_port() > + env = port.setup_environ_for_server(port.driver_name()) > + self.assertEquals(env['QTWEBKIT_PLUGIN_PATH'], 'MOCK output of child process/lib/plugins') > + except RuntimeError, e: > + print e I don't think we want to add extra output to test-webkitpy. The test should set things up so that the exception doesn't get thrown.
Comment on attachment 124987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124987&action=review >> Tools/Scripts/webkitpy/layout_tests/port/qt.py:140 >> + raise RuntimeError('WEBKIT_TESTFONTS not set') > > You should add tests that show that this exception is/isn't thrown in the appropriate cases. More importantly, this is the wrong place for this code. You should override check_sys_deps() and implement this check there. Use _log.error() to report the error, and return False if the variable isn't set up properly. Look at chromium.py for an example of this. >> Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:86 >> + print e > > I don't think we want to add extra output to test-webkitpy. The test should set things up so that the exception doesn't get thrown. See above.
Created attachment 125168 [details] Patch
Comment on attachment 125168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125168&action=review Still need a unit test for this. And Dirk should probably give it the final r+. > Tools/Scripts/webkitpy/layout_tests/port/qt.py:158 > + _log.error('\nThe WEBKIT_TESTFONTS environment variable is not defined or not set properly.') > + _log.error('You must set it before running the tests.') > + _log.error('Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts') Do we normally use multiple _log.error calls to log multiple lines? Dirk would know.
(In reply to comment #13) > Still need a unit test for this. And Dirk should probably give it the final r+. > Ok, thanks. I will have a look at the unit tests we already have and prepare one for this check. > > Tools/Scripts/webkitpy/layout_tests/port/qt.py:158 > > + _log.error('\nThe WEBKIT_TESTFONTS environment variable is not defined or not set properly.') > > + _log.error('You must set it before running the tests.') > > + _log.error('Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts') > > Do we normally use multiple _log.error calls to log multiple lines? Dirk would know. I will wait for Dirk's comments then and address it all later, but I believe I saw multiple _log.error calls in chromium.py as well. Thanks for the reviews.
(In reply to comment #13) > > Tools/Scripts/webkitpy/layout_tests/port/qt.py:158 > > + _log.error('\nThe WEBKIT_TESTFONTS environment variable is not defined or not set properly.') > > + _log.error('You must set it before running the tests.') > > + _log.error('Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts') > > Do we normally use multiple _log.error calls to log multiple lines? Dirk would know. Yes, we normally use multiple calls for multiple lines (one line per call ensures that timestamps and other information are always logged).
Created attachment 125706 [details] Patch
(In reply to comment #16) > Created an attachment (id=125706) [details] > Patch Hi Dirk, would you mind to have a look at this patch? Thanks and sorry for bothering you! :)
sorry for the delay ... you can always feel free to bug me right away for a review.
Comment on attachment 125706 [details] Patch Thanks, Dirk!
Comment on attachment 125706 [details] Patch Clearing flags on attachment: 125706 Committed r107273: <http://trac.webkit.org/changeset/107273>