Bug 77466 - [Qt][WK2] run-webkit-tests --qt crashes if WEBKIT_TESTFONTS is not set
Summary: [Qt][WK2] run-webkit-tests --qt crashes if WEBKIT_TESTFONTS is not set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on: 77552
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 12:37 PST by Jesus Sanchez-Palencia
Modified: 2012-02-09 12:19 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.90 KB, patch)
2012-01-31 12:53 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2012-02-01 12:15 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (1.82 KB, patch)
2012-02-02 12:57 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2012-02-06 14:06 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2012-01-31 12:37:29 PST
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.
Comment 1 Jesus Sanchez-Palencia 2012-01-31 12:53:45 PST
Created attachment 124800 [details]
Patch
Comment 2 Csaba Osztrogonác 2012-02-01 01:13:30 PST
Comment on attachment 124800 [details]
Patch

Cool, it should be landed as soon as possible. ;)
Comment 3 WebKit Review Bot 2012-02-01 02:37:29 PST
Comment on attachment 124800 [details]
Patch

Clearing flags on attachment: 124800

Committed r106460: <http://trac.webkit.org/changeset/106460>
Comment 4 WebKit Review Bot 2012-02-01 02:37:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Adam Roben (:aroben) 2012-02-01 06:40:22 PST
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
Comment 6 Csaba Osztrogonác 2012-02-01 06:55:32 PST
Reopen, because it was rolled out: http://trac.webkit.org/changeset/106466
Comment 7 Jesus Sanchez-Palencia 2012-02-01 09:53:13 PST
(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!
Comment 8 Adam Roben (:aroben) 2012-02-01 10:03:11 PST
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.
Comment 9 Jesus Sanchez-Palencia 2012-02-01 12:15:07 PST
Created attachment 124987 [details]
Patch
Comment 10 Adam Roben (:aroben) 2012-02-01 12:22:21 PST
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 11 Dirk Pranke 2012-02-01 12:51:13 PST
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.
Comment 12 Jesus Sanchez-Palencia 2012-02-02 12:57:29 PST
Created attachment 125168 [details]
Patch
Comment 13 Adam Roben (:aroben) 2012-02-03 12:33:54 PST
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.
Comment 14 Jesus Sanchez-Palencia 2012-02-03 12:39:59 PST
(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.
Comment 15 Dirk Pranke 2012-02-03 12:50:27 PST
(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).
Comment 16 Jesus Sanchez-Palencia 2012-02-06 14:06:02 PST
Created attachment 125706 [details]
Patch
Comment 17 Jesus Sanchez-Palencia 2012-02-09 04:16:30 PST
(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! :)
Comment 18 Dirk Pranke 2012-02-09 10:33:42 PST
sorry for the delay ... you can always feel free to bug me right away for a review.
Comment 19 Jesus Sanchez-Palencia 2012-02-09 10:50:59 PST
Comment on attachment 125706 [details]
Patch

Thanks, Dirk!
Comment 20 WebKit Review Bot 2012-02-09 12:19:01 PST
Comment on attachment 125706 [details]
Patch

Clearing flags on attachment: 125706

Committed r107273: <http://trac.webkit.org/changeset/107273>
Comment 21 WebKit Review Bot 2012-02-09 12:19:07 PST
All reviewed patches have been landed.  Closing bug.