Bug 75417 – NRWT should use test_expectation.txt on wk2 platforms

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

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"

[Balazs Kelemen]

As a first I skipped them in http://trac.webkit.org/changeset/103881.

[Csaba Osztrogonác]

See http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r103875%20%2817825%29/results.html for details.

[Balazs Kelemen]

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 ***

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 120878 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 120878 [details]
Patch

Please test this.

[Csaba Osztrogonác]

Created attachment 120930 [details]
Patch

[Dirk Pranke]

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?

[Balazs Kelemen]

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

[Csaba Osztrogonác]

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

[Eric Seidel (no email)]

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?

[Balazs Kelemen]

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

[Dirk Pranke]

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.

[Csaba Osztrogonác]

Created attachment 121324 [details]
Patch

[Dirk Pranke]

Comment on attachment 121324 [details]
Patch

looks good. thank you!

[Csaba Osztrogonác]

Comment on attachment 121324 [details]
Patch

Clearing flags on attachment: 121324

Committed r104273: <http://trac.webkit.org/changeset/104273>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.