Bug 222964

Created attachment 422945 [details] Patch

Created attachment 423030 [details] Patch

Created attachment 423066 [details] Patch

Created attachment 423126 [details] Patch

Created attachment 423131 [details] Patch

<rdar://problem/75468562>

Created attachment 423374 [details] Patch

There were a couple of questions about tests that seemed deleted, like: webgl/2.0.0/conformance/canvas/render-after-resize-test.html webgl/2.0.0/conformance/context/constants-and-properties.html I don't think these were removed from the upstream conformance snapshot; rather, during development of WebKit's WebGL 2.0 implementation, these tests were copied from the top-of-tree conformance suite into WebKit's snapshot, which was versioned "2.0.0". Newly marked failures of tests like expando-loss.html are concerning. Do you know why those are happening? Have the tests been expanded upstream, for example? Mega-patches like this are really hard to review. If you can provide some assurances that you understand all of the changes in test passes/failures, and/or file bugs about each of the new failures, that would make it easier to approve.

Created attachment 423475 [details] Patch

(In reply to Kenneth Russell from comment #8) > There were a couple of questions about tests that seemed deleted, like: > > webgl/2.0.0/conformance/canvas/render-after-resize-test.html > webgl/2.0.0/conformance/context/constants-and-properties.html > > I don't think these were removed from the upstream conformance > snapshot; rather, during development of WebKit's WebGL 2.0 > implementation, these tests were copied from the top-of-tree > conformance suite into WebKit's snapshot, which was versioned "2.0.0". Changed wording in ChangeLog. > Newly marked failures of tests like expando-loss.html are concerning. > Do you know why those are happening? Have the tests been expanded > upstream, for example? The test was not run in WebGL2, it was run in WebGL 1. Maybe I broke it not knowning the tests don't test what they're supposed to test? Maybe it worked before (works with release Safari). Other reasons for difference in other cases: - The test is skipped errorneously - The test is missing - The test is run in WebGL1 > Mega-patches like this are really hard to review. Yeah.. I don't know how else to do this, though. > If you can provide > some assurances that you understand all of the changes in test > passes/failures, and/or file bugs about each of the new failures, that > would make it easier to approve. Filed the deps of weblg2conformance , referred to these in TestExpectations.

(In reply to Kimmo Kinnunen from comment #10) > (In reply to Kenneth Russell from comment #8) > > Mega-patches like this are really hard to review. > > Yeah.. I don't know how else to do this, though. Some ideas (without looking closely at the mega-patch): - Only post code changes (plus a description of test changes). - Figure out how to remove all the "removed file" patches and just provide a list of removed files.

Understood Kimmo that these large updates are difficult. Good suggestions David. Kimmo, please ping for explicit review if the EWS runs are clean, since I don't think emails are sent if they all pass, only if some fail. Thanks.

(In reply to Kenneth Russell from comment #12) > Understood Kimmo that these large updates are difficult. Good suggestions > David. > > Kimmo, please ping for explicit review if the EWS runs are clean, since I > don't think emails are sent if they all pass, only if some fail. Thanks. Yeah, no.. The idea was to review this and then get it landed out-of-band. The infrastructure fails in various places, bots are of no help. Maybe I then spend even some more time to get the files in place in some other way..

(In reply to David Kilzer (:ddkilzer) from comment #11) > (In reply to Kimmo Kinnunen from comment #10) > > (In reply to Kenneth Russell from comment #8) > > > Mega-patches like this are really hard to review. > > > > Yeah.. I don't know how else to do this, though. > > Some ideas (without looking closely at the mega-patch): > > - Only post code changes (plus a description of test changes). I don't understand this. Depending on philosophical viewpoint, I think the options are: a) all the changes are code changes (barring few images and videos and such) b) no changes are code changes (the changes are reviewed in other place and this is just a data update) In this update, I don't try to do any code change of my unique own invention. > - Figure out how to remove all the "removed file" patches and just provide a > list of removed files. Git provides options to prune the deleted content, but it is no use since then the patch cannot apply and as such cannot land after a green EWS. None of these really help the task at hand. The objective is to a) review b) land. The review seems to be equal to "bots == green" (which shouldn't need a reviewer to inspect and then rs, but I digress) So the only objective is to get something that the patch goes through the bots. So the suggestions should not be about how to massage the data, unless that affects the primary problem. I think the tooling in WebKit will break no matter what, and as such there never is going to be a massage that gets this done. I don't think this is progressing. I try to figure out something else.

I also don't understand what the blocker is. Doesn't this just make some tests function as intended, and align expectations with reality? This seems like a clear improvement.

(In reply to Alexey Proskuryakov from comment #15) > I also don't understand what the blocker is. Doesn't this just make some > tests function as intended, and align expectations with reality? This seems > like a clear improvement. It's not clear whether accidental new bugs in this test upgrade might have caused regressions in previously-passing tests. As Kimmo points out, bugs in the way some of the tests are currently run are either causing or masking failures. The concern is that the deltas are too large to examine. When there's a huge influx of new failures all at once, it's impossible to triage them and understand whether they're real, caused by a test or configuration bug, etc.

Is "previsously passing" a meaningful signal when we've been running the tests entirely incorrectly? And there is a separate WebGL 1 test suite that isn't modified here. I see this a lot more like importing a whole new test suite. > When there's a huge influx of new failures all at once, it's impossible to triage them and understand whether they're real, caused by a test or configuration bug, etc. Perhaps the disagreement is not as much about this change as about what will happen next. The change is valuable in that we now have a new test suite that we can defend against regressions, and that seems like an automatic r+. But if there is the intention to work through all the failures, that's even better, and asking to make this process easier is fair. My question is, is there actually anything of this kind that cannot be done after landing just as efficiently? Is any relevant information lost once this is landed?

(In reply to Alexey Proskuryakov from comment #17) > > My question is, is there actually anything of this kind that cannot be done > after landing just as efficiently? Is any relevant information lost once > this is landed? A curated list of tests that were passing in the previous test suite and failing in the new suite would be valuable. Those should plausibly be investigated with the most urgency to understand whether they're real regressions. If there are bugs in how we're running the new test suite and those are the cause of the new failures, we should ideally track those down and fix them while landing the new test suite. I fully trust Kimmo to follow up well on these new test failures, and plan to contribute to burning down the failure list. Another significant concern is that the large number of new failures that are now suppressed will allow significant regressions to the core implementation until more of the tests are passing fully.

Just a note that it looks like there's been a significant recent regression in the video-related conformance tests in https://github.com/KhronosGroup/WebGL . It's been discovered while pulling the latest version of the tests into Chromium: https://chromium-review.googlesource.com/2775887 . We're working on a fix but you might want to hold off refreshing the conformance suite in WebKit until it is, or manually choose an older commit to sync to. It's for reasons like this that massive test suite upgrades which introduce lots of unreviewable new failures are undesirable.

Is it safe to land this yet?

Would be sort of safe, potentially some fixes and skips needed. Landing it pending policy decision on when we want to do it...