Skip to content

Commit df12fff

Browse files
ditmanditman
authored
[web] Preserve canvaskit variant during tests. (flutter#43868)
A ~~simpler~~ very similar version of flutter#43854 * Makes it harder for users to accidentally remove default configuration values, while still allowing them to do so if needed (configuration is now overridden with a subset of values, rather than passing a full configuration object). * Moves `merge` from the configuration object and into the override method. * Removes a test-only configuration option: * `window._flutter_canvaskit_variant_for_test_only` [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
1 parent c3c898d commit df12fff

7 files changed

Lines changed: 56 additions & 74 deletions

File tree

‎lib/web_ui/dev/test_platform.dart‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -540,18 +540,18 @@ class BrowserPlatform extends PlatformPlugin {
540540

541541
final String testRunner = isWasm ? '/test_dart2wasm.js' : 'packages/test/dart.js';
542542

543-
final String canvasKitVariant = getCanvasKitVariant();
544543
return shelf.Response.ok('''
545544
<!DOCTYPE html>
546545
<html>
547546
<head>
548547
<title>${htmlEscape.convert(test)} Test</title>
549548
<meta name="assetBase" content="/">
550549
<script>
551-
window._flutter_canvaskit_variant_for_test_only = "$canvasKitVariant";
552550
window.flutterConfiguration = {
553551
canvasKitBaseUrl: "/canvaskit/",
554-
canvasKitVariant: "$canvasKitVariant",
552+
// Some of our tests rely on color emoji
553+
useColorEmoji: true,
554+
canvasKitVariant: "${getCanvasKitVariant()}",
555555
};
556556
</script>
557557
$link

‎lib/web_ui/lib/src/engine/configuration.dart‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,38 @@ FlutterConfiguration get configuration =>
5555
_configuration ??= FlutterConfiguration.legacy(_jsConfiguration);
5656
FlutterConfiguration? _configuration;
5757

58-
/// Sets the given configuration as the current one.
58+
/// Overrides the initial test configuration with new values coming from `newConfig`.
59+
///
60+
/// The initial test configuration (AKA `_jsConfiguration`) is set in the
61+
/// `test_platform.dart` file. See: `window.flutterConfiguration` in `_testBootstrapHandler`.
62+
///
63+
/// The result of calling this method each time is:
64+
///
65+
/// [configuration] = _jsConfiguration + newConfig
66+
///
67+
/// Subsequent calls to this method don't *add* more to an already overridden
68+
/// configuration; this method always starts from an original `_jsConfiguration`,
69+
/// and adds `newConfig` to it.
70+
///
71+
/// If `newConfig` is null, [configuration] resets to the initial `_jsConfiguration`.
5972
///
6073
/// This must be called before the engine is initialized. Calling it after the
6174
/// engine is initialized will result in some of the properties not taking
6275
/// effect because they are consumed during initialization.
6376
@visibleForTesting
64-
void debugSetConfiguration(FlutterConfiguration configuration) {
65-
_configuration = configuration;
77+
void debugOverrideJsConfiguration(JsFlutterConfiguration? newConfig) {
78+
if (newConfig != null) {
79+
final JSObject newJsConfig = objectConstructor.assign(
80+
<String, Object>{}.jsify(),
81+
_jsConfiguration.jsify(),
82+
newConfig.jsify(),
83+
);
84+
_configuration = FlutterConfiguration()
85+
..setUserConfiguration(newJsConfig as JsFlutterConfiguration);
86+
print('Overridden engine JS config to: ${newJsConfig.dartify()}');
87+
} else {
88+
_configuration = null;
89+
}
6690
}
6791

6892
/// Supplies Web Engine configuration properties.
@@ -121,16 +145,6 @@ class FlutterConfiguration {
121145
}
122146
}
123147

124-
FlutterConfiguration merge(JsFlutterConfiguration newConfig) {
125-
final JSAny mergedJsConfig = objectConstructor.assign(
126-
<String, Object?>{}.jsify()!,
127-
_configuration.jsify(),
128-
newConfig.jsify(),
129-
);
130-
return FlutterConfiguration()
131-
..setUserConfiguration(mergedJsConfig as JsFlutterConfiguration);
132-
}
133-
134148
// Static constant parameters.
135149
//
136150
// These properties affect tree shaking and therefore cannot be supplied at

‎lib/web_ui/lib/src/engine/dom.dart‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,12 @@ extension JSAnyToObjectExtension on JSAny {
6363
@JS('Object')
6464
external DomObjectConstructor get objectConstructor;
6565

66-
6766
@JS()
6867
@staticInterop
6968
class DomObjectConstructor {}
7069

7170
extension DomObjectConstructorExtension on DomObjectConstructor {
72-
external JSAny assign(JSAny target, JSAny? source1, [JSAny? source2]);
71+
external JSObject assign(JSAny? target, JSAny? source1, JSAny? source2);
7372
}
7473

7574
@JS()

‎lib/web_ui/test/canvaskit/configuration_canvaskit_variant_test.dart‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,35 +10,36 @@ import 'package:ui/src/engine.dart';
1010

1111
import 'common.dart';
1212

13-
@JS('_flutter_canvaskit_variant_for_test_only')
14-
external String? get _flutterCanvaskitVariantForTestOnly;
15-
1613
void main() {
1714
internalBootstrapBrowserTest(() => testMain);
1815
}
1916

17+
// This test exists to make sure we don't accidentally run the CanvasKit test suite
18+
// in "auto" mode. The CanvasKit variant should always be deterministic.
2019
void testMain() {
2120
setUpCanvasKitTest();
2221

23-
// This is to make sure we don't accidentally run the CanvasKit test suite in
24-
// auto mode. The CanvasKit variant should always be deterministic.
2522
test('CanvasKit tests always run with a specific variant', () {
2623
expect(
2724
configuration.canvasKitVariant,
2825
anyOf(CanvasKitVariant.chromium, CanvasKitVariant.full),
29-
);
30-
expect(
31-
_flutterCanvaskitVariantForTestOnly,
32-
anyOf('chromium', 'full'),
26+
reason: 'canvasKitVariant must be set to "chromium" or "full" in canvaskit tests!',
3327
);
3428
});
3529

36-
// This is to make sure that the variant specified by the test harness is
37-
// correctly preserved during tests in the global `configuration` object.
38-
test('CanvasKitVariant configuration is preserved in tests', () {
39-
expect(
40-
configuration.canvasKitVariant,
41-
CanvasKitVariant.values.byName(_flutterCanvaskitVariantForTestOnly!),
30+
test('debugOverrideJsConfiguration can bypass (and restore) variant', () {
31+
// Set-up in the test_platform.dart file. See `_testBootstrapHandler`.
32+
final CanvasKitVariant originalValue = configuration.canvasKitVariant;
33+
expect(configuration.canvasKitVariant, isNot(CanvasKitVariant.auto));
34+
35+
debugOverrideJsConfiguration(
36+
<String, Object?>{
37+
'canvasKitVariant': 'auto',
38+
}.jsify() as JsFlutterConfiguration?
4239
);
40+
expect(configuration.canvasKitVariant, CanvasKitVariant.auto);
41+
42+
debugOverrideJsConfiguration(null);
43+
expect(configuration.canvasKitVariant, originalValue);
4344
});
4445
}

‎lib/web_ui/test/canvaskit/embedded_views_test.dart‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
// found in the LICENSE file.
44

55
import 'dart:async';
6+
import 'dart:js_interop';
67

7-
import 'package:js/js_util.dart' as js_util;
88
import 'package:test/bootstrap/browser.dart';
99
import 'package:test/test.dart';
1010
import 'package:ui/src/engine.dart';
@@ -806,12 +806,13 @@ void testMain() {
806806

807807
test('works correctly with max overlays == 2', () async {
808808
final Rasterizer rasterizer = CanvasKitRenderer.instance.rasterizer;
809-
final FlutterConfiguration config = FlutterConfiguration()
810-
..setUserConfiguration(
811-
js_util.jsify(<String, Object?>{
812-
'canvasKitMaximumSurfaces': 2,
813-
}) as JsFlutterConfiguration);
814-
debugSetConfiguration(config);
809+
debugOverrideJsConfiguration(
810+
<String, Object?>{
811+
'canvasKitMaximumSurfaces': 2,
812+
}.jsify() as JsFlutterConfiguration?
813+
);
814+
expect(configuration.canvasKitMaximumSurfaces, 2);
815+
expect(configuration.canvasKitVariant, isNot(CanvasKitVariant.auto));
815816

816817
SurfaceFactory.instance.debugClear();
817818

@@ -854,7 +855,7 @@ void testMain() {
854855
]);
855856

856857
// Reset configuration
857-
debugSetConfiguration(FlutterConfiguration());
858+
debugOverrideJsConfiguration(null);
858859
});
859860

860861
test(

‎lib/web_ui/test/common/test_initialization.dart‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// Use of this source code is governed by a BSD-style license that can be
33
// found in the LICENSE file.
44

5-
import 'dart:js_util' as js_util;
6-
75
import 'package:test/test.dart';
86
import 'package:ui/src/engine.dart' as engine;
97
import 'package:ui/ui.dart' as ui;
@@ -21,14 +19,6 @@ void setUpUnitTests({
2119
ui_web.debugEmulateFlutterTesterEnvironment = true;
2220
}
2321

24-
// Some of our tests rely on color emoji
25-
final engine.FlutterConfiguration config = engine.configuration.merge(
26-
js_util.jsify(<String, Object?>{
27-
'useColorEmoji': true,
28-
}) as engine.JsFlutterConfiguration,
29-
);
30-
engine.debugSetConfiguration(config);
31-
3222
debugFontsScope = configureDebugFontsAssetScope(fakeAssetManager);
3323
await engine.initializeEngine(assetManager: fakeAssetManager);
3424
engine.renderer.fontCollection.fontFallbackManager?.downloadQueue.fallbackFontUrlPrefixOverride = 'assets/fallback_fonts/';

‎lib/web_ui/test/engine/configuration_test.dart‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -33,29 +33,6 @@ void testMain() {
3333

3434
expect(config.canvasKitMaximumSurfaces, 16);
3535
});
36-
37-
test('merge', () {
38-
final FlutterConfiguration originalConfig = FlutterConfiguration.legacy(
39-
js_util.jsify(<String, Object?>{
40-
'useColorEmoji': false,
41-
'canvasKitMaximumSurfaces': 99,
42-
}) as JsFlutterConfiguration,
43-
);
44-
final FlutterConfiguration mergedConfig = originalConfig.merge(
45-
js_util.jsify(<String, Object?>{
46-
'useColorEmoji': true,
47-
}) as JsFlutterConfiguration,
48-
);
49-
50-
// `useColorEmoji` should've been overriden.
51-
expect(mergedConfig.useColorEmoji, isTrue);
52-
// `canvasKitMaximumSurfaces` should've been preserved.
53-
expect(mergedConfig.canvasKitMaximumSurfaces, 99);
54-
55-
// Original config should not have been mutated.
56-
expect(originalConfig.useColorEmoji, isFalse);
57-
expect(originalConfig.canvasKitMaximumSurfaces, 99);
58-
});
5936
});
6037

6138
group('setUserConfiguration', () {

0 commit comments

Comments
 (0)