Get unit tests working with dart2wasm #38784
Conversation
flutter-dashboard
bot
added
the
platform-web
lib/web_ui/lib/src/engine/view_embedder/hot_restart_cache_handler.dart
Outdated
Show resolved
Hide resolved
| 'Bad state: Test.\n' | ||
| 'The picture has been disposed. ' | ||
| 'When the picture was disposed the stack trace was:\n' | ||
| ), | ||
| contains('StackTrace_current'), |
There was a problem hiding this comment.
Was this omitted intentionally? If this isn’t expected in wasm, we could guard it by an if.
There was a problem hiding this comment.
Yes this was omitted intentionally. This is broken in dart2wasm, and I thought the validation of the exact token "StackTrace_current" felt a little brittle. We're not actually emitting the StackTrace_current part, that's coming from the dart runtime, so I don't know why our unit tests should be testing that string in particular. If you feel strongly about it, I can bring that check back just for non-wasm cases, but I don't really like the check in general personally.
| @@ -73,32 +72,6 @@ void testMain() { | |||
| ); | |||
| }); | |||
|
|
|||
| test('responds correctly to flutter/platform Clipboard.getData failure', | |||
There was a problem hiding this comment.
Can this be skipped for wasm instead of deleted?
There was a problem hiding this comment.
The reason I deleted it is because it doesn't actually do what it says it does. The setting of the clipboard actually just silently fails in dart2js, so the test never actually did what it said it would do. In dart2wasm, the runtime is in strict mode, so it threw when we tried to set the clipboard. On deeper analysis on dart2js, the whole test is basically bogus, so I just deleted it.
eyebrowsoffire
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 38784, due to - The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
| dimensions: <String, String> {'Browser': browserName}, | ||
| dimensions: <String, String> { | ||
| 'Browser': browserName, | ||
| if (isWasm) 'Wasm': 'true', |
There was a problem hiding this comment.
This means we may end up in a BROWSER x WASM x RENDERER kind of matrix, and I think we only care about BROWSER x MODE where MODE is much more limited than the full matrix. We only care about these modes:
- HTML-JS
- CanvasKit-JS
- Skwasm-Wasm
We don't care about testing these:
- HTML-Wasm
- CanvasKit-Wasm
- Skwasm-JS (for now)
But maybe it's OK to keep this more expressive than we need.
There was a problem hiding this comment.
We definitely do care about testing HTML-Wasm and CanvasKit-Wasm, at least right now. That's what this PR is actually doing, adding tests for those.
| /// Whether we are running from a wasm module compiled with dart2wasm. | ||
| /// Note: Currently the ffi library is available from dart2wasm but not dart2js | ||
| /// or dartdevc. | ||
| bool get isWasm => const bool.fromEnvironment('dart.library.ffi'); |
There was a problem hiding this comment.
1 != 1.0 would also do the trick, I think.
There was a problem hiding this comment.
I prefer this, because it's very clearly const. 1 != 1.0 might not be optimized by the baseline compiler to be const.
| /// The js-interop layer backing [_elements]. | ||
| /// | ||
| /// Elements are stored in a JS global array named [defaultCacheName]. | ||
| late List<DomElement?>? _jsElements; | ||
| late List<Object?>? _jsElements; |
There was a problem hiding this comment.
I'm not 100% sure. I had to iterate a bunch on this to keep it from exploding in both JS and wasm targets. Let me try changing these back to DomElement? and see if that works.
There was a problem hiding this comment.
Yeah I tried changing it back, but I get these sorts of errors from dart2wasm:
Type '_GrowableList<Object?>' is not a subtype of type 'List<JSValue?>?' in type cast
The js interop runtime just returns a weakly typed list of objects. So we have to explicitly cast elements one way or another, either by doing a .cast<DomElement?> on the list or just doing it at the point of removal which is what I'm doing now. I think this way is a bit simpler.
…118743) * 2722c548b Remove use of SkTAddOffset and sk_careful_memcpy (flutter/engine#38977) * 290636c1c Get unit tests working with dart2wasm (flutter/engine#38784)
…lutter#118743) * 2722c548b Remove use of SkTAddOffset and sk_careful_memcpy (flutter/engine#38977) * 290636c1c Get unit tests working with dart2wasm (flutter/engine#38784)
This has a lot of changes to get our unit tests running and passing with dart2wasm. Some tests have been skipped, but I have filed issues for us to follow up and get them unskipped as soon as we have a solution for those test cases.