[web] Preserve canvaskit variant during tests. #43868

ditman · 2023-07-20T21:33:25Z

A ~~simpler~~ very similar version of #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

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…zation (flutter#43854)" This reverts commit 6c224e8.

…dded to felt.

lib/web_ui/test/canvaskit/configuration_canvaskit_variant_test.dart

This way we cannot accidentally remove/unset properties when writing tests.

mdebbar

Thanks for making this improvement!

My only serious concern is with the behavior of debugOverrideJsConfiguration(null).

mdebbar · 2023-07-21T15:15:55Z

lib/web_ui/dev/test_platform.dart

          <title>${htmlEscape.convert(test)} Test</title>
          <meta name="assetBase" content="/">
          <script>
-            window._flutter_canvaskit_variant_for_test_only = "$canvasKitVariant";


I still think this is valuable as it protects us from accidental messing with the configuration. I don't feel too strongly about it though, so I'll leave it up to your judgement.

I am just trying to avoid adding more special global stuff to the configuration of the flutter app. In this case, it feels extra redundant because we're already passing it through the engine configuration call a few lines below.

mdebbar · 2023-07-21T15:21:48Z

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

-/// Sets the given configuration as the current one.
+/// Overrides [configuration] with new values coming from `newConfig`.
+///
+/// If `newConfig` is null, the override is removed.


It isn't clear to me what "removing the override" means. Do we go back to the hardcoded default values? Do we go back to what was specified in window.flutterConfiguration?

By looking at the code, it looks like the former is the case. If that's correct, then I think we should change it. Going back to the hardcoded defaults means we are resetting the configuration for all subsequent tests, and we lose the configurations that were passed by test_platform.dart.

In other similar situations, we keep two copies: the original value (in this case, it's whatever came from window.flutterConfiguration) and the override value. When debugOverrideXXX = null; then we just set the override value to null but we keep the original value. An example here:

engine/lib/web_ui/lib/src/engine/browser_detection.dart

Lines 54 to 56 in 3342e09

BrowserEngine get browserEngine {

return debugBrowserEngineOverride ?? _browserEngine;

}

If my understanding is correct, something like this captures the failure if test_platform.dart sets a canvasKitVariant that's different from the default:

final CanvasKitVariant originalValue = configuration.canvasKitVariant; debugOverrideJsConfiguration( <String, Object?>{ 'canvasKitVariant': 'auto', }.jsify() as JsFlutterConfiguration? ); expect(configuration.canvasKitVariant, CanvasKitVariant.auto); debugOverrideJsConfiguration(null); expect(configuration.canvasKitVariant, originalValue);

When we remove the override in configuration, we should be going back to the default configuration values set in JS:

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

Lines 53 to 56 in cd8cfa0

/// The Web Engine configuration for the current application.

FlutterConfiguration get configuration =>

_configuration ??= FlutterConfiguration.legacy(_jsConfiguration);

FlutterConfiguration? _configuration;

The test that you proposed above should indeed pass, let's add it to the configuration tests!

I added the test to the same test file that tests that the canvaskit variant is set by default (but that it can also be overridden).

I've also expanded the documentation of the method a little bit, but I'm not sure if it's doing it any favors for clarity or making it worse 😮‍💨, let me know!

lib/web_ui/test/canvaskit/embedded_views_test.dart

lib/web_ui/test/common/test_initialization.dart

mdebbar

mdebbar · 2023-07-25T15:38:45Z

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

+/// Subsequent calls to this method don't *add* more to an already overridden
+/// configuration; this method always starts from an original `_jsConfiguration`,
+/// and adds `newConfig` to it.


Oh interesting. Good call on clarifying this!

…ine#43868)

…131314) flutter/engine@43f727e...df12fff 2023-07-26 [email protected] [web] Preserve canvaskit variant during tests. (flutter/engine#43868) 2023-07-26 [email protected] Roll Skia from 28773cec6e8d to 826e38ba8db3 (5 revisions) (flutter/engine#44031) 2023-07-26 [email protected] Roll Fuchsia Mac SDK from 0StTjIqUxGkc3nOWT... to d6O9t74z-k2svOmvz... (flutter/engine#44030) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 0StTjIqUxGkc to d6O9t74z-k2s If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#131314) flutter/engine@43f727e...df12fff 2023-07-26 [email protected] [web] Preserve canvaskit variant during tests. (flutter/engine#43868) 2023-07-26 [email protected] Roll Skia from 28773cec6e8d to 826e38ba8db3 (5 revisions) (flutter/engine#44031) 2023-07-26 [email protected] Roll Fuchsia Mac SDK from 0StTjIqUxGkc3nOWT... to d6O9t74z-k2svOmvz... (flutter/engine#44030) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 0StTjIqUxGkc to d6O9t74z-k2s If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

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

ditman added 2 commits July 20, 2023 14:14

Revert "[web] Preserve correct CanvasKit Variant during test initiali…

d131388

…zation (flutter#43854)" This reverts commit 6c224e8.

Set emoji config in the test_platform.dart. Preserve all safeguards a…

bb636fc

…dded to felt.

ditman requested a review from mdebbar July 20, 2023 21:33

github-actions bot added the platform-web Code specifically for the web engine label Jul 20, 2023

mdebbar reviewed Jul 20, 2023

View reviewed changes

lib/web_ui/test/canvaskit/configuration_canvaskit_variant_test.dart Show resolved Hide resolved

ditman added 5 commits July 20, 2023 17:40

Remove unused import.

202892a

Recover configuration test.

9e416d0

Remove extra whitespace.

f83d7aa

Preserve canvaskit config in embedded_view_test.dart

6464ca8

Always merge config when overriding.

2d39f4e

This way we cannot accidentally remove/unset properties when writing tests.

ditman requested a review from mdebbar July 21, 2023 02:15

mdebbar reviewed Jul 21, 2023

View reviewed changes

Addressed PR feedback.

1a67e36

ditman requested a review from mdebbar July 21, 2023 18:47

mdebbar approved these changes Jul 25, 2023

View reviewed changes

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2023

auto-submit bot merged commit df12fff into flutter:main Jul 26, 2023

ditman deleted the preserve-canvaskit-variant-during-tests branch July 26, 2023 05:04

engine-flutter-autoroll mentioned this pull request Jul 26, 2023

Roll Flutter Engine from 43f727e4748a to df12fff329a1 (3 revisions) flutter/flutter#131314

Merged

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

df12fff32 [web] Preserve canvaskit variant during tests. (flutter/eng…

f10a369

…ine#43868)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[web] Preserve canvaskit variant during tests. #43868

[web] Preserve canvaskit variant during tests. #43868

Uh oh!

ditman commented Jul 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

mdebbar left a comment

Uh oh!

mdebbar Jul 21, 2023

Uh oh!

ditman Jul 21, 2023

Uh oh!

mdebbar Jul 21, 2023

Uh oh!

mdebbar Jul 21, 2023 •

edited

Loading

Uh oh!

ditman Jul 21, 2023

Uh oh!

ditman Jul 21, 2023

Uh oh!

Uh oh!

Uh oh!

mdebbar left a comment

Uh oh!

mdebbar Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	BrowserEngine get browserEngine {
	return debugBrowserEngineOverride ?? _browserEngine;
	}

	/// The Web Engine configuration for the current application.
	FlutterConfiguration get configuration =>
	_configuration ??= FlutterConfiguration.legacy(_jsConfiguration);
	FlutterConfiguration? _configuration;

[web] Preserve canvaskit variant during tests. #43868

[web] Preserve canvaskit variant during tests. #43868

Uh oh!

Conversation

ditman commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

ditman Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 21, 2023 • 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!

ditman Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

ditman Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ditman commented Jul 20, 2023 •

edited

Loading

mdebbar Jul 21, 2023 •

edited

Loading