Bug 77466 – [Qt][WK2] run-webkit-tests --qt crashes if WEBKIT_TESTFONTS is not set

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

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.

[Jesus Sanchez-Palencia]

Created attachment 124800 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 124800 [details]
Patch

Cool, it should be landed as soon as possible. ;)

[WebKit Review Bot]

Comment on attachment 124800 [details]
Patch

Clearing flags on attachment: 124800

Committed r106460: <http://trac.webkit.org/changeset/106460>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

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

[Csaba Osztrogonác]

Reopen, because it was rolled out: http://trac.webkit.org/changeset/106466

[Jesus Sanchez-Palencia]

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

[Adam Roben (:aroben)]

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.

[Jesus Sanchez-Palencia]

Created attachment 124987 [details]
Patch

[Adam Roben (:aroben)]

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.

[Dirk Pranke]

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.

[Jesus Sanchez-Palencia]

Created attachment 125168 [details]
Patch

[Adam Roben (:aroben)]

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.

[Jesus Sanchez-Palencia]

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

[Dirk Pranke]

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

[Jesus Sanchez-Palencia]

Created attachment 125706 [details]
Patch

[Jesus Sanchez-Palencia]

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

[Dirk Pranke]

sorry for the delay ... you can always feel free to bug me right away for a review.

[Jesus Sanchez-Palencia]

Comment on attachment 125706 [details]
Patch

Thanks, Dirk!

[WebKit Review Bot]

Comment on attachment 125706 [details]
Patch

Clearing flags on attachment: 125706

Committed r107273: <http://trac.webkit.org/changeset/107273>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.