Added golden test to make sure that spawn engines work. #23066

gaaclarke · 2020-12-15T01:17:27Z

Description

I also had to fix a bug that left us with spawn engines sharing a platform view controller between it and the spawner.

Related Issues

fixes flutter/flutter#72319

Tests

I added the following tests:

Create->Spawn->Kill integration test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

gaaclarke · 2020-12-17T00:25:05Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

-      [self](flutter::Shell& shell) {
-        [self recreatePlatformViewController];
+      [result](flutter::Shell& shell) {
+        [result recreatePlatformViewController];
        return std::make_unique<flutter::PlatformViewIOS>(
-            shell, self->_renderingApi, self->_platformViewsController, shell.GetTaskRunners());


This was the root of all the problems I experienced the last couple of days. I don't think we need the ScopedTaskRunner. I got confused..

This was the root of all the problems I experienced the last couple of days. I don't think we need the ScopedTaskRunner.

Wait rly? Ha, awesome.

Yea, so I was getting crashes inside the raster thread after the spawner was killed. It was a crash talking to the external view embedder. So, it appeared that the raster event queue had a queued event talking to that deleted rasterizer. It turns out the rasterizer that was involved in the crash was actually the spawned engines rasterizer. So, the event was correctly going forward with a live rasterizer but it ultimately was referencing something that had been deleted, the platformViewsController. It's easy to overlook that that is a shared_ptr, even then I didn't expect there to be a crash inside. The shared_ptr is kind of pointless if it doesn't actually maintain ownership of its children (or use weak ptrs).

xster

Awesome

xster · 2020-12-17T05:35:44Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

-      [self](flutter::Shell& shell) {
-        [self recreatePlatformViewController];
+      [result](flutter::Shell& shell) {
+        [result recreatePlatformViewController];
        return std::make_unique<flutter::PlatformViewIOS>(
-            shell, self->_renderingApi, self->_platformViewsController, shell.GetTaskRunners());


This was the root of all the problems I experienced the last couple of days. I don't think we need the ScopedTaskRunner.

Wait rly? Ha, awesome.

xster · 2020-12-17T05:38:33Z

testing/scenario_app/run_ios_tests.sh

@@ -36,6 +36,6 @@ fi
 cd ios/Scenarios
 set -o pipefail && xcodebuild -sdk iphonesimulator \
  -scheme Scenarios \
-  -destination 'platform=iOS Simulator,name=iPhone 8' \
+  -destination 'platform=iOS Simulator,OS=13.0,name=iPhone 8' \


what was the issue? Was it behaving different on Xcode 12?

Yea, on CI their default OS is 13, if you run it with a recent version of Xcode it will use iOS 14 which will cause all the tests to fail (minor differences in the golden images).

xster · 2020-12-17T05:39:20Z

testing/scenario_app/lib/src/scenarios.dart

@@ -46,6 +46,7 @@ Map<String, ScenarioFactory> _scenarios = <String, ScenarioFactory>{
  'initial_route_reply': () => InitialRouteReply(PlatformDispatcher.instance),
  'platform_view_with_continuous_texture': () => PlatformViewWithContinuousTexture(PlatformDispatcher.instance, 'Platform View', id: _viewId++),
  'bogus_font_text': () => BogusFontText(PlatformDispatcher.instance),
+  'spawn_engine_works' : () => BogusFontText(PlatformDispatcher.instance),


You're compounding 2 tests. Maybe something generic like PoppableScreenScenario?

I planned on doing this. Unfortunately I spent hours yesterday trying to fix my scenario tests. I keep getting a bad kernel error. I can't figure it out so I can't work on this right now.

flutter/flutter#72525

xster · 2020-12-17T05:44:20Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/SpawnEngineTest.m

+
+#import "SpawnEngineTest.h"
+#import "GoldenImage.h"
+


might be simpler if you used GoldenTestManager.h

I had an eye out for this too. These tests are kind of a mess. You don't want to be in a situation where when you add tests you have to edit existing code, you should just be writing new code. That's why I don't like the GoldenTestManager. Anything that seemed to have references to platform view tests I tried to avoid, to help us try to get a perspective on what a test should really have. I didn't have a chance to refactor because my environment is broken.

GoldenTestManager isn't related to platform view anymore. It was previously but I refactored it a bit a few weeks ago.

This is specifically what I'm talking about:
https://github.com/gaaclarke/engine/blob/788f879350fc48199415d0eecf708e33c3d2ebf5/testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenTestManager.m#L26:L26

This has to be edited to be used. Once I saw "platform view" I avoided it.

…ter/engine#23066)

* c608b07 bump fuchsia toolchain to clang-12 (flutter/engine#23067) * 8a3e9a2 fix crash in FontCollection::init() when FontFamily is empty (flutter/engine#23019) * 95ba5ca Roll Skia from 6f31e27f1e29 to 85fa75616dfe (7 revisions) (flutter/engine#23118) * 50e7d89 [web] Switch web-render option default to auto (flutter/engine#23090) * 8854520 Roll Skia from 85fa75616dfe to d6f2338ab194 (3 revisions) (flutter/engine#23119) * acad21c [web] Tests for rich paragraph DOM (flutter/engine#23097) * 27ebbc4 Rename PointerState.isDown as per style guide (flutter/engine#23120) * 19950f5 [web] Rich paragraph getBoxesForRange (flutter/engine#23098) * 581acbe Roll Skia from d6f2338ab194 to 1d89532d5988 (1 revision) (flutter/engine#23122) * 540b191 Roll Fuchsia Mac SDK from acylwa3i4... to chLTYsKMR... (flutter/engine#23125) * 3b52edf Roll Skia from 1d89532d5988 to 7839f66540b6 (1 revision) (flutter/engine#23126) * 6b25350 Roll Fuchsia Linux SDK from TIKHoiQyP... to wu6yV-_BL... (flutter/engine#23127) * 9c72085 Roll Skia from 7839f66540b6 to 20f1b3462878 (1 revision) (flutter/engine#23129) * ee323d0 Roll Skia from 20f1b3462878 to 995f0366bd21 (2 revisions) (flutter/engine#23132) * 625aa69 Roll Skia from 995f0366bd21 to b64da3907f76 (1 revision) (flutter/engine#23135) * affc421 Roll Skia from b64da3907f76 to 81da68af2ecf (7 revisions) (flutter/engine#23142) * 9a6a31e Roll Fuchsia Mac SDK from chLTYsKMR... to RDUxjnng0... (flutter/engine#23143) * 78657ed Added golden test to make sure that spawn engines work. (flutter/engine#23066) * bb9cac2 Roll Fuchsia Linux SDK from wu6yV-_BL... to _l04etgVd... (flutter/engine#23145) * 51c9ae9 Add --strict_null_safety_checks to the Dart flag allowlist (flutter/engine#23144) * 76310c4 Add missing sdk constriant in pubspec.yaml files. (flutter/engine#23124) * 6e54f0d [fuchsia] Add wrapper for zx_clock_get_monotonic. (flutter/engine#23128) * ade75e0 [web] Rich paragraph getPositionForOffset (flutter/engine#23133) * 2ef2c86 Fix engine in preparation for implementing dart-lang/language#1274 (flutter/engine#23131) * 9384324 Update android_lint deps (flutter/engine#23151) * ea4bb2a Roll Skia from 81da68af2ecf to 7b920446a8fc (14 revisions) (flutter/engine#23152) * 4b5e4e6 Make it easier to turn on Xcode symlinks (flutter/engine#23150) * 325f069 Roll fuchsia toolchain (flutter/engine#23155) * 05704d3 Use include for C/C++ headers in darwin/macos (flutter/engine#23035) * 8d3d69b Turned no malloc scribble and randomized the tests. (flutter/engine#23014) * 418cc48 Fix macOS crash when modifier keys pressed. (flutter/engine#23154) * a8c360d Update FlutterPlatformViewsTests (flutter/engine#23158) * d941aef [web] Rich text painting on bitmap canvas (flutter/engine#23136) * 67cb0f3 Revert "[web] Switch web-render option default to auto (#23090)" (flutter/engine#23161) * c4b48c5 Roll Skia from 7b920446a8fc to dfc880bd9ba0 (14 revisions) (flutter/engine#23164) * de1de9d Disable FlutterPluginAppLifeCycleDelegateTest testWillResignActive (flutter/engine#23166)

googlebot added the cla: yes label Dec 15, 2020

gaaclarke added 2 commits December 16, 2020 15:09

Added golden test to make sure that spawn engines work.

074346f

fixed spawn platform view controller

54b2745

gaaclarke force-pushed the lfe-spawn-golden-test branch 2 times, most recently from feabb30 to d4673d9 Compare December 17, 2020 00:03

stoped keeping the spawner alive

788f879

gaaclarke force-pushed the lfe-spawn-golden-test branch from d4673d9 to 788f879 Compare December 17, 2020 00:19

flutter-dashboard bot added the platform-ios label Dec 17, 2020

gaaclarke marked this pull request as ready for review December 17, 2020 00:21

gaaclarke requested a review from xster December 17, 2020 00:24

gaaclarke commented Dec 17, 2020

View reviewed changes

xster approved these changes Dec 17, 2020

View reviewed changes

gaaclarke merged commit 78657ed into flutter:master Dec 17, 2020

engine-flutter-autoroll mentioned this pull request Dec 17, 2020

Roll Engine from fd6b409f950b to bb9cac29a030 (19 revisions) flutter/flutter#72536

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 17, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

e4d830e

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 17, 2020

Roll Engine from fd6b409f950b to 76310c4264df (21 revisions) flutter/flutter#72547

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 17, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

3b3e182

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 17, 2020

Roll Engine from fd6b409f950b to 9384324cc15c (25 revisions) flutter/flutter#72556

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 17, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

c42fd0b

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 17, 2020

Roll Engine from fd6b409f950b to 4b5e4e60578b (27 revisions) flutter/flutter#72564

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 17, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

0391068

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 18, 2020

Roll Engine from fd6b409f950b to 8d3d69b6b0f9 (30 revisions) flutter/flutter#72566

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 18, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

854b9f9

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 18, 2020

Roll Engine from fd6b409f950b to 8d3d69b6b0f9 (30 revisions) flutter/flutter#72575

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 18, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

ff130f4

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 18, 2020

Roll Engine from fd6b409f950b to a8c360dc241a (32 revisions) flutter/flutter#72580

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 18, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

3eec54c

…ter/engine#23066)

engine-flutter-autoroll mentioned this pull request Dec 18, 2020

Roll Engine from fd6b409f950b to de1de9d44f58 (36 revisions) flutter/flutter#72584

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 18, 2020

78657ed Added golden test to make sure that spawn engines work. (flut…

efc7c13

…ter/engine#23066)

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021

Added golden test to make sure that spawn engines work. (flutter#23066)

c7b1d18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added golden test to make sure that spawn engines work. #23066

Added golden test to make sure that spawn engines work. #23066

gaaclarke commented Dec 15, 2020 •

edited

Loading

gaaclarke Dec 17, 2020

xster Dec 17, 2020

gaaclarke Dec 17, 2020

xster left a comment

xster Dec 17, 2020

xster Dec 17, 2020

gaaclarke Dec 17, 2020

xster Dec 17, 2020

gaaclarke Dec 17, 2020

gaaclarke Dec 17, 2020

xster Dec 17, 2020

gaaclarke Dec 17, 2020

xster Dec 17, 2020

gaaclarke Dec 17, 2020


		#import "SpawnEngineTest.h"
		#import "GoldenImage.h"

Added golden test to make sure that spawn engines work. #23066

Added golden test to make sure that spawn engines work. #23066

Conversation

gaaclarke commented Dec 15, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Dec 15, 2020 •

edited

Loading