Bug 75417 - NRWT should use test_expectation.txt on wk2 platforms
Summary: NRWT should use test_expectation.txt on wk2 platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-01 11:57 PST by Balazs Kelemen
Modified: 2012-01-06 00:14 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2012-01-02 06:32 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2012-01-03 03:54 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2012-01-05 13:40 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 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 Balazs Kelemen 2012-01-01 12:14:18 PST
As a first I skipped them in http://trac.webkit.org/changeset/103881.
Comment 3 Balazs Kelemen 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 Csaba Osztrogonác 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 Csaba Osztrogonác 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 Csaba Osztrogonác 2012-01-02 06:32:42 PST
Created attachment 120878 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-01-02 09:55:17 PST
Comment on attachment 120878 [details]
Patch

Please test this.
Comment 8 Csaba Osztrogonác 2012-01-03 03:54:05 PST
Created attachment 120930 [details]
Patch
Comment 9 Dirk Pranke 2012-01-04 13:13:51 PST
Comment on attachment 120930 [details]
Patch

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 Balazs Kelemen 2012-01-05 01:49:42 PST
(In reply to comment #9)
> (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?

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 Csaba Osztrogonác 2012-01-05 07:49:15 PST
(In reply to comment #9)
> (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?

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 Eric Seidel (no email) 2012-01-05 10:14:25 PST
Comment on attachment 120930 [details]
Patch

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 Balazs Kelemen 2012-01-05 10:49:00 PST
(In reply to comment #12)
> (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?

Ossy, I recommend to test wetter the file exist, in the wk2 case.
At least that way it can not regress again.
Comment 14 Dirk Pranke 2012-01-05 11:47:14 PST
Comment on attachment 120930 [details]
Patch

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 Csaba Osztrogonác 2012-01-05 13:40:58 PST
Created attachment 121324 [details]
Patch
Comment 16 Dirk Pranke 2012-01-05 14:11:22 PST
Comment on attachment 121324 [details]
Patch

looks good. thank you!
Comment 17 Csaba Osztrogonác 2012-01-06 00:14:13 PST
Comment on attachment 121324 [details]
Patch

Clearing flags on attachment: 121324

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