Web test reorganization #39984

eyebrowsoffire · 2023-03-01T19:30:14Z

This is a major rework of the way we organize, compile, and run unit tests. See lib/web_ui/README.md and lib/web_ui/test/README.md for more details.

lib/web_ui/README.md

mdebbar · 2023-03-02T19:34:36Z

lib/web_ui/README.md

 ```

 ### Optimizing local builds

 Concurrency of various build steps can be configured via environment variables:

- `FELT_DART2JS_CONCURRENCY` specifies the number of concurrent `dart2js`


Are we sure this isn't being used by recipes?

We are sure, because the web_engine recipes will no longer be used at all after this PR, and we'll be using new fresh and shiny engine_v2 builders

mdebbar · 2023-03-02T20:26:21Z

lib/web_ui/dev/felt.dart

@@ -25,7 +24,6 @@ CommandRunner<bool> runner = CommandRunner<bool>(
  ..addCommand(CleanCommand())
  ..addCommand(GenerateFallbackFontDataCommand())
  ..addCommand(LicensesCommand())
-  ..addCommand(RunCommand())


AFAIK, this was being used by recipes. Did you remove it?

Again, the old web_engine recipes are defunct and will no longer be used. Basically all CI is going to be redone and modernized with this change.

lib/web_ui/dev/steps/compile_bundle_step.dart

Co-authored-by: Mouad Debbar <mouad.debbar@gmail.com>

flutter-dashboard · 2023-03-24T03:26:37Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #39984 at sha 6d5fa74

mdebbar · 2023-03-24T16:42:32Z

Looked at the golden diffs. They are tiny diffs of antialiasing/shadows.

The ones I'm a bit worried about are text diffs. They are showing some artifacts:

wasm	non-wasm

These are probably caused by slightly different rounding in wasm mode.

godofredoc · 2023-03-24T20:28:12Z

ci/builders/linux_web_engine.json

-                    ],
-                    "realm": "production"
-                }
+  "builds": [


Content wise the config LGTM, but let's monitor capacity usage. With these many shards a single build will be using 1/4+ of the total capacity.

godofredoc

LGTM for the configs

yjbanov · 2023-03-24T03:51:40Z

lib/web_ui/README.md

+command via the `--local-web-sdk=wasm_release` command.
+
+##### Examples
+Builds all web engine targets, then runs a Flutter app using it:


An example for how to specify targets would be helpful too.

There's an example of that directly below

Cool! I missed it.

yjbanov · 2023-03-24T03:58:20Z

lib/web_ui/dev/felt_config.dart

+  ArtifactDependencies.none()
+    : canvasKit = false
+    , canvasKitChromium = false
+    , skwasm = false;


style nit: in Dart commas go on the previous line

yjbanov · 2023-03-24T04:28:42Z

lib/web_ui/dev/steps/compile_bundle_step.dart

+        if (testFiles != null && !testFiles!.contains(testFile)) {
+          return MapEntry<String, CompileResult>(relativePath, CompileResult.filtered);
+        }
+        final bool success = await compiler.compileTest(testFile);


How do we communicate failures to LUCI? It seems even success == false results in a successful completion of the future.

The compilation actually writes out a json file that contains the result of the compilation of each test. It still will run any of the tests that actually did compile. I did this because it was really irritating debugging dart2wasm stuff and never being able to actually run any tests at all if a single one failed to compile. However, I need to double check that if some tests fail to compile but all the ones that compile succeed that it actually properly fails and LUCI can pick that up. I'm not positive if I actually did that.

It turns out we were immediately failing still, despite my best efforts otherwise 😆 . I went ahead and made a change that allows us to fail compilation and continue, and then bubble up that result at the end.

yjbanov · 2023-03-24T04:30:35Z

lib/web_ui/dev/felt_config.dart

+  final String directory;
+}
+
+class TestBundle {


nit: "bundle" sounds too similar to "set", how about TestCompilation?

yjbanov · 2023-03-24T04:31:05Z

lib/web_ui/dev/felt_config.dart

+  }
+}
+
+class TestSuite {


nit: "suite" is also too close to "set" and "bundle". How about TestRun?

yjbanov · 2023-03-24T23:58:35Z

lib/web_ui/test/felt_config.yaml

@@ -0,0 +1,268 @@
+compile-configs:


Consider documenting at the top of the file common update scenarios, such as adding a new test suite. Or do we expect this to mostly stay as is?

I do expect this to possibly change in the future. I think rather than doing a bunch of comments in the .yaml file, I'll just refer to the README.md in this directory which describes the structure of this file. Do you feel like that readme already has enough info? Or is there something more specific you'd like to see in there? I do have examples for each data type (test set, test bundle, test suite, run configuration, compile configuration, etc)

yjbanov

eyebrowsoffire · 2023-03-27T21:50:27Z

Looked at the golden diffs. They are tiny diffs of antialiasing/shadows.

The ones I'm a bit worried about are text diffs. They are showing some artifacts:

wasm non-wasm

These are probably caused by slightly different rounding in wasm mode.

I went ahead and approved all the goldens. When I looked at the one you posted though in Skia Gold, the left image is actually safari with dart2js, and the right image is Chrome with wasm. So I think its a separate issue with safari rendering. Do you want to file a follow-on for that issue?

flutter-dashboard · 2023-03-27T22:59:29Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #39984 at sha ba83669

flutter-dashboard · 2023-03-27T23:53:49Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #39984 at sha 9b61f3a

ditman

Can't wait to give this a shot!

ditman · 2023-03-27T23:46:34Z

lib/web_ui/README.md

@@ -99,7 +116,7 @@ this option is particularly effective for building on low-powered laptops.
 ### Test browsers

 Chromium, Firefox, and Safari for iOS are version-locked using the
-[browser_lock.yaml][2] configuration file. Safari for macOS is supplied by the
+[browser_lock.yaml][3] configuration file. Safari for macOS is supplied by the
 computer's operating system. Tests can be run in Edge locally, but Edge is not


Is there a way to determine the version of safari that's running in CI? I've tried myself and the journey is pretty cryptic :P

I honestly don't know.

flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 1, 2023

eyebrowsoffire added 14 commits March 2, 2023 09:34

Wrote felt_config file and parsing.

c13cd89

Some suite filtering and copy artifacts step refactor.

b60b1b4

CompileBundleStep implemented.

8760d77

Suites running, but assets alongside tests are not being served.

752547d

Reduce verbosity and fix serving assets from test directory.

1bdd442

All filters except file filters are implemented.

b6e520e

File filter implemented.

e26e6a3

Move around tests themselves.

c2d464d

Test against canvaskit_chromium.

3c9c37d

Added and updated documentation.

a7906e8

Started some CI stuff.

b0cfad9

Add bringup:true to the new web engine step.

78859af

Change felt parameters.

2dd558d

Remove old web engine builders.

35163cf

eyebrowsoffire force-pushed the web_test_reorganization branch from 94c6ea4 to 35163cf Compare March 2, 2023 17:38

mdebbar mentioned this pull request Mar 2, 2023

[web] Remember which variant of CanvasKit is being used #40027

Merged

eyebrowsoffire and others added 7 commits March 2, 2023 13:09

Always hard code canvaskit variants for chrome and edge.

d721f10

Merge branch 'main' into web_test_reorganization

ad81ea6

improve some error messages

c48e279

support filtering by canvaskit variant

83f67b8

Merge branch 'main' into web_test_reorganization

c5f60d1

Change linux_web_engine to new felt commands.

1687708

Skip the legacy build path for license checks.

d6ec4e7

mdebbar reviewed Mar 8, 2023

View reviewed changes

eyebrowsoffire and others added 5 commits March 8, 2023 14:09

Update lib/web_ui/dev/steps/compile_bundle_step.dart

77e0119

Co-authored-by: Mouad Debbar <mouad.debbar@gmail.com>

Revert weird change to README.md

7fcc94b

Merge branch 'main' into web_test_reorganization

8294f13

Merge branch 'main' into web_test_reorganization

581023a

Merge branch 'main' into web_test_reorganization

28808e7

eyebrowsoffire added 6 commits March 23, 2023 18:47

Get rid of extra array layer.

d25ca9e

No need for empty tests array in build steps

aed3f1b

Merge branch 'main' into web_test_reorganization

f47bfd3

It's script for tests, but scripts for generators.

2271295

Mac, not MacOS

d4a7f5a

Merge branch 'main' into web_test_reorganization

6d5fa74

eyebrowsoffire marked this pull request as ready for review March 24, 2023 03:07

flutter-dashboard bot added the will affect goldens label Mar 24, 2023

eyebrowsoffire requested a review from godofredoc March 24, 2023 16:41

eyebrowsoffire changed the title ~~Web test reorganization (WIP)~~ Web test reorganization Mar 24, 2023

godofredoc reviewed Mar 24, 2023

View reviewed changes

yjbanov reviewed Mar 24, 2023

View reviewed changes

yjbanov approved these changes Mar 25, 2023

View reviewed changes

eyebrowsoffire added 2 commits March 27, 2023 14:27

Yegor's comments.

4ed8d43

Merge branch 'main' into web_test_reorganization

d66c188

eyebrowsoffire force-pushed the web_test_reorganization branch from 83c7431 to d66c188 Compare March 27, 2023 21:27

eyebrowsoffire mentioned this pull request Mar 27, 2023

Deprecate --use-local-canvaskit flag #40680

Closed

8 tasks

Add back screenshot logging under verbose flag.

ba83669

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2023

Merge branch 'main' into web_test_reorganization

9b61f3a

ditman approved these changes Mar 27, 2023

View reviewed changes

auto-submit bot merged commit bcd6d3c into flutter:main Mar 28, 2023

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter Engine from 86e406698055 to 83fa712e2ee8 (2 revisions) flutter/flutter#123573

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2023

bcd6d3c96 Web test reorganization (flutter/engine#39984)

c3b91a2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web test reorganization #39984

Web test reorganization #39984

eyebrowsoffire commented Mar 1, 2023 •

edited

Loading

mdebbar Mar 2, 2023

eyebrowsoffire Mar 8, 2023

mdebbar Mar 2, 2023

eyebrowsoffire Mar 8, 2023

flutter-dashboard bot commented Mar 24, 2023

mdebbar commented Mar 24, 2023 •

edited

Loading

godofredoc Mar 24, 2023

godofredoc left a comment

yjbanov Mar 24, 2023

eyebrowsoffire Mar 25, 2023

yjbanov Mar 25, 2023

yjbanov Mar 24, 2023

yjbanov Mar 24, 2023

eyebrowsoffire Mar 25, 2023

eyebrowsoffire Mar 27, 2023

yjbanov Mar 24, 2023

yjbanov Mar 24, 2023

yjbanov Mar 24, 2023

eyebrowsoffire Mar 25, 2023

yjbanov left a comment

eyebrowsoffire commented Mar 27, 2023

flutter-dashboard bot commented Mar 27, 2023

flutter-dashboard bot commented Mar 27, 2023

ditman left a comment

ditman Mar 27, 2023

eyebrowsoffire Mar 28, 2023

Web test reorganization #39984

Web test reorganization #39984

Conversation

eyebrowsoffire commented Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Mar 24, 2023

mdebbar commented Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

godofredoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

eyebrowsoffire commented Mar 27, 2023

flutter-dashboard bot commented Mar 27, 2023

flutter-dashboard bot commented Mar 27, 2023

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyebrowsoffire commented Mar 1, 2023 •

edited

Loading

mdebbar commented Mar 24, 2023 •

edited

Loading