I'm not sure I understand this logic. -expected.txt files are just more-convenient, more in-your-face versions of TestExpectations files. Surely you're not suggesting we get rid of TestExpectations files?
On Mon, Jun 6, 2022 at 6:30 PM Xianzhu Wang <[email protected]> wrote: > I think we first need to answer a question: Why do we need *-expected.txt > for WPT tests? > > Upstream WPT doesn't have *-expected.txt. *-expected.txt is a > blink-specific thing to allow some failing WPT tests to pass on blink with > temporarily allowed failures. If a test has -expected.txt, it means Chrome > behaves differently than the standard web platform behavior (or the test > itself is wrong). The files are sometimes harmful because they hide > failures and web platform incompatibilities (e.g. > <http://crbug.com/772405>). So I think -expected.txt files for WPT tests > should be rare and eventually be removed. An -expected.txt should not be > treated as a part of the test itself because a) it doesn't exist in > upstream WPT and 2) it doesn't describe the standard expected behavior, > thus isn't necessarily placed besides the test. > > The directory name 'platform' may be misleading. 'baselines' may be a > better name (but we should not rename until we decide what to do for > generic baselines). For WPT tests, 'failures' is perhaps an even better > name. > > On Friday, June 3, 2022 at 6:55:49 AM UTC-7 Dominic Farolino wrote: > >> For the long term, we can definitely move this back once we have separate >>> legacy tests and wpt into different folders. I can also reach out to you >>> guys to better understand your needs. >>> >>> For the folders Dom mentioned, I can check to see if any of that can be >>> removed. I understand the README is needed for virtual test suites. >>> >> >> Can you define "long term" here? Is there a timeline? I was really hoping >> that we'd go back to how things were almost immediately. Still the need for >> the delay does not quite make sense to me, and I'm really hoping that for >> Blink developer experience we can revert this back to the previous setup >> ASAP. >> >> Under "web_tests/virtual" we have baselines for wpt and pure virtual >>> legacy tests, we can list the folders under "virtual/prefix" for each >>> virtual test suite, which will make the dependency very large(yes), and we >>> need make such change each time when we add new virtual suites. I don't >>> think this is something blink devs want to do. >>> >> >> I'm sorry, but like Domenic I am not sure what to make of much of this. >> What I do know is that Blink developers also don't want the current WPT >> writing experience where expectation files are positioned far away from the >> source test. I think this experience matters a lot. >> >> On Thu, Jun 2, 2022 at 4:25 PM Weizhong Xia <[email protected]> wrote: >> >>> Previously you don't need to specify anything in BUILD.gn is because we >>> are downloading the whole "web_tests" folder. Now we want to run legacy >>> tests and wpt in different steps. Under "web_tests/virtual" we have >>> baselines for wpt and pure virtual legacy tests, we can list the folders >>> under "virtual/prefix" for each virtual test suite, which will make the >>> dependency very large(yes), and we need make such change each time when we >>> add new virtual suites. I don't think this is something blink devs want to >>> do. >>> >>> The discussion is at the beginning of the crbug. So pls scroll back to >>> #c1, and read from there. >>> >>> thanks, Weizhong >>> >>> On Thu, Jun 2, 2022 at 12:29 PM Domenic Denicola <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Thu, Jun 2, 2022 at 3:21 PM Weizhong Xia <[email protected]> >>>> wrote: >>>> >>>>> Folks >>>>> >>>>> I'm sorry to see this has caused inconvenience, and sorry for being >>>>> late in response to this, due to the same reason Dom had. >>>>> >>>> >>>> Thanks for responding and listening to our concerns! >>>> >>>> >>>>> >>>>> The reason to move baselines to one central place at this point is to >>>>> make it possible to specify dependency in BUILD.gn. >>>>> >>>> >>>> I don't quite understand this. I've had to work with WPTs and WPT >>>> expectations my entire time working on Chromium. I've never had to "specify >>>> dependency in BUILD.gn"; I don't really know what that means. I'd love to >>>> hear more (or be referred to a doc explaining the issue), so that I >>>> understand why we're making the sacrifice we're making. (The linked bug >>>> isn't very understandable for me, unfortunately. I can't even understand >>>> enough to find the part you mentioned where you discussed different >>>> approaches.) >>>> >>>> I'm also unsure how the decision was weighed. Is the population of >>>> people specifying dependency in BUILD.gn very large, so that their needs >>>> are outweighing those of the Chromium developers working with web platform >>>> tests? >>>> >>>> >>>>> This is part of the work to use upstream wptrunner to run wpt tests. >>>>> In crbug/1299834 <https://crbug.com/1299834> we have tried to discuss >>>>> some different approaches. I would say this is the least disruptive way. >>>>> For blink engprod, I think to make blink devs happy is our top priority. >>>>> >>>>> For the long term, we can definitely move this back once we have >>>>> separate legacy tests and wpt into different folders. I can also reach out >>>>> to you guys to better understand your needs. >>>>> >>>>> For the folders Dom mentioned, I can check to see if any of that can >>>>> be removed. I understand the README is needed for virtual test suites. >>>>> >>>>> Cheers, Weizhong >>>>> >>>>> On Wednesday, June 1, 2022 at 6:06:56 AM UTC-7 [email protected] >>>>> wrote: >>>>> >>>>>> +1 from me as well. I was similarly caught by surprise by this change >>>>>> (during reviews for webexposed changes), and am similarly not seeing the >>>>>> upside for this. >>>>>> >>>>>> While I'm sure this is a change that was meant to be a positive one, >>>>>> I'd love to better understand the reasoning, and whether the current >>>>>> situation is a temporary one, or one that is planned to be permanent even >>>>>> after the move to blink_wpt_tests is done. >>>>>> >>>>>> On Fri, May 27, 2022 at 6:10 PM Domenic Denicola <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> +1. This was really unpleasantly surprising. When I first saw the >>>>>>> original blink-dev email, I thought "generic baselines" meant something >>>>>>> like "non-web platform test baselines", not "WPT expectation files that >>>>>>> are >>>>>>> platform-agnostic". >>>>>>> >>>>>>> In addition to the context-switching cost, it's just much harder to >>>>>>> navigate between tests and their expectations, which is something I do >>>>>>> quite often. E.g., they are no longer grouped together in code reviews, >>>>>>> since their file paths are lexicographically far away from each other. >>>>>>> And, >>>>>>> as someone maintaining and reviewing several WPT directories, moving >>>>>>> these >>>>>>> crucial files out of the directories I commonly work in (and have >>>>>>> metadata >>>>>>> marking me as the point-of-contact for) into separate directories >>>>>>> dilutes >>>>>>> the cohesiveness of my projects. >>>>>>> >>>>>>> >>>>>>> On Fri, May 27, 2022 at 12:03 PM Dominic Farolino <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> I write a lot of web platform tests as a Web Platform engineer; >>>>>>>> recently I wrote one in external/wpt/ >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/> >>>>>>>> (the external web platform tests directory), and was shocked to find >>>>>>>> the >>>>>>>> eradication of `-expected.txt` files. I placed my expectations file >>>>>>>> next to >>>>>>>> the source file as we've done for many years, and found that my test >>>>>>>> was >>>>>>>> "failing" because the test runner couldn't find my test expectations >>>>>>>> file. >>>>>>>> >>>>>>>> I dug deeper and found https://crrev.com/c/3603221 which was >>>>>>>> responsible for moving more than 21,000 *platform-agnostic* test >>>>>>>> expectations files away from their source files and into >>>>>>>> web_tests/platform/generic >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/> >>>>>>>> directory. I found more discussion in this email thread >>>>>>>> <https://groups.google.com/a/chromium.org/g/blink-dev/c/0WmmgEkqdOo> >>>>>>>> which >>>>>>>> I missed because blink-dev emails do not go directly in my inbox. >>>>>>>> >>>>>>>> I must say I find this change extraordinarily inconvenient as a Web >>>>>>>> Platform engineer, and I want to push back against this. A minority of >>>>>>>> web >>>>>>>> platform tests have platform-specific failures, which justifies the >>>>>>>> need >>>>>>>> for *some* platform-specific test expectations directories, but I >>>>>>>> believe a huge majority have generic baselines that are wildly >>>>>>>> convenient >>>>>>>> to have right next to the actual tests themselves. Putting them in a >>>>>>>> separate directory means I and others have to open a separate browser >>>>>>>> tab >>>>>>>> to view how many expectations there are for a given directory, and >>>>>>>> requires >>>>>>>> a lot of unnecessary context switching. It is particularly confusing >>>>>>>> for >>>>>>>> clusters of tests whose names are all *very* similar and vary by >>>>>>>> only a few numbers or suffixes—this increases the cost of the context >>>>>>>> switching. >>>>>>>> >>>>>>>> Furthermore, it renders tons of directories absolutely useless! All >>>>>>>> ~150 directories in web_tests/virtual >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/> >>>>>>>> (for >>>>>>>> VirtualTestSuites) are just empty directories with README files—these >>>>>>>> used >>>>>>>> to house virtualtest-specific expectations. So now for fenced frames >>>>>>>> (the >>>>>>>> project I'm working on right now), we have the following test >>>>>>>> directories: >>>>>>>> >>>>>>>> - web_tests/wpt_internal/fenced_frame/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/fenced_frame/> >>>>>>>> - web_tests/virtual/fenced-frame-mparch/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/fenced-frame-mparch/> >>>>>>>> - web_tests/virtual/fenced-frame-shadow-dom/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/fenced-frame-shadow-dom/> >>>>>>>> - web_tests/platform/generic/wpt_internal/fenced_frame/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/wpt_internal/fenced_frame/> >>>>>>>> - web_tests/platform/generic/virtual/fenced-frame-mparch/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/virtual/fenced-frame-mparch/> >>>>>>>> - web_tests/platform/generic/virtual/fenced-frame-shadow-dom/ >>>>>>>> >>>>>>>> <https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/virtual/fenced-frame-shadow-dom/> >>>>>>>> - + platform-specific directories, which are relatively rare >>>>>>>> for us >>>>>>>> >>>>>>>> This is so weird! Regardless of whether or not there are plans to >>>>>>>> clean this up, I can't see the upsides. The ousting of >>>>>>>> platform-agnostic >>>>>>>> expectations is only an inconvenience for WP engineers, while there >>>>>>>> might be some test-infra conveniences around BUILD.gn dependencies >>>>>>>> <https://bugs.chromium.org/p/chromium/issues/detail?id=1299834#:~:text=This%20way%20all%20the%20baselines%20will%20be%20in%20platform%20directory%2C%20make%20it%20a%20little%20bit%20easier%20to%20specify%20dependency%20in%20BUILD.gn.> >>>>>>>> (maybe?). >>>>>>>> In any case, I am hard pressed to find justification in this move, and >>>>>>>> would love to see if we can reconsider this. >>>>>>>> >>>>>>>> Thoughts? >>>>>>>> >>>>>>>> Dom >>>>>>>> >>>>>>>> -- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "blink-dev" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to [email protected]. >>>>>>>> To view this discussion on the web visit >>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP-uykAN06y5o-WYznnicvm1YREbSsLbs6dM57LtL4vCWB%3Duzw%40mail.gmail.com >>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP-uykAN06y5o-WYznnicvm1YREbSsLbs6dM57LtL4vCWB%3Duzw%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>> . >>>>>>>> >>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "blink-dev" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to [email protected]. >>>>>>> >>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra9jXeotZVYNKBMmW90x36%2BdOCqcqfZ-ZpPW0qJVUBptbQ%40mail.gmail.com >>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra9jXeotZVYNKBMmW90x36%2BdOCqcqfZ-ZpPW0qJVUBptbQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra_TC219PU3H4Jr7-C6WLb80b3WufVHTxxGG3BB5bdWbTw%40mail.gmail.com.
