Bug 75417

Summary: NRWT should use test_expectation.txt on wk2 platforms
Product: WebKit Reporter: Balazs Kelemen <kbalazs@webkit.org>
Component: WebKit2Assignee: Csaba Osztrogonác <ossy@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, dpranke@chromium.org, eric@webkit.org, jchaffraix@webkit.org, ojan@chromium.org, ossy@webkit.org, robert@webkit.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description From 2012-01-01 11:57:55 PST
Tests added in http://trac.webkit.org/changeset/103875 has metric differences between Qt-WK1/2 which is weird. Need to investigate wetter it can be fixed without diverging the expectations.

An example diff:
--- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/css/caption-width-absolute-position-offset-top-expected.txt 
+++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/css/caption-width-absolute-position-offset-top-actual.txt 
@@ -10,7 +10,7 @@
         RenderTableSection {TBODY} at (0,2) size 106x36
           RenderTableRow {TR} at (0,0) size 106x36
             RenderTableCell {TD} at (0,15) size 106x6 [border: (2px solid #FFA500)] [r=0 c=0 rs=1 cs=1]
-layer at (8,100) size 56x20
-  RenderBlock (positioned) {CAPTION} at (8,100) size 56x20 [border: (4px solid #008000)]
-    RenderText {#text} at (4,4) size 48x12
+layer at (8,100) size 56x21
+  RenderBlock (positioned) {CAPTION} at (8,100) size 56x21 [border: (4px solid #008000)]
+    RenderText {#text} at (4,4) size 48x13
       text run at (4,4) width 48: "xxxx"
------- Comment #1 From 2012-01-01 12:14:18 PST -------
As a first I skipped them in http://trac.webkit.org/changeset/103881.
------- Comment #2 From 2012-01-02 03:25:12 PST -------
See http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r103875%20%2817825%29/results.html for details.
------- Comment #3 From 2012-01-02 05:16:29 PST -------
Find out that these needs platform results. The strange thing is that these tests was supposed to be skipped on Qt by test_expectations, but they still failed on wk2. This is likely an NRWT bug.

*** This bug has been marked as a duplicate of bug 74888 ***
------- Comment #4 From 2012-01-02 06:09:58 PST -------
path_to_test_expectations_file() in webkit.py:
    def path_to_test_expectations_file(self):
        # test_expectations are always in mac/ not mac-leopard/ by convention, hence we use port_name instead of name().
        expectations_directory = self._wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name
        return self._filesystem.join(self._webkit_baseline_path(expectations_directory), 'test_expectations.txt')

It returns qt-wk2 incorrectly. As far as I know nobody uses platform-wk2/test_expectations.txt, because there isn't any checked in test_expectations.txt in these directories.
------- Comment #5 From 2012-01-02 06:27:03 PST -------
This change introduced in http://trac.webkit.org/changeset/90077 . 
See https://bugs.webkit.org/show_bug.cgi?id=63501#c6 for details.

This change was implemented to avoid duplicated expectations (mac, mac-wk2, test_expectations.txt vs Skipped files) But now there is separated wk2/Skipped file instead of including mac-wk2/Skipped for all wk2 port and now NRWT handles duplicated expectations correctly - Skipped list entry is stronger than a test_expectation.txt entry.

So I propose reverting this part of the original change to enable test_expecatitons.txt on wk2 platforms too.
------- Comment #6 From 2012-01-02 06:32:42 PST -------
Created an attachment (id=120878) [details]
Patch
------- Comment #7 From 2012-01-02 09:55:17 PST -------
(From update of attachment 120878 [details])
Please test this.
------- Comment #8 From 2012-01-03 03:54:05 PST -------
Created an attachment (id=120930) [details]
Patch
------- Comment #9 From 2012-01-04 13:13:51 PST -------
(From update of attachment 120930 [details])
So is the intent that we have a single test_expectations.txt file for both wk1 and wk2? Do we need to distinguish different results for each, and, if so, how will you do that?
------- Comment #10 From 2012-01-05 01:49:42 PST -------
(In reply to comment #9)
> (From update of attachment 120930 [details] [details])
> So is the intent that we have a single test_expectations.txt file for both wk1 and wk2? Do we need to distinguish different results for each, and, if so, how will you do that?

This is how NRWT works. It is a question wetter it should be improved - for example kkristof has a patch for cascading expectations - but it does not belong to this bug. The patch is an easy fix for a real bug, so I hope we can short-circuit this story.
------- Comment #11 From 2012-01-05 07:49:15 PST -------
(In reply to comment #9)
> (From update of attachment 120930 [details] [details])
> So is the intent that we have a single test_expectations.txt file for both wk1 and wk2? Do we need to distinguish different results for each, and, if so, how will you do that?

There are only 4 test_expectations.txt: qt, gtk, chromium, mac and win. Now nobody uses actively test_expectations.txt except chromuium port, everybody prefers Skipped lists (because it is simpler and you can use cascaded Skipped lists.) And we can add different results into qt,qt-wk2 ; mac,mac-wk2 and gtk,qtk-wk2 dirs irrespectively of test_expectations.txt

But now NRWT ignores test_expectations.txt files for wk2 platforms, which isn't correct. Chromium guys usually add tests into test_expectations.txt not to break bots and let the port maintainters do rebaselines themselves.
------- Comment #12 From 2012-01-05 10:14:25 PST -------
(From update of attachment 120930 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=120930&action=review

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:97
> +    def test_path_to_test_expectations_file(self):
> +        port = TestWebKitPort()
> +        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/test_expectations.txt')

I don't see how this tests your change.  You want to test wha thappens in the wk2 case, no?
------- Comment #13 From 2012-01-05 10:49:00 PST -------
(In reply to comment #12)
> (From update of attachment 120930 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120930&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:97
> > +    def test_path_to_test_expectations_file(self):
> > +        port = TestWebKitPort()
> > +        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/test_expectations.txt')
> 
> I don't see how this tests your change.  You want to test wha thappens in the wk2 case, no?

Ossy, I recommend to test wetter the file exist, in the wk2 case.
At least that way it can not regress again.
------- Comment #14 From 2012-01-05 11:47:14 PST -------
(From update of attachment 120930 [details])
The change in webkit.py looks fine. Please add tests for both with and without the --webkit-test-runner option just to get coverage of both options.
------- Comment #15 From 2012-01-05 13:40:58 PST -------
Created an attachment (id=121324) [details]
Patch
------- Comment #16 From 2012-01-05 14:11:22 PST -------
(From update of attachment 121324 [details])
looks good. thank you!
------- Comment #17 From 2012-01-06 00:14:13 PST -------
(From update of attachment 121324 [details])
Clearing flags on attachment: 121324

Committed r104273: <http://trac.webkit.org/changeset/104273>
------- Comment #18 From 2012-01-06 00:14:22 PST -------
All reviewed patches have been landed.  Closing bug.