[web] Build unit tests with dart2js instead of build_runner #20390
Conversation
nturgut
force-pushed
the
unit_tests_redesign
branch
from
4781ac0 to
a124ec8
Compare
…ds for 8 core MacBook
nturgut
force-pushed
the
unit_tests_redesign
branch
from
8693597 to
4fad778
Compare
| TestTypesRequested testTypesRequested = null; | ||
|
|
||
| /// How many dart2js build tasks are running at the same time. | ||
| final Pool _pool = Pool(8); |
There was a problem hiding this comment.
We should use BUILD_MAX_WORKERS_PER_TASK as the pool size. Also, currently we default to 4. I recall you and @mdebbar preferring 4 as the default back when I introduced BUILD_MAX_WORKERS_PER_TASK.
There was a problem hiding this comment.
"We should use BUILD_MAX_WORKERS_PER_TASK as the pool size" -> Why? Can you add a link if you made a prior analysis.
Also Why @mdebbar prefers 4?
The highest priority now for Flutter team is to make tree green. One thing we should do is to drop run time for LUCI tasks. I confirmed that all LUCI bots have at least 8 cores. I am ok with an argument for making this higher, I don't see it is 4?
There was a problem hiding this comment.
I believe it was just a conversation over VC. The only documented part was this: #17616 (comment)
There was a problem hiding this comment.
"We should use BUILD_MAX_WORKERS_PER_TASK as the pool size" -> Why? Can you add a link if you made a prior analysis.
Because I am using 32 as the pool size, and it speed up my builds considerably. I added this capability in #17616. Currently, this change removes the capability to configure the pool size.
There was a problem hiding this comment.
I see, the argument was not every machine has 16 cores. Right now all LUCI machines have at least 8 cores so we can use 8.
There was a problem hiding this comment.
I believe the argument for 4 cores was in favor of contributors using laptops. A lot of laptops have only 4 cores (although typically they are hyper-threaded, so 8 might still be OK).
| TestTypesRequested testTypesRequested = null; | ||
|
|
||
| /// How many dart2js build tasks are running at the same time. | ||
| final Pool _pool = Pool(8); |
There was a problem hiding this comment.
I believe it was just a conversation over VC. The only documented part was this: #17616 (comment)
| } | ||
|
|
||
| // Currently iOS Safari tests are running on simulator, which does not | ||
| // support canvaskit backend. |
There was a problem hiding this comment.
Did you mean to put this comment here? I'm having trouble connecting it to the if block below. There's nothing in the code related to iOS, Safari, or simulators.
There was a problem hiding this comment.
Yes, if anyone is reading the code, hoping to understand how a test runs, I'm telling them "be aware, these lines won't run on iOS Safari"
This is also in the skips in the individual tests but I think it's good to leave these stories to the readers.
|
After this change my local builds sped up drastically from 2.5 minutes down to 49 seconds! |
3 participants
build_filter functionality was giving an error for windows for while therefore in windows tests were not building.
This PR changes test_runner.dart to build unit tests with dart2js instead of build_runner.
It uses pool to run builds in parallel. Time for tests for 8 core Macbook:
Fixes: flutter/flutter#62361 After this fix the Windows developers will be able to run
felt_windows.bat testagain. In order to make this step run also adding other small changes to the felt tool.