Bug 215543

Created attachment 406673 [details] Patch

Comment on attachment 406673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406673&action=review > LayoutTests/platform/ios-simulator/TestExpectations:124 > +# This test requires force input and is skiped in the general case. Probably should fix the spelling of "skipped", but also, what does this comment mean? The whole directory is skipped for non-iOS, and it seems we should un-skip the whole directory, not a single file. I don’t think we need to say "requires force input"; these tests are in a directory named iOS.

Comment on attachment 406673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406673&action=review This moves partly obsolete iOS 14 expectations on top of iOS 13 expectations. This should not be done mechanically, as this is the time where we verify which ones got fixed throughout the development cycle (see some examples below). But if done mechanically, LayoutTests/platform/ios-13 should be updated to not lose test coverage in the meanwhile. > LayoutTests/platform/ios-device/TestExpectations:143 > +fast/events/ios/rotation [ Skip ] # https://bugs.webkit.org/show_bug.cgi?id=175800 All fast/events/ios tests are skipped in open source, so moving this line from internal does not make sense. > LayoutTests/platform/ios-device/TestExpectations:148 > +# <rdar://problem/42138622> [iOS] WebGL conformance LayoutTests failing when run on device I wonder if this is still the case, now that we have IOSurface in simulator. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:134 > +# <rdar://problem/51449034> This radar was fixed a long time ago. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:135 > +[ Debug ] media/W3C/audio/events/event_progress.html [ Pass Failure ] But this test used to be flaky on some old internal builds of iOS 14. Not any more though, and never in open source in recent memory. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:136 > +[ Debug ] media/W3C/video/events/event_order_loadstart_progress.html [ Pass Failure ] This test didn't fail in recent memory. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:139 > +# <rdar://problem/60227623> [ wk1 and wk2 ] fast/text/international/generic-font-family-language-traditional.html is failing. > +fast/text/international/generic-font-family-language-traditional.html [ ImageOnlyFailure ] This test isn't failing, and the radar is fixed. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:142 > +# <rdar://problem/61889653> [ wk2 Guard-Malloc ] fast/block/nested-renderers.html is flaky crashing with alert - WebCore::FrameLoader::commitProvisionalLoad() + 5563 > +fast/block/nested-renderers.html [ Pass Crash ] This was a one-time memory corruption crash outside WebKit, we shouldn't mark the test as a flaky crash because of that. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:145 > +# <rdar://problem/22064820> Test compositing/backing/form-controls-backing.html failing on iOS Simulator since it was added > +compositing/backing/form-controls-backing.html [ Failure Timeout ] There weren't any timeouts in recent memory. There is already a Failure expectation for this test LayoutTests/platform/ios/TestExpectations, adding another one in this file is not helpful. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:149 > +# <rdar://problem/63539061> REGRESSION: (r261506?): [ iOS wk2 ] http/tests/in-app-browser-privacy/app-bound-domain-gets-app-bound-session.html is failing consistently. > +http/tests/in-app-browser-privacy/app-bound-domain-gets-app-bound-session.html [ Failure ] > +http/tests/in-app-browser-privacy/non-app-bound-iframe-under-app-bound-domain-is-app-bound.html [ Failure ] This radar was fixed in June. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:152 > +# <rdar://66598268> ([ wk2 ] quicklook/powerpoint-legacy.html is a constant failure) > +quicklook/powerpoint-legacy.html [ Pass Failure ] This is an underlying framework bug in iOS 14, we should not add expectations for it in open source and reduce iOS 13 coverage now. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:155 > +# rdar://67034553 ([ Layout Tests ] REGRESSION (r295093?): [ wk2 ] animations/unprefixed-events-mixed-with-prefixed.html is a flaky failure) > +animations/unprefixed-events-mixed-with-prefixed.html Not really sure what's going on with this once, but it's also claimed to be an iOS 14 issue. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:158 > +# rdar://67079948 ([ Layout Tests ] REGRESSION (r265515): [ wk2 ] fast/events/touch/ios/touch-events-with-modifiers.html is a constant failure) > +fast/events/touch/ios/touch-events-with-modifiers.html [ Pass Failure ] This test shouldn't be running in open source at all, as touch events implementation is different. The whole fast/events/touch/ios directory is skipped in open source, so this incorrectly unskips the test. Like for all internal only tests, this test's expectation should not be mixed with open source expectations. > LayoutTests/platform/ios-simulator/TestExpectations:114 > +# <rdar://problem/35756253> REGRESSION (17E105): 8 canvas test failures > +canvas/philip/tests/2d.shadow.image.scale.html [ Failure ] This radar was fixed in 2018, and at least this first test is passing on iOS Simulator, didn't check others in this group. > LayoutTests/platform/ios-simulator/TestExpectations:122 > +# rdar://52615537 (UIHelper.setKeyboardInputModeIdentifier("zh_Hans-Pinyin@sw=Pinyin-Simplified;hw=Automatic") does not switch to Pinyin unless sim configured with Pinyin (199472)) > +fast/events/ios/keydown-keyup-keypress-keys-in-non-editable-using-chinese-keyboard.html [ Skip ] All fast/events/ios tests are skipped in open source, so moving the expectation from internal does not make sense. >> LayoutTests/platform/ios-simulator/TestExpectations:124 >> +# This test requires force input and is skiped in the general case. > > Probably should fix the spelling of "skipped", but also, what does this comment mean? The whole directory is skipped for non-iOS, and it seems we should un-skip the whole directory, not a single file. I don’t think we need to say "requires force input"; these tests are in a directory named iOS. Most of the tests are run using iPhone SE simulator, where pressure events don't work AFAIK. There are select few that run using iPhone 7 simulator. I think that the internal expectations are confused. The test was made iPhone 7 only in rdar://53458890, but then the expectation was moved into the general expectations file in an "unreviewed gardening" patch.

I’m not sure I understand the mechanism that prevented these expectations from being used when running tests on iOS 13. Where is the code that implements that rule? I assumed many of these were wrong and your review highlights that. But this patch does not introduce any of those mistakes unless it’s incorrectly mixing iOS 14 with 13. I propose to migrate these so they are not hard to see for open source contributors and alongside the other expectations. It seems to me that moving these to open source and correcting the many mistakes are not the same task. I’d love to have many of these mistakes corrected either before or after migrating to open source. I believe that migration is overdue and we are using internal expectations excessively without any good Apple secrecy reason. I don’t agree with your assertion that moving to open source is the right time to verify which ones got fixed throughout the development cycle. That may have been practice before but I don’t think it’s a good system.

I think that there are important best practices that outweigh other considerations. - Do not reduce test coverage when moving or refactoring expectations. - When moving expectations, verify them, and remove obsolete ones. There is no better time to do this, and deferring to someone doing it later has never worked out in WebKit project history. - Possibly should go without saying, but technical accuracy like not adding expectations for files in fully skipped directories is something that is easy to get wrong in mass moves. There are several reasons why expectations can be in internal: 1. Functionality that only works in internal builds, like touch events. I can see these expectations potentially being in open source, but we need to develop new mechanisms for this. 2. Tests that behave in different ways on configurations that we only test on internal bots, like ASan, GuardMalloc or StressGC. It is bad that we hack TestExpectations for these by weakening expectations on all internal bots. But at least we can have strong [ Pass ] expectations in open source, and weakening them just because there is an internal configuration where the test works less reliably is strictly worse. At some point, we should develop a way to annotate these accurately, but until then, it's better to have clean strict open source expectations, and a mess in internal than to have a mess everywhere. 3. Test failures that occur on future OS versions. We want to move these to open source at some point of course, and generally before shipping. There are a lot of benefits to keeping these internal besides secrecy though, as OS regressions are by definition outside WebKit. We can be a lot more expressive about these in internal expectations. Having a move event close to the end of development is a great forcing function to clean up things that got fixed. 4. iOS device results. It is not possible to run tests on a customer OS, and may never be. I'm not sure why open source LayoutTests/platform/ios-device was created. While there is nothing secret, I think that it can only complicate things. Either way, these results aren't being maintained, so it doesn't really matter where they are. > I’m not sure I understand the mechanism that prevented these expectations > from being used when running tests on iOS 13. Where is the code that > implements that rule? Open source testers are on iOS 13 and don't see these internal expectations. Internal testers may have less than ideal expectations (I think that this differs case by case), but reducing strictness of open source expectations is unnecessary and strictly worse for test coverage. > I assumed many of these were wrong and your review highlights that. But this > patch does not introduce any of those mistakes unless it’s incorrectly > mixing iOS 14 with 13. I propose to migrate these so they are not hard to > see for open source contributors and alongside the other expectations. Seeing that I had comments about every single iOS expectations change in this patch, I assume that you are not proposing to land it as is? > It seems to me that moving these to open source and correcting the many > mistakes are not the same task. I’d love to have many of these mistakes > corrected either before or after migrating to open source. Correcting these mistakes after migrating is much harder, as there isn't git/svn blame any more, and everything becomes a mystery. Many of the comments that I made here were possible because the patch hasn't landed yet, and I could easily see what you've been moving from. Additionally, there is no forcing function to make things better later. Landing accurate expectations for things that are moved is a well defined task that we've been dealing with, mostly successfully. Cleaning things up afterwards is something that the project has never succeeded at, so I think that to change the processes as you propose, we need a better case than a claim that we could do such cleanup. > I believe that migration is overdue and we are using internal expectations > excessively without any good Apple secrecy reason. I still posit that thinking about this as a single migration confuses the discussion. The majority of iOS 14 test results were moved in r264950, and that was a good time to do it. The things that remain internal are there for different reasons, and generally either need to stay there, or would take a larger change to be appropriate in open source. > I don’t agree with your assertion that moving to open source is the right > time to verify which ones got fixed throughout the development cycle. That > may have been practice before but I don’t think it’s a good system. I still think that mechanically moving expectations is worse in more important ways. There is a lot to improve, but reducing test coverage is a problem that outweighs other considerations for me.

I’ll let you take care of this Alexey; I don't think I'm qualified to do it the way you suggest.