[web] enable CanvasKit tests using a local bundle fetched from CIPD #92134

yjbanov · 2021-10-19T21:57:51Z

Fetch CanvasKit from CIPD to the Flutter SDK cache.
Copy CanvasKit files to build/web/canvaskit during flutter build web.
Use the local CanvasKit copy (instead of unpkg.com) when running unit-tests.
Add test shards that run currently passing tests in CanvasKit mode (which is the vast majority).
Golden tests use the ("WebRenderer": "canvaskit") key/value pair to distinguish the goldens from the HTML ones.

christopherfujino · 2021-10-20T20:09:00Z

dev/bots/test.dart

@@ -1430,8 +1497,7 @@ Future<void> _runFlutterWebTest(String workingDirectory, List<String> tests) asy
        '--concurrency=1',  // do not parallelize on Cirrus, to reduce flakiness
      '-v',
      '--platform=chrome',
-      // TODO(ferhatb): Run web tests with both rendering backends.
-      '--web-renderer=html', // use html backend for web tests.
+      '--web-renderer=$webRenderer', // use html backend for web tests.


nit: update comment

CaseyHillers

.ci.yaml lgtm

CaseyHillers · 2021-10-20T20:14:44Z

.ci.yaml

+          {"dependency": "goldctl"}
+        ]
+      shard: web_canvaskit_tests
+      subshard: "0"


What's the reasoning for these being strings instead of numbers?

Technically all subshard names are strings, but some shards have a custom convention. Web shards use the numeric convention that uses subshard number ("0", "1", etc) for all subshards except the last one, which uses "_last" suffix (e.g. 7_last).

Having said that I totally botched the convention and now I fixed it. Unfortunately, because .ci.yaml changes do not take effect immediately it's easy to make mistakes like that..

christopherfujino · 2021-10-20T20:16:53Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

@@ -365,6 +389,37 @@ class FlutterWebPlatform extends PlatformPlugin {
    }
  }

+  /// Serves a local build of CanvasKit, replacing the CDN build, which can


christopherfujino · 2021-10-20T20:27:40Z

packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart

@@ -637,7 +637,7 @@ void main() {
    environment.outputDir.childDirectory('a').childFile('a.txt')
      ..createSync(recursive: true)
      ..writeAsStringSync('A');
-    await const WebServiceWorker().build(environment);
+    await WebServiceWorker(globals.fs).build(environment);


Avoid adding new calls to globals in tests. I suggest doing something like:

for (final FileSystemStyle style in <FileSystemStyle>[FileSystemStyle.posix, FileSystemStyle.windows]) { test('WebServiceWorker generates a service_worker for a web resource folder for ${style == FileSystemStyle.posix ? 'posix' : 'windows'}', () { final FileSystem fileSystem = MemoryFileSystem(style: style); ... }); }

After an offline discussion, reverted to globals.fs. The test is using TestBed and Environment which are setup to use globals.fs. This file overall needs a separate clean-ups to stop depending on globals.

christopherfujino · 2021-10-20T20:30:41Z

packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart

@@ -637,7 +637,7 @@ void main() {
    environment.outputDir.childDirectory('a').childFile('a.txt')
      ..createSync(recursive: true)
      ..writeAsStringSync('A');
-    await const WebServiceWorker().build(environment);
+    await WebServiceWorker(globals.fs).build(environment);


avoid adding globals to tests

christopherfujino

Some nits about the tests, but otherwise the tools change LGTM

keyonghan

LGTM with test ownership.

Piinks

Should the test shards that are being added show up in CI here? They do not appear to be listed as checks, and without running them, Gold is not seeing this new info.

Piinks

According to https://flutter-gold.skia.org/search?crs=github&issue=92134&patchsets=1&positive=true
these are the only builds that are sending info to Gold:

Windows framework_tests_widgets
Windows framework_tests_libraries
Linux web_tests_5
Linux web_tests_4
Linux web_tests_3
Linux web_tests_2
Mac framework_tests_widgets
Linux web_tests_1
Linux web_tests_7_last
Linux web_tests_6
Linux framework_tests_widgets
Linux web_tests_0
Linux framework_tests_libraries
Mac framework_tests_libraries

yjbanov · 2021-10-20T22:45:15Z

@Piinks good question. Unfortunately, the new shards do not show up right away. More details here, but the tl;dr is that new test shards are added as bringup: true and submitted. They take time to propagate to LUCI and start running without failing the build. Once we confirm that everything is working (perhaps with follow-up fixes), then we can make them build blocking.

yjbanov · 2021-10-20T22:45:52Z

@Piinks good question. Unfortunately, the new shards do not show up right away. More details here, but the tl;dr is that new test shards are added as bringup: true and submitted. They take time to propagate to LUCI and start running without failing the build. Once we confirm that everything is working (perhaps with follow-up fixes), then we can make them build blocking.

yjbanov · 2021-10-20T22:46:45Z

(sorry for duplicate posts; Github is giving me errors when I post but is actually posting my comments)

Piinks · 2021-10-20T23:07:26Z

Ah I see, ok! In that case it sounds like we would validate later that Gold is receiving the data correctly. Would that testing happen in post submit then? Currently the Gold flow checks images in during presubmit. It may cause some issues temporarily if these are run in post submit first, not sure though since we haven't done that before. 🙃

yjbanov · 2021-10-20T23:26:31Z

Ah I see, ok! In that case it sounds like we would validate later that Gold is receiving the data correctly. Would that testing happen in post submit then? Currently the Gold flow checks images in during presubmit. It may cause some issues temporarily if these are run in post submit first, not sure though since we haven't done that before. upside_down_face

Oh no! Catch-22. @CaseyHillers save us!

CaseyHillers · 2021-10-21T04:34:12Z

Oh no! Catch-22. @CaseyHillers save us!

@Piinks if we're not running in presubmit, I don't think there would be a way for gold to block PRs. This should be safe to land. If there's a breakage, that's a bug and we'll need to fix the process to adhere with the tree's flaky test policy.

mdebbar · 2021-10-21T13:13:34Z

dev/bots/test.dart

+Future<void> _runWebHtmlUnitTests() async {
+  await _runWebUnitTests('html');
+}
+
+Future<void> _runWebCanvasKitUnitTests() async {
+  await _runWebUnitTests('canvaskit');
+}


Suggestion: There's no need for async/await here.

Suggested change

Future<void> _runWebHtmlUnitTests() async {

await _runWebUnitTests('html');

}

Future<void> _runWebCanvasKitUnitTests() async {

await _runWebUnitTests('canvaskit');

}

Future<void> _runWebHtmlUnitTests() {

return _runWebUnitTests('html');

}

Future<void> _runWebCanvasKitUnitTests() {

return _runWebUnitTests('canvaskit');

}

mdebbar · 2021-10-21T13:43:27Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

+    final Directory canvasKitDirectory = flutterWebSdk.childDirectory('canvaskit');
+    for (final File file in canvasKitDirectory.listSync(recursive: true).whereType<File>()) {
+      final String relativePath = fileSystem.path.relative(file.path, from: canvasKitDirectory.path);
+      final String targetPath = fileSystem.path.join(environment.outputDir.path, 'canvaskit', relativePath);


Just curious, why not:

final File target = environment.outputDir.childDirectory('canvaskit').childFile(relativePath); await file.copy(target.path);

childFile takes a basename, not a sub-path.

mdebbar · 2021-10-21T13:48:28Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+    // TODO(yjbanov): https://github.com/flutter/flutter/issues/52588
+    //
+    // Update this when we start building CanvasKit from sources. In the
+    // meantime, get the Web SDK directory directory from cache rather than


s/directory directory/directory

Piinks · 2021-10-21T15:43:23Z

@Piinks if we're not running in presubmit, I don't think there would be a way for gold to block PRs. This should be safe to land. If there's a breakage, that's a bug and we'll need to fix the process to adhere with the tree's flaky test policy.

SGTM!

Piinks

LGTM - Gold changes look fine, but will need to confirm and triage all of the images with the new key set once these shards are actually running.

yjbanov · 2021-10-21T18:13:10Z

LGTM - Gold changes look fine, but will need to confirm and triage all of the images with the new key set once these shards are actually running.

Thanks! I might need some handholding to see it through. Might need help from you, @Piinks, and from @CaseyHillers.

…lutter#92134)

abelokon0711 · 2022-03-09T11:10:39Z

Could you explain me please how exactly does this solve #70101?

P-B1101 · 2022-07-02T06:07:25Z

I think this command will use bundled canvaskit from your build folder.
flutter build web --dart-define=FLUTTER_WEB_CANVASKIT_URL=/canvaskit/
I test it using IIS from my localhost and it's worked. I think it will work on any other host too.

google-cla bot added the cla: yes label Oct 19, 2021

yjbanov force-pushed the ck-tests branch 3 times, most recently from e5a82ca to 371987b Compare October 19, 2021 23:49

yjbanov mentioned this pull request Oct 20, 2021

TESTOWNERS instructions unclear #92190

Closed

yjbanov force-pushed the ck-tests branch from 371987b to 786e83a Compare October 20, 2021 18:38

yjbanov marked this pull request as ready for review October 20, 2021 19:52

yjbanov requested review from CaseyHillers, keyonghan and Piinks as code owners October 20, 2021 19:52

yjbanov requested review from harryterkelsen and christopherfujino October 20, 2021 19:52

christopherfujino reviewed Oct 20, 2021

View reviewed changes

CaseyHillers approved these changes Oct 20, 2021

View reviewed changes

christopherfujino reviewed Oct 20, 2021

View reviewed changes

christopherfujino approved these changes Oct 20, 2021

View reviewed changes

keyonghan approved these changes Oct 20, 2021

View reviewed changes

Piinks reviewed Oct 20, 2021

View reviewed changes

yjbanov force-pushed the ck-tests branch from 786e83a to 60d8776 Compare October 21, 2021 00:01

mdebbar approved these changes Oct 21, 2021

View reviewed changes

[web] enable CanvasKit tests using a local bundle fetched from CIPD

098965c

yjbanov force-pushed the ck-tests branch from 60d8776 to 098965c Compare October 21, 2021 17:18

yjbanov requested a review from Piinks October 21, 2021 17:31

Piinks approved these changes Oct 21, 2021

View reviewed changes

yjbanov merged commit ad7727a into flutter:master Oct 21, 2021

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021

[web] enable CanvasKit tests using a local bundle fetched from CIPD (f…

a2ce9df

…lutter#92134)

TahaTesser mentioned this pull request Jan 12, 2022

[web] flutter build web --web-renderer html also generates a canvaskit folder #96509

Closed

jonas-martinez mentioned this pull request Mar 28, 2022

Add the canvas file to Devtools on compile lenra-io/devtool#51

Closed

rob-mur mentioned this pull request Jan 19, 2023

Prefer require-corp over credentialless for Cross-Origin-Embedder-Policy fzyzcjy/flutter_rust_bridge#985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[web] enable CanvasKit tests using a local bundle fetched from CIPD #92134

[web] enable CanvasKit tests using a local bundle fetched from CIPD #92134

yjbanov commented Oct 19, 2021 •

edited

Loading

christopherfujino Oct 20, 2021

yjbanov Oct 20, 2021

CaseyHillers left a comment

CaseyHillers Oct 20, 2021

yjbanov Oct 20, 2021

christopherfujino Oct 20, 2021

christopherfujino Oct 20, 2021 •

edited

Loading

yjbanov Oct 20, 2021

yjbanov Oct 21, 2021

christopherfujino Oct 20, 2021

yjbanov Oct 20, 2021

christopherfujino left a comment

keyonghan left a comment

Piinks left a comment

Piinks left a comment

yjbanov commented Oct 20, 2021

yjbanov commented Oct 20, 2021

yjbanov commented Oct 20, 2021

Piinks commented Oct 20, 2021

yjbanov commented Oct 20, 2021

CaseyHillers commented Oct 21, 2021

mdebbar Oct 21, 2021

yjbanov Oct 21, 2021

mdebbar Oct 21, 2021

yjbanov Oct 21, 2021

mdebbar Oct 21, 2021

yjbanov Oct 21, 2021

Piinks commented Oct 21, 2021

Piinks left a comment

yjbanov commented Oct 21, 2021

abelokon0711 commented Mar 9, 2022

P-B1101 commented Jul 2, 2022

[web] enable CanvasKit tests using a local bundle fetched from CIPD #92134

[web] enable CanvasKit tests using a local bundle fetched from CIPD #92134

Conversation

yjbanov commented Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyHillers 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

christopherfujino Oct 20, 2021 • 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

Choose a reason for hiding this comment

christopherfujino left a comment

Choose a reason for hiding this comment

keyonghan left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

yjbanov commented Oct 20, 2021

yjbanov commented Oct 20, 2021

yjbanov commented Oct 20, 2021

Piinks commented Oct 20, 2021

yjbanov commented Oct 20, 2021

CaseyHillers commented Oct 21, 2021

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

Piinks commented Oct 21, 2021

Piinks left a comment

Choose a reason for hiding this comment

yjbanov commented Oct 21, 2021

abelokon0711 commented Mar 9, 2022

P-B1101 commented Jul 2, 2022

yjbanov commented Oct 19, 2021 •

edited

Loading

christopherfujino Oct 20, 2021 •

edited

Loading