FYI I just reverted the CL, and will take necessary actions once we reach consensus.
Nate, yes if you plan to land your change in chromium, the presubmit check will prevent the CL to land if the json file is not up to date. You should be good now as the CL is reverted. Thanks, Weizhong On Tue, Aug 17, 2021 at 11:48 AM Nate Chapin <[email protected]> wrote: > I hit this on > https://chromium-review.googlesource.com/c/chromium/src/+/3098588 > > The only test change is in external/wpt, and it's a presubmit error which > will block the CQ, so I think as it stands currently, you can't ignore the > warning even for external/wpt/. > > On Tue, Aug 17, 2021 at 11:08 AM 'Weizhong Xia' via blink-dev < > [email protected]> wrote: > >> Running collect_web_tests on external/wpt is kind of slow, because we >> need to generate wpt manifest first. Running that on other folders will be >> fast. And I will take a look to see if there is room for improvement. >> >> One thing worth mentioning is that if the change in external/wpt is meant >> to be exported to upstream <https://github.com/web-platform-tests/wpt>, >> you can ignore the presubmit check warning. (Should have mentioned this in >> the PSA). >> >> On Tue, Aug 17, 2021 at 11:01 AM Dirk Pranke <[email protected]> wrote: >> >>> On Tue, Aug 17, 2021 at 10:48 AM Ian Kilpatrick < >>> [email protected]> wrote: >>> >>>> So I was a little surprised by this last night. >>>> >>> >>> Yeah, we probably should've had the discussion first, sorry :(. >>> >>> >>>> >>>> 1. This was relatively slow on my macbook, running locally takes ~40s. >>>> From just now: >>>> >>>> ikilpatrick-macbookpro:src ikilpatrick$ touch >>>> third_party/blink/web_tests/external/wpt/css/css-tables/test.html >>>> >>>> ikilpatrick-macbookpro:src ikilpatrick$ time >>>> third_party/blink/tools/collect_web_tests.py external/wpt >>>> >>>> >>>> real 0m40.492s >>>> >>>> user 0m23.007s >>>> >>>> sys 0m41.715s >>>> >>>> 2. I had a bunch of uncommitted tests in various wpt directories that I >>>> needed to move out to somewhere else (rather than the script just adding >>>> committed tests). >>>> >>> >>> One other concern is that we might end up with a lot of merge conflicts >>> in a single file, and so we might want to shard by the top level >>> directories or something ... >>> >>> (I'd also note that uploading patches is already relatively slow, do we >>>> have metrics for how long developers are waiting for upload scripts to run? >>>> - I suspect this is due to WPT presubmit scripts but unsure) >>>> >>> >>> The upload checks are just generally crazy slow these days. I'm not sure >>> if we have collected metrics for them, but things are an order of magnitude >>> slower than I'd like them to be. >>> >>> But that's partially a separate topic (apart from this check, of course). >>> >>> -- Dirk >>> >>> >>>> Ian >>>> >>>> >>>> >>>> >>>> On Tue, Aug 17, 2021 at 10:20 AM 'Dirk Pranke' via blink-dev < >>>> [email protected]> wrote: >>>> >>>>> >>>>> >>>>> On Tue, Aug 17, 2021 at 10:08 AM 'Weizhong Xia' via blink-dev < >>>>> [email protected]> wrote: >>>>> >>>>>> tl;dr: We introduced a mechanism that pre-collects test cases to >>>>>> //third_party/blink/web_tests/AllTestsByDirectories.json, and uses a >>>>>> presubmit check to ensure this file is up to date. You can stop here if >>>>>> you >>>>>> don't write web test cases. >>>>>> >>>>>> Dear blink-devs, >>>>>> >>>>>> You may have noticed that when you try to upload a CL, you are >>>>>> prompted to update AllTestsByDirectories.json if your CL changes web test >>>>>> cases. Yes, today we landed a CL >>>>>> <https://chromium-review.googlesource.com/c/chromium/src/+/3067372> >>>>>> that pre-collects test cases and saves that to the json file. The reason >>>>>> to >>>>>> do this is that rwt tries to collect tests on every swarming bots, and >>>>>> that >>>>>> takes about 30 seconds (the best case when running on SSD). This is not a >>>>>> small amount of time as we are targeting a 10-min CQ. After this CL >>>>>> lands, >>>>>> collecting tests now takes less than 2 seconds for all platforms. >>>>>> >>>>>> Impact to you: if your CL adds/deletes/renames web test cases, you >>>>>> will be prompted. Follow the instructions there should be enough. But if >>>>>> you still have an issue, (even though we have tested many different >>>>>> scenarios we can think about), feel free to ping/email me. >>>>>> >>>>>> There is no change to how you run rwt locally. We added a new switch >>>>>> to tell the swarming bots to load tests from the json file. This switch >>>>>> is >>>>>> turned off by default. Developers usually don't need to turn this on, >>>>>> unless all of your tests are saved to the json file and you are running a >>>>>> full test so want to save about 30 seconds. >>>>>> >>>>> >>>>> FWIW, I think this is an important and probably worthwhile change >>>>> (disclaimer: I did review it and was involved in the design). The number >>>>> of >>>>> tests we have now is substantial and the startup delay is a significant >>>>> cost. We need to be looking into ways to reduce this. >>>>> >>>>> I think it's possible we should turn this on by default and/or do some >>>>> hooking to make things aware of when you've added new tests (e.g., via git >>>>> status) to try and make this a little more seamless, but I also think >>>>> landing it off by default was a good first step. >>>>> >>>>> Some may ask if we can just generate this at compile time. >>>>> Unfortunately, no, we can't, because there's no way to keep it correctly >>>>> up-to-date when you do add tests after having generated the file at least >>>>> once [ we asked this ourselves and actually tried this approach and broke >>>>> deterministic builds :( ]. >>>>> >>>>> I'm happy to hear other thoughts. Perhaps there are other ways we can >>>>> optimize things to be fast enough ... >>>>> >>>>> -- Dirk >>>>> >>>>> >>>>>> >>>>>> Thanks for your time! >>>>>> >>>>>> Cheers, Weizhong >>>>>> >>>>>> -- >>>>>> 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/CADXrSiprRWy_UCC0w2oohbSGGTbeP%3DdkyYnBOx7j8Y-abDSe2A%40mail.gmail.com >>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSiprRWy_UCC0w2oohbSGGTbeP%3DdkyYnBOx7j8Y-abDSe2A%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/CAEoffTAc389E6wteVXUAuKX8ZBFWBa2UEMwAbd9oCxJ9pxNCKQ%40mail.gmail.com >>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTAc389E6wteVXUAuKX8ZBFWBa2UEMwAbd9oCxJ9pxNCKQ%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/CADXrSipBCHvjwf-AA1DGHUVjVzKQka5bBvTBnSRx5sEUpnJN_Q%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSipBCHvjwf-AA1DGHUVjVzKQka5bBvTBnSRx5sEUpnJN_Q%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/CADXrSip-icEpE5X4kC0O95VLOCHGXc2tjBErqvhHZ%3D7LHK%2BY%3DQ%40mail.gmail.com.
