Bug 42947 – run-webkit-tests should check first for $WEBKIT_TESTFONTS is set

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mahesh Kulkarni]

run-webkit-tests shows crashed for every test run on env where $WEBKIT_TESTFONTS is not set or not set to right font directory. 

Scripts should show error in this case and terminate

[Mahesh Kulkarni]

Created attachment 62526 [details]
patch

Before running layoutTest check for WEBKIT_TESTFONTS set and exists

[Darin Adler]

Comment on attachment 62526 [details]
patch

Change seems fine.

ChangeLog has a tab in it, so commit-queue would reject it.

[Mahesh Kulkarni]

Created attachment 62540 [details]
removed tab 

removed ChangeLog tabs for commit scripts to work

[WebKit Commit Bot]

Comment on attachment 62540 [details]
removed tab 

Rejecting patch 62540 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Last 500 characters of output:
't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: Scripts/old-run-webkit-tests
|===================================================================
|--- Scripts/old-run-webkit-tests	(revision 64021)
|+++ Scripts/old-run-webkit-tests	(working copy)
--------------------------
No file to patch.  Skipping patch.
2 out of 2 hunks ignored
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Full output: http://queues.webkit.org/results/3593499

[Eric Seidel (no email)]

Comment on attachment 62526 [details]
patch

Cleared Darin Adler's review+ from obsolete attachment 62526 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Andreas Kling]

Committed r64123: <http://trac.webkit.org/changeset/64123>

[Eric Seidel (no email)]

Comment on attachment 62540 [details]
removed tab 

Seems it would be better to just check out the fonts for them and store them somewhere.  Like WebKitLibraries or the build directory or whatever.

[Dumitru Daniliuc]

i don't think webkit's cygwin distribution comes with git (i don't have it), so i think we should either add it there, or at the very least change the error message on windows and tell users how to get the fonts without git.

also, out of curiosity, why did it become a problem now? were new tests added? and if those fonts are indeed needed by the gtk and win ports, then maybe they should be moved to a less qt-specific location?

[Eric Seidel (no email)]

Comment on attachment 62540 [details]
removed tab 

Is isCygwin() right?  Won't that error for Apple's Win port?  Maybe it's right for it to do so?

If we don't have git, we could fail to auto-pull the fonts, but if we do, we should just pull them.  I also suspect it should be possible to pull them w/o git. :)

[Mahesh Kulkarni]

(In reply to comment #8)
> i don't think webkit's cygwin distribution comes with git (i don't have it), so i think we should either add it there, or at the very least change the error message on windows and tell users how to get the fonts without git.
> 

The total size of fonts used are 19.9MB. Would it be overkill to just add those to webkit svn repository? 

GIT is not mandatory. One could download the zip file from the location though. 

> also, out of curiosity, why did it become a problem now? were new tests added? and if those fonts are indeed needed by the gtk and win ports, then maybe they should be moved to a less qt-specific location?

No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message.

[Mahesh Kulkarni]

(In reply to comment #9)
> (From update of attachment 62540 [details])
> Is isCygwin() right?  Won't that error for Apple's Win port?  Maybe it's right for it to do so?
> 

Apple's win port also uses testfont for layout testing. 

> If we don't have git, we could fail to auto-pull the fonts, but if we do, we should just pull them.  I also suspect it should be possible to pull them w/o git. :)

I agree with your comment #7 suggestion, having it those fonts in trunk would make sense. As long as check-out of 20mb more is a problem?

[Dumitru Daniliuc]

> No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message.

i don't think this is true. the win and gtk bots were happy to run all tests without these fonts.

[Mahesh Kulkarni]

(In reply to comment #12)
> > No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message.
> 
> i don't think this is true. the win and gtk bots were happy to run all tests without these fonts.

Actually if you check the patch,

 elsif (isGtk()) {
     $platform = "gtk";
-    if (!$ENV{"WEBKIT_TESTFONTS"}) {
-        print "The WEBKIT_TESTFONTS environment variable is not defined.\n";
-        print "You must set it before running the tests.\n";
-        print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n";
-        exit 1;
-    }

GTK always had this check. This patch just moves it to QT and win port as well. 
On #webkit IRC, I came to know apple's win port uses this env variable so isCygwin() also added to the check

[Antonio Gomes]

> Actually if you check the patch,
> 
>  elsif (isGtk()) {
>      $platform = "gtk";
> -    if (!$ENV{"WEBKIT_TESTFONTS"}) {
> -        print "The WEBKIT_TESTFONTS environment variable is not defined.\n";
> -        print "You must set it before running the tests.\n";
> -        print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n";
> -        exit 1;
> -    }
> 
> GTK always had this check. This patch just moves it to QT and win port as well. 
> On #webkit IRC, I came to know apple's win port uses this env variable so isCygwin() also added to the check

Not sure about cygwin, but for Qt is it a valuable check, yes.