PSA: Introduce collect_web_tests.py for web test

[Weizhong Xia]

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 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.

Thanks for your time!

Cheers, Weizhong

[Dirk Pranke]

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 blink-dev+...@chromium.org.
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.

[Ian Kilpatrick]

So I was a little surprised by this last night.

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).

(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)

Ian

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTAc389E6wteVXUAuKX8ZBFWBa2UEMwAbd9oCxJ9pxNCKQ%40mail.gmail.com.

[Ken Rockot]

Would it be a bad idea to add build rules covering all web tests, and manually maintain lists of test sources like we do for all other source files in the tree?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJL3UpS64fRvbJLf%2B3uFA_6UDgXoLWgUFGZSN7mAsVFOrP8syA%40mail.gmail.com.

[Dirk Pranke]

On Tue, Aug 17, 2021 at 10:48 AM Ian Kilpatrick <ikilp...@chromium.org> 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

[Dirk Pranke]

Unfortunately, there's > 175K files under //third_party/blink/web_tests, which would increase the size of the build files *a lot* and I have no idea what that would do to GN and Ninja run times ... But, it is theoretically possible to do so.

-- Dirk

[Weizhong Xia]

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, you can ignore the presubmit check warning. (Should have mentioned this in the PSA).

[Domenic Denicola]

On Tue, Aug 17, 2021 at 2:08 PM 'Weizhong Xia' via blink-dev <blin...@chromium.org> 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, 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...

 

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.

[Xianzhu Wang]

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.

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.

[Nate Chapin]

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/.

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.

[Weizhong Xia]

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

[Dirk Pranke]

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

[Ken Rockot]

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?

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.

[Dirk Pranke]

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

[Ken Rockot]

On Tue, Aug 17, 2021 at 1:31 PM Dirk Pranke <dpr...@google.com> wrote:

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?

I would expect it to ~eliminate the running cost of collecting tests from the filesystem, while also avoiding the introduction of yet another generated file to be checked in (probably with lots of contention).

It's work that can be done at build time, but only if we have sufficient build metadata.

[Dirk Pranke]

On Tue, Aug 17, 2021 at 1:46 PM Ken Rockot <roc...@chromium.org> wrote:

On Tue, Aug 17, 2021 at 1:31 PM Dirk Pranke <dpr...@google.com> wrote:

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?

I would expect it to ~eliminate the running cost of collecting tests from the filesystem, while also avoiding the introduction of yet another generated file to be checked in (probably with lots of contention).

Sure.

 

It's work that can be done at build time, but only if we have sufficient build metadata.

Yeah, if we're willing to make "every test must be listed in GN" a requirement that's certainly doable. We don't necessarily have to list every *file* (i.e., every baseline, import, etc.) though there may be advantages to doing so (there are clearly also disadvantages to doing so).

It's unclear to me whether doing that would be worth the cost in increased GN runtime, but we could look into it.

-- Dirk