-
Notifications
You must be signed in to change notification settings - Fork 6k
Wasm harness for unit tests. #36255
Wasm harness for unit tests. #36255
Conversation
|
Gold has detected about 1 new digest(s) on patchset 15. |
|
Gold has detected about 34 new digest(s) on patchset 19. |
|
Gold has detected about 4 new digest(s) on patchset 19. |
|
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. |
lib/web_ui/dev/run.dart
Outdated
| @override | ||
| String get name => 'run'; | ||
|
|
||
| bool get wasm => boolArg('wasm'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think according to effective dart this should be isWasm: https://dart.dev/guides/language/effective-dart/design#prefer-a-non-imperative-verb-phrase-for-a-boolean-property-or-variable
There was a problem hiding this comment.
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
lib/web_ui/dev/test_platform.dart
Outdated
| bool _closed = false; | ||
|
|
||
| /// Whether we are running tests that have been compiled to WebAssembly. | ||
| final bool _wasm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise, _isWasm
joshualitt
left a comment
There was a problem hiding this 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
|
@jakemac53 , I know you can't give a thorough review, I'm sending this to your desk mostly as FYI. |
|
Gold has detected about 1 new digest(s) on patchset 27. |
|
Gold has detected about 1 new digest(s) on patchset 28. |
joshualitt
left a comment
There was a problem hiding this 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.
|
Gold has detected about 1 new digest(s) on patchset 31. |
harryterkelsen
left a comment
There was a problem hiding this 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}', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.