Bug 194846 – [WinCairo] Enable wk1/wk2 suffix for platform search path.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Basuke Suzuki]

Currently WinCairo doesn't look into wincairo-wk2 or wincairo-wk1 directory. This patch allows that to enable WebKit or WebKit Legacy specific expectations.

[Basuke Suzuki]

Created attachment 362466 [details]
Patch

[Fujii Hironori]

Comment on attachment 362466 [details]
Patch

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

> LayoutTests/platform/wincairo-wk1/TestExpectations:16
> +http/tests/websocket/tests/hybi/network-process-crash-error.html [ Skip ]

It seems that all WK1 TestExpectations has this line, e.g. ios-wk1/TestExpectations and mac-wk1/TestExpectations.
In such case, this line should be moved into the top TestExpectations, and platform/wk2/TestExpectations should have the following line to enable this test only in WK2.
> http/tests/websocket/tests/hybi/network-process-crash-error.html [ Pass ]

WinCairo WK2 takes platform/wk2/TestExpectations into account.

[Fujii Hironori]

Comment on attachment 362466 [details]
Patch

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

> Tools/Scripts/webkitpy/port/win.py:503
> +        for version in versions:

I don't like this code because this is too generic even though this is a WinCairoPort's method.
It seems that you are following mac.py.
Actually mac port is maintianing following TestExpectations.

mac
mac-highsierra
mac-highsierra-wk1
mac-sierra
mac-sierra-wk1
mac-sierra-wk2
mac-wk1
mac-wk2

However, WinCairo port doesn't have a plan to maintain Windows version specific TestExpectations.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 362466 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362466&action=review
> 
> > LayoutTests/platform/wincairo-wk1/TestExpectations:16
> > +http/tests/websocket/tests/hybi/network-process-crash-error.html [ Skip ]
> 
> It seems that all WK1 TestExpectations has this line, e.g.
> ios-wk1/TestExpectations and mac-wk1/TestExpectations.
> In such case, this line should be moved into the top TestExpectations, and
> platform/wk2/TestExpectations should have the following line to enable this
> test only in WK2.
> > http/tests/websocket/tests/hybi/network-process-crash-error.html [ Pass ]
> 
> WinCairo WK2 takes platform/wk2/TestExpectations into account.

That's not the point of this patch. This patch just try to enable wk1/wk2 platform variation to WinCairo. That purpose should be on separate patch.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 362466 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362466&action=review
> 
> > Tools/Scripts/webkitpy/port/win.py:503
> > +        for version in versions:
> 
> I don't like this code because this is too generic even though this is a
> WinCairoPort's method.

I'm sorry, I can't understand. This is the basis of structured programming that once the context is defined, it shouldn't be verbose. In this context, the method is implemented in WinCairoPort. In the method, version is meant to be for WinCairo. 


> It seems that you are following mac.py.
> Actually mac port is maintianing following TestExpectations.
> 
> mac
> mac-highsierra
> mac-highsierra-wk1
> mac-sierra
> mac-sierra-wk1
> mac-sierra-wk2
> mac-wk1
> mac-wk2
> 
> However, WinCairo port doesn't have a plan to maintain Windows version
> specific TestExpectations.

No, we already support wincairo-win10, wincairo-win7 or such. Again, what's your point?

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #5)
> > However, WinCairo port doesn't have a plan to maintain Windows version
> > specific TestExpectations.
> 
> No, we already support wincairo-win10, wincairo-win7 or such. Again, what's
> your point?

What do you mean? No such sub directories (wincairo-win10 and wincairo-win7) in the following directory:
https://trac.webkit.org/browser/webkit/trunk/LayoutTests/platform

[Fujii Hironori]

I understand what you mean. You mean origianl WinCairoPort class already supports wincairo-win10 and wincairo-win7. That's right.
But, I'm talking about maintaining TestExpectations.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #7)
> I understand what you mean. You mean origianl WinCairoPort class already
> supports wincairo-win10 and wincairo-win7. That's right.
> But, I'm talking about maintaining TestExpectations.

We don't have to maintain whole available expectations. This separation makes the accuracy of TestExpectations better. I don't like the test with [Pass Failure] because of the difference between WebKit and Legacy.

Same thing for Windows version. What if somebody found a bug when it fails on Windows 7? We don't plan to maintain that, but the guy complains. With the version separation, we simply open a new bug for that guy, add a line to wincairo-win7/TestExpectations with that bug id and that's it.

I think that's why both wk1/wk2 separation and Windows version separation are good to have to maintain TextExpectations healthy. What do you think?

[Don Olmstead]

Comment on attachment 362466 [details]
Patch

r=me

We can just remove this when we fully switch to WebKitTestRunner.

[Basuke Suzuki]

Comment on attachment 362466 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 362466 [details]
Patch

Clearing flags on attachment: 362466

Committed r241979: <https://trac.webkit.org/changeset/241979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48333562>