Bug 106187 – [Mac Lion] [WK2] tiled-drawing tests are being run when they shouldn’t be

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

The following tests have been failing on Lion WebKit2 bots:
platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html
platform/mac/tiled-drawing/fixed/four-bars-zoomed.html
platform/mac/tiled-drawing/fixed/negative-scroll-offset.html
platform/mac/tiled-drawing/sticky/sticky-horizontal.html
platform/mac/tiled-drawing/sticky/sticky-vertical.html

e.g.
http://build.webkit.org/results/Apple%20Lion%20Release%20WK2%20(Tests)/r138908%20(6663)/results.html

[Radar WebKit Bug Importer]

<rdar://problem/12962620>

[Ryosuke Niwa]

Added test expectations in http://trac.webkit.org/changeset/138912.

[Alexey Proskuryakov]

When did this start? Can this be the same as bug 106205?

[Simon Fraser (smfr)]

tiled drawing tests should be skipped on Lion. This used to be impossible because of TestExpectations order, but that was fixed.

[Simon Fraser (smfr)]

Bug 105583 fixed the TestExpectations fallback order.

[Simon Fraser (smfr)]

These tests should not be skipped for WK2 on all platforms; that's just disabling testing.

[Ryosuke Niwa]

(In reply to comment #6)
> These tests should not be skipped for WK2 on all platforms; that's just disabling testing.

Oh oops :/ Somehow I forgot to add [ Lion ] there. Fixing that now and skipping them instead.

[Jessie Berlin]

This should be fixed in http://trac.webkit.org/changeset/141323

[Simon Fraser (smfr)]

That didn't remove these lines from platform/mac-wk2/TestExpectations:

webkit.org/b/106187 platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html [ Failure ]
webkit.org/b/106187 platform/mac/tiled-drawing/fixed/four-bars-zoomed.html [ Failure ]
webkit.org/b/106187 platform/mac/tiled-drawing/fixed/negative-scroll-offset.html [ Failure ]
webkit.org/b/106187 platform/mac/tiled-drawing/sticky/sticky-horizontal.html [ Failure ]
webkit.org/b/106187 platform/mac/tiled-drawing/sticky/sticky-vertical.html [ Failure ]

[Jessie Berlin]

(In reply to comment #9)
> That didn't remove these lines from platform/mac-wk2/TestExpectations:
> 
> webkit.org/b/106187 platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html [ Failure ]
> webkit.org/b/106187 platform/mac/tiled-drawing/fixed/four-bars-zoomed.html [ Failure ]
> webkit.org/b/106187 platform/mac/tiled-drawing/fixed/negative-scroll-offset.html [ Failure ]
> webkit.org/b/106187 platform/mac/tiled-drawing/sticky/sticky-horizontal.html [ Failure ]
> webkit.org/b/106187 platform/mac/tiled-drawing/sticky/sticky-vertical.html [ Failure ]

Removed those lines in http://trac.webkit.org/changeset/141329

[Jessie Berlin]

Apparently 

platform/mac/tiled-drawing/ [ Pass ]

is considered more specific than

[ Lion ] platform/mac/tiled-drawing/ [ Skip ]

?

http://build.webkit.org/builders/Apple%20Lion%20Release%20WK2%20%28Tests%29/builds/7471/steps/layout-test/logs/stdio
15:30:00.382 68230 LayoutTests/platform/mac-wk2/TestExpectations:413 More specific entry for platform/mac/tiled-drawing/ on line LayoutTests/platform/mac-wk2/TestExpectations:412 overrides line LayoutTests/platform/mac-wk2/TestExpectations:413. platform/mac/tiled-drawing/
15:30:00.382 68230 LayoutTests/platform/mac-wk2/TestExpectations:413 More specific entry for platform/mac/tiled-drawing/ on line LayoutTests/platform/mac-wk2/TestExpectations:412 overrides line LayoutTests/platform/mac-wk2/TestExpectations:413. platform/mac/tiled-drawing/

So even that fix is not working here.

[Jessie Berlin]

Looking at test_expectations.py:

        # At this point we know we have seen a previous exact match on this                                                                                                                                       
        # base path, so we need to check the two sets of modifiers.                                                                                                                                               

        # FIXME: This code was originally designed to allow lines that matched                                                                                                                                    
        # more modifiers to override lines that matched fewer modifiers.                                                                                                                                          
        # However, we currently view these as errors.                                                                                                                                                             
        #                                                                                                                                                                                                         
        # To use the "more modifiers wins" policy, change the errors for overrides                                                                                                                                
        # to be warnings and return False".

[Dirk Pranke]

Looking ... I think perhaps this is a buggy error message; at the very least, line 413 is more specific than line 412. I'm also not sure why you're getting that error so many times.

[Jessie Berlin]

(In reply to comment #13)
> Looking ... I think perhaps this is a buggy error message; at the very least, line 413 is more specific than line 412. I'm also not sure why you're getting that error so many times.

I think I have a fix, I am just testing it.

[Dirk Pranke]

Yup, buggy error message (the two line numbers are swapped).

At any rate, you can't have both qualified and unqualified lines in a single file like that. As the comment you found indicates, having multiple lines that both match a single configuration is an error. We used to support more-specific overriding less-specific, but it seemed to be unwarranted complexity.

In this case, I would replace the two lines:

platform/mac/tiled-drawing/ [ Pass ]                           
[ Lion ] platform/mac/tiled-drawing/ [ Skip ]                                   

With just:

[ MountainLion ] platform/mac/tiled-drawing/ [ Pass ]

There is no current way to "futureproof" things so that all WK1 and only Lion WK2 is skipped. You would need to add either a specific WK1 TestExpectations file or add support for WK1 and WK2 keywords. I would recommend doing the former, especially if you plan to continue deprecating WK1 on Apple Mac more and more.

[Ryosuke Niwa]

(In reply to comment #15)
> Yup, buggy error message (the two line numbers are swapped).
> 
> At any rate, you can't have both qualified and unqualified lines in a single file like that. As the comment you found indicates, having multiple lines that both match a single configuration is an error. We used to support more-specific overriding less-specific, but it seemed to be unwarranted complexity.
> 
> In this case, I would replace the two lines:
> 
> platform/mac/tiled-drawing/ [ Pass ]                           
> [ Lion ] platform/mac/tiled-drawing/ [ Skip ]                                   
> 
> With just:
> 
> [ MountainLion ] platform/mac/tiled-drawing/ [ Pass ]

That wouldn't work because we need to make it pass on MacFuture as well.

[Dirk Pranke]

(In reply to comment #16)
> > In this case, I would replace the two lines:
> > 
> > platform/mac/tiled-drawing/ [ Pass ]                           
> > [ Lion ] platform/mac/tiled-drawing/ [ Skip ]                                   
> > 
> > With just:
> > 
> > [ MountainLion ] platform/mac/tiled-drawing/ [ Pass ]
> 
> That wouldn't work because we need to make it pass on MacFuture as well.

That is what I was getting at with my last paragraph. I don't think "Future" is a supported keyword, but we could probably add it. It's not really the right solution, though.

[Jessie Berlin]

For this particular bug, I think moving the tests from platform/mac to platform/mac-wk2 and adding an entry in the mac-lion TestExpectations file to skip them is more appropriate, since the feature is mac-wk2 only.

I will post a patch that does that shortly.

[Jessie Berlin]

Created attachment 185649 [details]
Patch

[Dirk Pranke]

bug 108146 filed to discuss adding -wk1 specific directories and expectations files.

[Jessie Berlin]

Comment on attachment 185649 [details]
Patch

Committed in http://trac.webkit.org/changeset/141362