Fix skwasm tests #145570

eyebrowsoffire · 2024-03-21T23:32:08Z

Skwasm tests are now running as bringup: true in postsubmit, but failing. Let's get them fixed.

packages/flutter/lib/src/services/platform_channel.dart

flutter-dashboard · 2024-04-08T19:22:18Z

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #145570 at sha 09c677cd50733fdb8463c864f6c134891beac9ff

flutter-dashboard · 2024-04-08T20:59:20Z

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

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #145570 at sha d691bf354ae49a4571e711f8809d8596c2381a9e

ditman

Nothing jumps to me as totally egregious. (The bulk of the PR comes from the isSkiaWeb-related test skips.)

ditman · 2024-04-08T22:46:15Z

packages/flutter/lib/src/foundation/_capabilities_web.dart

 @JS('window.flutterCanvasKit')
 external JSAny? get _windowFlutterCanvasKit;

+@JS('window._flutter_skwasmInstance')


The mix of cases here is a little alarming... should this be either _flutterSkwasmInstance or _flutter_skwasm_instance?

(I just realized we're in the wrong repo to be discussing this :) )

Yeah this would need to change at the engine level. Yeah maybe the case mixing is weird.

ditman · 2024-04-08T22:46:38Z

dev/bots/test.dart

+    // These tests are broken and need to be fixed.
+    // TODO(jacksongardner): https://github.com/flutter/flutter/issues/71604
+    'test/material/text_field_test.dart',
+    'test/widgets/performance_overlay_test.dart',


Not sure how the performance_overlay_test is breaking, but it might just be failing on detecting that the web doesn't have a performance_overlay (?) #52258 (and others)

The basic difference here is that the HTML renderer silently does nothing when the performance overlay is enabled, and the canvaskit and skwasm renderers actually throw. I think it's fine to just skip it this way for now, since that's what we're doing with CanvasKit.

ditman · 2024-04-08T22:57:04Z

packages/flutter/test/widgets/selectable_region_context_menu_test.dart


 extension on web.HTMLCollection {
-  Iterable<web.Element> get iterable => _genIterable(this);
+  Iterable<web.Element?> get iterable => Iterable<web.Element?>.generate(length, (int index) => item(index));


Why are these iterable of nullable values? (also on CSSRuleList below)

Because collections like these can theoretically have holes in them with undefined in there (yay JavaScript) so item(index) has a nullable return. We do the null assertions inside the test, which feels like the appropriate place.

packages/flutter/test/scheduler/ticker_test.dart

ditman · 2024-04-08T22:58:25Z

packages/flutter/test/material/navigation_rail_test.dart


    // Wait for any pending shader compilation.
-    tester.pumpAndSettle();
+    await tester.pumpAndSettle();


Good catches!

The test harness actually caught this! The widgets test binding has some code that guards against async stuff escaping outside of the test. For whatever reason, it didn't trigger in DDC, but running in dart2wasm triggered the async guard.

ditman · 2024-04-08T23:02:30Z

packages/flutter/lib/src/foundation/capabilities.dart

+/// Returns true if the application is using CanvasKit or Skwasm.
+///
+/// Only to be used for web.
+bool get isSkiaWeb => isCanvasKit || isSkwasm;


This change is bloating the PR quite a bit (I suppose everywhere we check isCanvasKit now we check isSkiaWeb?)

I wonder if this would be much simpler if:

isCanvasKit => capabilities.isCanvasKit || capabilities.isSkwasm;

And

isSkwasm => capabilities.isSkwasm;

To separate the global "canvaskit" from the skwasm-specific cases?

I mean, the capabilities of the renderer are similar, aren't they?

Essentially I'm doing a rename of the variable to be more accurate. I don't think we should have a variable called isCanvasKit that is true when the renderer is skwasm, that's just confusing. Hence, we have three separate explicit variables. Minimizing the number of lines changed in this particular PR is a non-goal, I'd rather the code be more clear.

Co-authored-by: David Iglesias <[email protected]>

ditman

If CI is happy, I don't have more to say!

flutter/flutter@4967a94...97cd47a 2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541) 2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538) 2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531) 2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527) 2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528) 2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488) 2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524) 2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021) 2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570) 2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515) 2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Skwasm tests are now running as `bringup: true` in postsubmit, but failing. Let's get them fixed.

flutter/flutter@4967a94...97cd47a 2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541) 2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538) 2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531) 2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527) 2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528) 2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488) 2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524) 2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021) 2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570) 2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515) 2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

JaffaKetchup · 2024-06-08T10:01:10Z

packages/flutter/lib/src/foundation/constants.dart

+///   in tests with [debugDefaultTargetPlatformOverride].
+/// * [dart:io.Platform], a way to find out the browser's platform that is not
+///   overridable in tests.
+const bool kIsWasm = kIsWeb && bool.fromEnvironment('dart.library.ffi');


dart-lang/sdk@6b71aa1 adds a proper environment variable to detect WASM - is there a difference?
EDIT: Having tested it out, the Dart env var always seems to be false?

The dart version will only work on Dart SDK version 3.5.0-185.0.dev and above.

I'm guessing that would be a better way to do this, though.

Yep, tested on .3 (included with latest Flutter beta) (I'm pretty sure at least), but didn't seem to work properly. Maybe I was doing something wrong though? Or there's an issue with its implementation in Dart?

dart dart --version for me

Can't right now, but I'm 99% sure it was that version. I've been tracking that commit (I didn't realise this constant existed) and waiting for it to arrive, and I know for a fact I did a flutter upgrade. So it should be .3 and that should be what's on the Flutter beta atm.
I can try again tomorrow.

github-actions bot removed the f: routes Navigator, Router, and related APIs. label Mar 29, 2024

kevmoo reviewed Apr 3, 2024

View reviewed changes

packages/flutter/lib/src/services/platform_channel.dart Outdated Show resolved Hide resolved

eyebrowsoffire marked this pull request as ready for review April 8, 2024 18:46

flutter-dashboard bot added the will affect goldens Changes to golden files label Apr 8, 2024

Enable and fix flutter framework unit tests on dart2wasm+skwasm

e59a641

eyebrowsoffire force-pushed the fix_skwasm_tests branch from c108806 to e59a641 Compare April 8, 2024 21:35

eyebrowsoffire requested a review from ditman April 8, 2024 21:38

ditman reviewed Apr 8, 2024

View reviewed changes

Update packages/flutter/test/scheduler/ticker_test.dart

03fad71

Co-authored-by: David Iglesias <[email protected]>

ditman approved these changes Apr 9, 2024

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024

auto-submit bot merged commit 51e70fa into flutter:master Apr 9, 2024

engine-flutter-autoroll mentioned this pull request Apr 10, 2024

Roll Flutter from 4967a94cd907 to 74b65d4e4c61 (10 revisions) flutter/packages#6496

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024

51e70fa Fix skwasm tests (flutter/flutter#145570)

dae6178

engine-flutter-autoroll mentioned this pull request Apr 10, 2024

Roll Flutter from 4967a94cd907 to 97cd47a02e60 (11 revisions) flutter/packages#6497

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024

51e70fa Fix skwasm tests (flutter/flutter#145570)

1777ded

gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024

Fix skwasm tests (flutter#145570)

83b2ab2

Skwasm tests are now running as `bringup: true` in postsubmit, but failing. Let's get them fixed.

JaffaKetchup reviewed Jun 8, 2024

View reviewed changes

eyebrowsoffire deleted the fix_skwasm_tests branch December 12, 2025 21:54

Fix skwasm tests #145570

Fix skwasm tests #145570

Uh oh!

Conversation

eyebrowsoffire commented Mar 21, 2024

Uh oh!

Uh oh!

flutter-dashboard bot commented Apr 8, 2024

Uh oh!

flutter-dashboard bot commented Apr 8, 2024

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

JaffaKetchup Jun 8, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JaffaKetchup Jun 8, 2024 •

edited

Loading