Summary: | [WinCairo] Enable wk1/wk2 suffix for platform search path. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||
Component: | Tools / Tests | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, bfulgham, commit-queue, don.olmstead, ews-watchlist, glenn, Hironori.Fujii, lforschler, pvollan, ross.kirsling, webkit-bug-importer, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Basuke Suzuki
2019-02-19 18:02:58 PST
Created attachment 362466 [details]
Patch
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. 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. (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. (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? (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 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. (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? Comment on attachment 362466 [details]
Patch
r=me
We can just remove this when we fully switch to WebKitTestRunner.
Comment on attachment 362466 [details]
Patch
Thanks!
Comment on attachment 362466 [details] Patch Clearing flags on attachment: 362466 Committed r241979: <https://trac.webkit.org/changeset/241979> All reviewed patches have been landed. Closing bug. |