Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@eyebrowsoffire
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire commented Sep 19, 2022

This adds some preliminary support for compiling our unit tests to WebAssembly and running them. This doesn't actually work yet for two reasons: first, the version of Chrome we use for unit tests is 105.0, but many of the features used by dart2wasm are only available in Chrome 107, which is currently in dev (not stable). Secondly, there are still a number of issues with the compiler itself and the JS interop machinery, and the test harness simply fails at runtime right now.

Hence, compiling unit tests to wasm actually does work with this patch, but running them does not, at least for the time being. That being said, these are all hidden behind a flag, so our normal testing and build paths are not affected, so I think we can merge this in without affecting anything. Further changes down the line can be made to this as needed as we upgrade to newer versions of Chromium and bugs get fixed in the Dart SDK.

@eyebrowsoffire eyebrowsoffire added the Work in progress (WIP) Not ready (yet) for review! label Sep 19, 2022
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Sep 19, 2022
@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 15.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

@eyebrowsoffire eyebrowsoffire removed the Work in progress (WIP) Not ready (yet) for review! label Sep 23, 2022
@skia-gold
Copy link

Gold has detected about 34 new digest(s) on patchset 19.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

@skia-gold
Copy link

Gold has detected about 4 new digest(s) on patchset 19.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

@flutter-dashboard
Copy link

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.

Changes reported for pull request #36255 at sha 07db831

@override
String get name => 'run';

bool get wasm => boolArg('wasm');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this bool everywhere we pass it

bool _closed = false;

/// Whether we are running tests that have been compiled to WebAssembly.
final bool _wasm;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise, _isWasm

Copy link
Contributor

@joshualitt joshualitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great modulo some nits

@joshualitt joshualitt requested a review from jakemac53 October 27, 2022 22:22
@joshualitt
Copy link
Contributor

@jakemac53 , I know you can't give a thorough review, I'm sending this to your desk mostly as FYI.

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 27.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 28.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

Copy link
Contributor

@joshualitt joshualitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be ready to go on this now that package:test has published.

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 31.
View them at https://flutter-engine-gold.skia.org/cl/github/36255

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// are designed to run in one specific mode. So instead, we specify the
// renderer explicitly.
'-DFLUTTER_WEB_AUTO_DETECT=false',
'-DFLUTTER_WEB_USE_SKIA=${renderer == Renderer.canvasKit}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this codepath always use skwasm? Or are we compiling everything with dart2wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is attempting to compile all the unit tests (which aren't skwasm) with dart2wasm. At this point dart2wasm doesn't require skwasm, but skwasm requires dart2wasm. All the skwasm rendering code I have so far is in a branch, which I'm waiting to merge until I can write some unit tests against it, which requires this change.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022
@auto-submit auto-submit bot merged commit 4d1d7a4 into flutter:main Nov 2, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2022
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants