I'm not entirely sure why you think this would be preferable to do inside GN.
I can see the thinking that says "this is where we list everything else that is needed at runtime" (and that makes sense to me), but are there other reasons? -- Dirk On Tue, Aug 17, 2021 at 12:36 PM Ken Rockot <[email protected]> wrote: > This does sound like a great problem to solve btw, and it's wonderful to > see work being done on it. > > Since we now have this nice script to collect all the tests, could we > try to split it into two pieces? My thinking would be: > > - One piece becomes a run-once-ever script to collect all the tests > from the filesystem and emit a tree of BUILD.gn files with targets to list > test sources (and subdirs as deps) explicitly > - The other piece becomes a build action which aggregates test sources > from aforementioned build targets and spits out this unified JSON file as a > proper build artifact > > I'm sure this can be done with GN, the only question would be as Dirk > points out, whether or not it might explode buildgen time. Even if it does, > maybe that's something we try to solve in GN? > > On Tue, Aug 17, 2021 at 12:00 PM 'Dirk Pranke' via blink-dev < > [email protected]> wrote: > >> I wouldn't particularly want to make this a standard step on the bots, >> because that's just not really how the code is structured. >> >> We could potentially do something as part of the isolate step. The code's >> not really structured for that, either, but that would be less distasteful >> than adding a separate step. >> >> -- Dirk >> >> On Tue, Aug 17, 2021 at 11:47 AM Xianzhu Wang <[email protected]> >> wrote: >> >>> Can we make this a standalone step or run it in the "isolate test" step >>> on the bot? It's like a preparation before sharding blink_web_tests, to >>> reduce the duplicated work on the swarming bots. >>> >>> On Tue, Aug 17, 2021 at 11:17 AM Domenic Denicola <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Tue, Aug 17, 2021 at 2:08 PM '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). >>>>> >>>> >>>> Aren't all changes in external/wpt meant to be exported to upstream? >>>> From what I understand that's the purpose of the directory... >>>> >>>> >>>>> >>>>> 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/CAM0wra9B-aZTjoovP1WGDL80od64CtEL30jM8ch-2Pm_cd9Log%40mail.gmail.com >>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra9B-aZTjoovP1WGDL80od64CtEL30jM8ch-2Pm_cd9Log%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/CAEoffTA0pg5V92k-6gOORv5J%3DOiNVEuDH6VyW2tKTKjdd2uK%2BA%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTA0pg5V92k-6gOORv5J%3DOiNVEuDH6VyW2tKTKjdd2uK%2BA%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/CAEoffTD5cbjibd8Ujg%2BMfjZ7d6Vevw8fK4%2BkzkVReh9rKkeSRw%40mail.gmail.com.
