Bug 80709

Summary: Convert regions parsing test to use testharness.js
Product: WebKit Reporter: Jacob Goldstein <jacobg>
Component: Tools / TestsAssignee: Jacob Goldstein <jacobg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, jacobg, krit, rniwa, webkit.review.bot, yosin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.7   
Attachments:
Description Flags
Patch
none
Patch none

Description Jacob Goldstein 2012-03-09 10:25:20 PST
Update test to use testharness.js for synergy between W3C and WebKit test style
Comment 1 Jacob Goldstein 2012-03-09 12:52:28 PST
Created attachment 131079 [details]
Patch
Comment 2 Adam Barth 2012-03-09 15:06:34 PST
Comment on attachment 131079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131079&action=review

> LayoutTests/fast/regions/webkit-flow-parsing-expected.txt:26
> +Result	Test Name	Message
> +Pass	testParse: Assigned none to property, expected return = none	
> +Pass	testParse: Assigned first-flow to property, expected return = first-flow	
> +Pass	testParse: Assigned 'first-flow' to property, expected return = ''	
> +Pass	testParse: Assigned ; to property, expected return = ''	
> +Pass	testParse: Assigned 1 to property, expected return = ''	
> +Pass	testParse: Assigned 1.2 to property, expected return = ''	
> +Pass	testParse: Assigned -1 to property, expected return = ''	
> +Pass	testParse: Assigned 12px to property, expected return = ''	
> +Pass	testComputedStyle: Assigned none to property, expected return = none	
> +Pass	testComputedStyle: Assigned \"\" to property, expected return = none	
> +Pass	testComputedStyle: Assigned 'first-flow' to property, expected return = none	
> +Pass	testComputedStyle: Assigned first-flow to property, expected return = first-flow	
> +Pass	testComputedStyle: Assigned 12px to property, expected return = none	
> +Pass	testNotInherited: Assigned none, none to property, expected return = none	
> +Pass	testNotInherited: Assigned none, child-flow to property, expected return = child-flow	
> +Pass	testNotInherited: Assigned parent-flow, none to property, expected return = none	
> +Pass	testNotInherited: Assigned parent-flow, child-flow to property, expected return = child-flow	

This is hard to read in a text dump.  Did the tests pass or fail/  The Result column appears to be empty.  Given that text dumps is the main way we look at these results, I'm not sure this is an improvement.
Comment 3 Jacob Goldstein 2012-03-09 15:50:25 PST
I understand your concern.  Some of this is user defined, and the output could also be improved via customizations to the testharnessreport.js file.

The message you see after "Pass", for example, "testParse: Assigned none to property, expected return = none", is the user defined string from the description argument of the assert_equals method.   assert_equals looks like this:

assert_equals(actual, expected, description)

All of the methods in testharness.js include a description argument that is output after the test result.  In the W3C test suite, the testharness.css file formats the output into a slightly more readable output.  Since we are dumping as text, that isn't possible, but other customizations in testharnessreport.js are.  I will look into this further.




(In reply to comment #2)
> (From update of attachment 131079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131079&action=review
> 
> > LayoutTests/fast/regions/webkit-flow-parsing-expected.txt:26
> > +Result	Test Name	Message
> > +Pass	testParse: Assigned none to property, expected return = none	
> > +Pass	testParse: Assigned first-flow to property, expected return = first-flow	
> > +Pass	testParse: Assigned 'first-flow' to property, expected return = ''	
> > +Pass	testParse: Assigned ; to property, expected return = ''	
> > +Pass	testParse: Assigned 1 to property, expected return = ''	
> > +Pass	testParse: Assigned 1.2 to property, expected return = ''	
> > +Pass	testParse: Assigned -1 to property, expected return = ''	
> > +Pass	testParse: Assigned 12px to property, expected return = ''	
> > +Pass	testComputedStyle: Assigned none to property, expected return = none	
> > +Pass	testComputedStyle: Assigned \"\" to property, expected return = none	
> > +Pass	testComputedStyle: Assigned 'first-flow' to property, expected return = none	
> > +Pass	testComputedStyle: Assigned first-flow to property, expected return = first-flow	
> > +Pass	testComputedStyle: Assigned 12px to property, expected return = none	
> > +Pass	testNotInherited: Assigned none, none to property, expected return = none	
> > +Pass	testNotInherited: Assigned none, child-flow to property, expected return = child-flow	
> > +Pass	testNotInherited: Assigned parent-flow, none to property, expected return = none	
> > +Pass	testNotInherited: Assigned parent-flow, child-flow to property, expected return = child-flow	
> 
> This is hard to read in a text dump.  Did the tests pass or fail/  The Result column appears to be empty.  Given that text dumps is the main way we look at these results, I'm not sure this is an improvement.
Comment 4 Adam Barth 2012-03-09 16:13:59 PST
It's interesting that the result came out much clearer in the bugzilla comments.
Comment 5 Jacob Goldstein 2012-03-16 14:17:10 PDT
Created attachment 132376 [details]
Patch

Updated to format out from testharness.js such that an HTML table is no longer used and the text file produced by dumpAsText is readable
Comment 6 Jacob Goldstein 2012-03-16 14:22:52 PDT
...for format out[put] from...

(In reply to comment #5)
> Created an attachment (id=132376) [details]
> Patch
> 
> Updated to format out from testharness.js such that an HTML table is no longer used and the text file produced by dumpAsText is readable
Comment 7 Adam Barth 2012-03-16 14:32:01 PDT
Comment on attachment 132376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132376&action=review

> LayoutTests/fast/regions/webkit-flow-parsing-expected.txt:19
> +PASS Test Parse none 
> +PASS Test Parse first-flow 
> +PASS Test Parse 'first-flow' 
> +PASS Test Parse ; 
> +PASS Test Parse 1 
> +PASS Test Parse 1.2 
> +PASS Test Parse -1 
> +PASS Test Parse 12px 
> +PASS Test Computed Style none 
> +PASS Test Computed Style '' 
> +PASS Test Computed Style 'first-flow' 
> +PASS Test Computed Style first-flow 
> +PASS Test Computed Style 12px  
> +PASS Test Non-Inherited none, none 
> +PASS Test Non-Inherited none, child-flow 
> +PASS Test Non-Inherited parent-flow, none 
> +PASS Test Non-Inherited parent-flow, child-flow 

This output is much better.  Thanks.
Comment 8 Jacob Goldstein 2012-03-20 09:54:19 PDT
Ready for review
Comment 9 Ryosuke Niwa 2012-03-20 10:26:39 PDT
It might make sense to move testharness.js to resources/w3c/ to signify the fact it's imported from w3c repository.
Comment 10 WebKit Review Bot 2012-03-20 10:56:32 PDT
Comment on attachment 132376 [details]
Patch

Clearing flags on attachment: 132376

Committed r111411: <http://trac.webkit.org/changeset/111411>
Comment 11 WebKit Review Bot 2012-03-20 10:56:38 PDT
All reviewed patches have been landed.  Closing bug.