[web] separate tests into build + test steps; add run command #27495

yjbanov · 2021-07-16T22:50:31Z

Introduce felt run that runs one or more build steps, identified by name. Build steps can run one after another to complete a larger task. For example, to run tests, one can run a compile_tests step followed by a run_tests step. A build graph can execute steps on different bots and pass build artifacts from bot to bot for higher parallelism and better machine utilisation.

Use felt run to separate web tests into build and test steps:

compile_tests - compiles tests but does not run them. This step is intended to be executed by a dedicated build bot, and its artifacts shared with bots that run the tests.
run_tests_BROWSER - where BROWSER can be chrome, edge, firefox, safari, ios-safari, are steps that run tests (but do not compile them). These steps are intended to be executed by build bots with a particular OS/browser combination.

Build steps are implemented on top of the existing Pipeline and PipelineStep classes. This way steps can be used by felt run as well as by existing commands. For example, this PR implements both felt run and felt test in terms of the same build step classes.

Additional clean-ups/refactorings to facilitate this design:

Remove all "skip" logic from the felt tool (e.g. where we decided based on the current platform/browser whether we should skip golden tests). The lack of a ScreenshotManager already indicates that the current browser doesn't support screenshots, and we already have a way to skip tests from within the test itself; there's no need for yet another way of skipping tests. This removes some complexity from the felt code.
Remove unused command-line options and their corresponding argument parsers: --chrome-version, --safari-version, --edge-version, --firefox-version, --type, --verbose.
Make BrowserLock a more structured class, so we don't have to parse YAML everywhere.
Make property names more consistent in browser_lock.yaml (use underscores).

Fixes flutter/flutter#85274

mdebbar

LGTM with a few suggestions but nothing that blocks the PR.

mdebbar · 2021-07-20T18:33:27Z

lib/web_ui/dev/run.dart

+    if (unrecognizedStepNames.isNotEmpty) {
+      io.stderr.writeln();
+      return false;
+    }


Should we explain what happened here and print the step names that were unrecognized? The user could make a typo in a step name and not understand why the command is failing.

You're too kind. I totally messed this one up :)

Fixing.

mdebbar · 2021-07-20T19:09:49Z

lib/web_ui/dev/steps/compile_tests_step.dart

+  if (htmlTargets.isNotEmpty) {
+    await _compileTestsInParallel(targets: htmlTargets, forCanvasKit: false);
+  }
+
+  if (canvasKitTargets.isNotEmpty) {
+    await _compileTestsInParallel(targets: canvasKitTargets, forCanvasKit: true);
+  }


Suggestion: in case both of these are true at the same time, should we compile them in parallel? Or do they have to be sequential?

Suggested change

if (htmlTargets.isNotEmpty) {

await _compileTestsInParallel(targets: htmlTargets, forCanvasKit: false);

}

if (canvasKitTargets.isNotEmpty) {

await _compileTestsInParallel(targets: canvasKitTargets, forCanvasKit: true);

}

await Future.wait([

if (htmlTargets.isNotEmpty)

_compileTestsInParallel(targets: htmlTargets, forCanvasKit: false),

if (canvasKitTargets.isNotEmpty)

_compileTestsInParallel(targets: canvasKitTargets, forCanvasKit: true),

]);

mdebbar · 2021-07-20T19:11:33Z

lib/web_ui/dev/steps/compile_tests_step.dart

+  await for (final bool isSuccess in results) {
+    if (!isSuccess) {
+      throw ToolExit('Failed to compile tests.');
+    }
+  }


I'm curious why you picked this over Future.wait()?

I just looked it up now. You can pass eagerError: true which might be even better than the await for loop since it awaits all tests in parallel instead of awaiting in sequence. e.g. what if the first test takes a long time to run, and the second test fails quickly? The await for loop waits until the first test is complete, while Future.wait(eagerError: true) stops as soon as the second test fails.

Oh wait. results is a stream, not a list of futures. Ok, my suggestion is probably invalid.

godofredoc

Not for this PR but maybe for a future improvement, it would be great to have the ability to skip and/or run individual tests from a test suite using felt command parameters.

LGTM with some questions and suggestions.

godofredoc · 2021-07-20T21:23:31Z

lib/web_ui/dev/run.dart

+/// Usage:
+///
+///     felt run name_of_build_step
+class RunCommand extends Command<bool> with ArgUtils<bool> {


Should this class be called BuildCommand instead? seems like this class encapsulates the functionality for build steps.

felt build is already taken. It means "build host_debug_unopt". In this case "run" makes sense because it does is run some steps. Those steps may be building, but the may also execute tests, and in the future check licenses, run analysis, and so on.

godofredoc · 2021-07-20T21:30:52Z

lib/web_ui/dev/steps/compile_tests_step.dart

+  required bool forCanvasKit,
+}) async {
+  /// Maximum number of concurrent dart2js processes to use.
+  final Pool pool = Pool(8);


Is it possible to use a constant for this or calculate it based on the number of available CPUs?

Done: made this configurable via FELT_DART2JS_CONCURRENCY environment variable.

godofredoc · 2021-07-20T21:41:10Z

lib/web_ui/dev/steps/run_tests_step.dart

+      await _runTestBatch(
+        testFiles: unitTestFiles,
+        browserEnvironment: browserEnvironment,
+        concurrency: 10,


Can we use a constant for this value or calculate it based on available CPUs?

godofredoc · 2021-07-20T21:49:04Z

lib/web_ui/dev/steps/run_tests_step.dart

+  required BrowserEnvironment browserEnvironment,
+  required bool doUpdateScreenshotGoldens,
+  required int concurrency,
+  required bool expectFailure,


What is the use case for this flag?

We run smoke tests that are expected to fail. This is for detecting when there's something wrong and the test suite isn't running at all.

yjbanov · 2021-08-03T23:22:04Z

@godofredoc

Not for this PR but maybe for a future improvement, it would be great to have the ability to skip and/or run individual tests from a test suite using felt command parameters.

That's already supported, although not via the new flutter run command, but via the flutter test command where you can pick individual targets to run. Skipping tests is supported using the usual skip parameter in the test framework.

…flutter/engine#27495)

…r#27495)

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jul 16, 2021

google-cla bot added the cla: yes label Jul 16, 2021

yjbanov force-pushed the shard-web-tests branch 2 times, most recently from 3f00f4e to cea6ce9 Compare July 19, 2021 02:32

yjbanov marked this pull request as ready for review July 19, 2021 04:19

yjbanov force-pushed the shard-web-tests branch 2 times, most recently from 3f427fc to 0c5f37b Compare July 19, 2021 18:51

yjbanov requested review from godofredoc and mdebbar July 19, 2021 23:43

yjbanov force-pushed the shard-web-tests branch 4 times, most recently from 9a4a434 to d15e57f Compare July 20, 2021 02:11

mdebbar approved these changes Jul 20, 2021

View reviewed changes

godofredoc approved these changes Jul 20, 2021

View reviewed changes

yjbanov force-pushed the shard-web-tests branch 2 times, most recently from 81f5d00 to 33e680f Compare August 3, 2021 20:46

[web] separate tests into build + test steps; add run command

ea8babb

yjbanov force-pushed the shard-web-tests branch from 33e680f to ea8babb Compare August 3, 2021 21:41

yjbanov merged commit ae8071b into flutter:master Aug 3, 2021

engine-flutter-autoroll mentioned this pull request Aug 4, 2021

Roll Engine from a474664cf546 to ae8071be209b (2 revisions) flutter/flutter#87605

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 4, 2021

ae8071b [web] separate tests into build + test steps; add run command (…

bde1ee1

…flutter/engine#27495)

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

[web] separate tests into build + test steps; add run command (flutte…

891e083

…r#27495)

filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022

[web] separate tests into build + test steps; add run command (flutte…

335445f

…r#27495)

[web] separate tests into build + test steps; add run command #27495

[web] separate tests into build + test steps; add run command #27495

Uh oh!

Conversation

yjbanov commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godofredoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yjbanov commented Jul 16, 2021 •

edited

Loading

yjbanov Aug 3, 2021 •

edited

Loading