[web] Preserve correct CanvasKit Variant during test initialization (f…

…lutter#43854) At some point, we started setting `useColorEmoji` to true in our tests, but we were doing it in a way that resets all other configurations to their defaults. This caused the `canvasKitVariant` config to be lost and always set to the default `auto`. This PR fixes the issue and adds tests to: 1. Make sure that the CanvasKit suite always runs with a specific variant (not `auto`). 2. Make sure the given CanvasKit variant makes it all the way through to the tests. The test harness uses a backdoor (a global JS property on `window`) to communicate which canvaskit variant it's using. The test then compares that with `configuration.canvasKitVariant` to make sure they match. If they don't match, then the configuration was lost somewhere on the way. Fixes flutter/flutter#130993
knopp · Jul 20, 2023 · 6c224e8 · 6c224e8
1 parent a3fc185
commit 6c224e8
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 9 deletions.
diff --git a/lib/web_ui/dev/felt_config.dart b/lib/web_ui/dev/felt_config.dart
@@ -202,6 +202,11 @@ class FeltConfig {
       if (runConfig == null) {
         throw AssertionError('Run config not found with name: `$runConfigName` (referenced by test suite: `$name`)');
       }
+      if (bundle.compileConfig.renderer == Renderer.canvaskit && runConfig.variant == null) {
+        throw AssertionError(
+            'Run config `$runConfigName` was used with a CanvasKit test bundle `$testBundleName` '
+            'but did not specify a CanvasKit variant (referenced by test suite: `$name`)');
+      }
       bool canvasKit = false;
       bool canvasKitChromium = false;
       bool skwasm = false;

diff --git a/lib/web_ui/dev/test_platform.dart b/lib/web_ui/dev/test_platform.dart
@@ -540,17 +540,18 @@ class BrowserPlatform extends PlatformPlugin {
 
       final String testRunner = isWasm ? '/test_dart2wasm.js' : 'packages/test/dart.js';
 
-
+      final String canvasKitVariant = getCanvasKitVariant();
       return shelf.Response.ok('''
         <!DOCTYPE html>
         <html>
         <head>
           <title>${htmlEscape.convert(test)} Test</title>
           <meta name="assetBase" content="/">
           <script>
+            window._flutter_canvaskit_variant_for_test_only = "$canvasKitVariant";
             window.flutterConfiguration = {
               canvasKitBaseUrl: "/canvaskit/",
-              canvasKitVariant: "${getCanvasKitVariant()}",
+              canvasKitVariant: "$canvasKitVariant",
             };
           </script>
           $link

diff --git a/lib/web_ui/lib/src/engine/configuration.dart b/lib/web_ui/lib/src/engine/configuration.dart
@@ -121,6 +121,16 @@ class FlutterConfiguration {
     }
   }
 
+  FlutterConfiguration merge(JsFlutterConfiguration newConfig) {
+    final JSAny mergedJsConfig = objectConstructor.assign(
+      <String, Object?>{}.jsify()!,
+      _configuration.jsify(),
+      newConfig.jsify(),
+    );
+    return FlutterConfiguration()
+      ..setUserConfiguration(mergedJsConfig as JsFlutterConfiguration);
+  }
+
   // Static constant parameters.
   //
   // These properties affect tree shaking and therefore cannot be supplied at

diff --git a/lib/web_ui/lib/src/engine/dom.dart b/lib/web_ui/lib/src/engine/dom.dart
@@ -61,8 +61,16 @@ extension JSAnyToObjectExtension on JSAny {
 }
 
 @JS('Object')
-external JSAny get _objectConstructor;
-Object get objectConstructor => _objectConstructor.toObjectShallow;
+external DomObjectConstructor get objectConstructor;
+
+
+@JS()
+@staticInterop
+class DomObjectConstructor {}
+
+extension DomObjectConstructorExtension on DomObjectConstructor {
+  external JSAny assign(JSAny target, JSAny? source1, [JSAny? source2]);
+}
 
 @JS()
 @staticInterop

diff --git a/lib/web_ui/test/canvaskit/configuration_canvaskit_variant_test.dart b/lib/web_ui/test/canvaskit/configuration_canvaskit_variant_test.dart
@@ -0,0 +1,44 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:js_interop';
+
+import 'package:test/bootstrap/browser.dart';
+import 'package:test/test.dart';
+import 'package:ui/src/engine.dart';
+
+import 'common.dart';
+
+@JS('_flutter_canvaskit_variant_for_test_only')
+external String? get _flutterCanvaskitVariantForTestOnly;
+
+void main() {
+  internalBootstrapBrowserTest(() => testMain);
+}
+
+void testMain() {
+  setUpCanvasKitTest();
+
+  // This is to make sure we don't accidentally run the CanvasKit test suite in
+  // auto mode. The CanvasKit variant should always be deterministic.
+  test('CanvasKit tests always run with a specific variant', () {
+    expect(
+      configuration.canvasKitVariant,
+      anyOf(CanvasKitVariant.chromium, CanvasKitVariant.full),
+    );
+    expect(
+      _flutterCanvaskitVariantForTestOnly,
+      anyOf('chromium', 'full'),
+    );
+  });
+
+  // This is to make sure that the variant specified by the test harness is
+  // correctly preserved during tests in the global `configuration` object.
+  test('CanvasKitVariant configuration is preserved in tests', () {
+    expect(
+      configuration.canvasKitVariant,
+      CanvasKitVariant.values.byName(_flutterCanvaskitVariantForTestOnly!),
+    );
+  });
+}
diff --git a/lib/web_ui/test/common/test_initialization.dart b/lib/web_ui/test/common/test_initialization.dart
@@ -22,11 +22,11 @@ void setUpUnitTests({
     }
 
     // Some of our tests rely on color emoji
-    final engine.FlutterConfiguration config = engine.FlutterConfiguration()
-      ..setUserConfiguration(
-        js_util.jsify(<String, Object?>{
-          'useColorEmoji': true,
-        }) as engine.JsFlutterConfiguration);
+    final engine.FlutterConfiguration config = engine.configuration.merge(
+      js_util.jsify(<String, Object?>{
+        'useColorEmoji': true,
+      }) as engine.JsFlutterConfiguration,
+    );
     engine.debugSetConfiguration(config);
 
     debugFontsScope = configureDebugFontsAssetScope(fakeAssetManager);

diff --git a/lib/web_ui/test/engine/configuration_test.dart b/lib/web_ui/test/engine/configuration_test.dart
@@ -33,6 +33,29 @@ void testMain() {
 
       expect(config.canvasKitMaximumSurfaces, 16);
     });
+
+    test('merge', () {
+      final FlutterConfiguration originalConfig = FlutterConfiguration.legacy(
+        js_util.jsify(<String, Object?>{
+          'useColorEmoji': false,
+          'canvasKitMaximumSurfaces': 99,
+        }) as JsFlutterConfiguration,
+      );
+      final FlutterConfiguration mergedConfig = originalConfig.merge(
+        js_util.jsify(<String, Object?>{
+          'useColorEmoji': true,
+        }) as JsFlutterConfiguration,
+      );
+
+      // `useColorEmoji` should've been overriden.
+      expect(mergedConfig.useColorEmoji, isTrue);
+      // `canvasKitMaximumSurfaces` should've been preserved.
+      expect(mergedConfig.canvasKitMaximumSurfaces, 99);
+
+      // Original config should not have been mutated.
+      expect(originalConfig.useColorEmoji, isFalse);
+      expect(originalConfig.canvasKitMaximumSurfaces, 99);
+    });
   });
 
   group('setUserConfiguration', () {

diff --git a/lib/web_ui/test/felt_config.yaml b/lib/web_ui/test/felt_config.yaml
@@ -115,9 +115,11 @@ run-configs:
 
   - name: firefox
     browser: firefox
+    canvaskit-variant: full
 
   - name: safari
     browser: safari
+    canvaskit-variant: full
 
 test-suites:
   - name: chrome-dart2js-html-engine