Add some AppLifecycleTests #11890

xster · 2019-09-05T03:43:22Z

I was pre-emptively adding some test before fixing flutter/flutter#37226.

Added some stuff to make the Scenario target runnable and interactive without wrapping with a test.

dnfield · 2019-09-05T04:51:15Z

I was under the impression we didn't want to add swift to the language surface of anything we maintain. @cbracken @chinmaygarde thoughts?

xster · 2019-09-05T05:06:10Z

We don't want to add swift to anything people depend on since there's users before 12.2. This is only in leaf targets that are only in tests that no one else uses.

dnfield · 2019-09-05T05:13:50Z

I thought part of the idea was not adding more languages the team/contributors need to know

chinmaygarde · 2019-09-05T05:34:52Z

If there isn't a pressing reason to use Swift, I'd rather stick to Objective-C. While Dan's point of needing to be language polyglot as being a potential hinderance to future contributions is certainly valid, I am also concerned that the usage of Swift might make it harder to integrate this target into the GN so that we can build all engine targets in one go. Currently, the way the Scenario's app is put together is a bit of a hack and needs a separate invocation after building the engine itself. Dan's comment here suggests that this is temporary till proper build system integration is in place. Needing to support Swift in GN will now become a prerequisite to this work.

As these are the first Swift TUs in the engine, I'd hold off on setting this precedent till wider consensus on Swift in the engine is reached.

xster · 2019-09-06T01:19:20Z

Doh. I got excited too early.

In terms of our users, they're already there (https://dashboards.corp.google.com/_8211ecd6_e372_4f1c_b4f8_a1f4f0411822#oyjxd7sm0, 26% of our users use swift currently) but the build system is a sensible reason. I'll swap them back to obj-c.

xster · 2019-09-09T20:52:00Z

Alright, re-translated. PTAL while I go puke some square brackets.

xster · 2019-09-09T21:00:24Z

I don't know what CQ+1 does but it sounds fancy so I'm trying it.

chinmaygarde · 2019-09-09T21:17:11Z

don't know what CQ+1 does but it sounds fancy

I edited the label description. It verified this PR in a try-job on LUCI. And yes, it is very fancy.

jmagman · 2019-09-09T21:13:22Z

testing/scenario_app/ios/Scenarios/Scenarios/Util.h

+
+#import <Flutter/Flutter.h>
+
+@interface Util : NSObject


Util isn't a great name. Maybe make this a FlutterEngine category? Also _Nullable shouldn't be used on NSObjects.

NS_ASSUME_NONNULL_BEGIN @interface FlutterEngine (ScenarioTesting) - (void)runWithScenario:(NSString*)scenario withCompletion:(void (^)(void))engineRunCompletion; @end NS_ASSUME_NONNULL_END

oh ya, good idea. Done.

Also _Nullable shouldn't be used on NSObjects.

Not sure I understood. But I think I did what you meant.

Sorry, I meant to use nullable instead of _Nullable. Rule of thumb if the compiler lets you use nullable, then use it.

ohhhh withCompletion:(void (^_Nullable)(void))engineRunCompletion; should be withCompletion:(nullable void (^)(void))engineRunCompletion;. Sigh block syntax...

jmagman · 2019-09-09T21:16:08Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+    [engineStartedExpectation fulfill];
+  }];
+
+  [self waitForExpectations:[NSArray arrayWithObject:engineStartedExpectation] timeout:5];


You don't need to specify the expectation, you can use -[self waitForExpectationsWithTimeout:5.0:handler:nil]

jmagman · 2019-09-09T21:23:09Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+  [self waitForExpectations:lifecycleExpectations timeout:5];
+
+  // Expected sequence from showing the FlutterViewController is inactive and resumed.
+  BOOL lifecyclesAsExpected = [lifecycleEvents


Instead of doing this array checking, you can enforce expectation order. Something like:

XCTestExpectation *inactiveExpectation = [self expectationWithDescription:@"inactive"]; XCTestExpectation *resumedExpectation = [self expectationWithDescription:@"resume"]; [engine.lifecycleChannel setMessageHandler:^(id message, FlutterReply callback) { if ([message isEqual:@"AppLifecycleState.inactive"]) { [inactiveExpectation fulfill]; else if ([message isEqual:@"AppLifecycleState.resumed"]) { [resumedExpectation fulfill]; } else { XCTFail(@"Unexpected lifecycle transition: %@", message); } }]; [self waitForExpectations:@[inactiveExpectation, resumedExpectation] timeout:5.0 enforceOrder:YES];

I think this would be confusing since I'm going to be checking that something isn't done once I fix the actual production problem in the next PR so I wouldn't be waiting for expectations in places.

What will the message be in the next PR that you're going to forbid that isn't covered by the XCTFail fall-through? Can you invert the expectation? I can write some example code with a concrete example.

Oh, that's really useful. I think I'll go with your approach when I fix flutter/flutter#37226 and flip the check around.

How would I synchronize at each step to make sure nothing happens then? Would it still be timeout based?

Otherwise, I would expect the error message to be in the shape of:

Expecting transitions inactive, resume. But you got inactive, paused, inactive, resume, inactive, resume.

-inverted is timeout based liked all the expectations, unfortunately, and enforcing order doesn't make sense. So it will fail immediately if fulfilled, but then will need to wait for the timeout to succeed

[self waitForExpectations:@[inactiveExpectation, resumedExpectation] timeout:5.0 enforceOrder:YES]; [self waitForExpectations:@[invertedExpectation] timeout:1.0]; // inactiveExpectation, resumedExpectation already fulfilled so wait less time?

However, what I wrote above should fail with "inactive, paused, inactive, resume, inactive, resume"
as
[inactive silently fulfilled]
FAIL: Unexpected lifecycle transition: paused

If you want to also fail on inactive, resume, inactive, resume, you should also set the expectations' assertForOverFulfill to YES. And if you expect the fulfillment to be more than one, you can also set expectedFulfillmentCount.

I was halfway changing it to the way you said, but then I noticed that they both currently do the same thing. They both assert order, check on the same granularity and since NSNotificationCenter posts are synchronous, they also fail on unexpected events at the time of unexpected events.

I think I still prefer the present way which at places lists out the total map of expected behavior.

Here are the new tests after the fix https://github.com/xster/engine/compare/lifecycle-test...xster:fix-ios-lifecycle?expand=1

jmagman · 2019-09-09T21:27:07Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+              initWithDescription:@"Current implementation sends another AppLifecycleState event"]];
+  [[NSNotificationCenter defaultCenter]
+      postNotificationName:UIApplicationWillResignActiveNotification
+                    object:nil];


This test is a bit confusing. Can you split it into a test per notification with the expectation order enforced instead of the array checks of the messages?

I'm going to be testing that there isn't an expectation per notification soon.

It would still make sense to assert the expectations between notifications? Can you break it down into separate tests at all?

I still think this is hard to read and it doesn't actually verify that the expectation was hit because of the notification, because it doesn't wait for the expectations between notifications. If this test ever fails because something is out of order it's going to be tricky to know which notification went awry and difficult to debug.

NSNotificationCenter posts are synchronous, and I'm removing these expectations right after in the next PR.

Well you would set up the expectation, post, then wait. We can talk about it in the next PR.

jmagman · 2019-09-09T21:42:02Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+
+#import <Flutter/Flutter.h>
+#import <XCTest/XCTest.h>
+#import "../Scenarios/ScreenBeforeFlutter.h"


You can just use a project import here.

#import "ScreenBeforeFlutter.h"

Ummm weird. Not sure why I was running into errors trying it before. Done.

jmagman · 2019-09-09T21:43:21Z

If there isn't a pressing reason to use Swift, I'd rather stick to Objective-C. While Dan's point of needing to be language polyglot as being a potential hinderance to future contributions is certainly valid, I am also concerned that the usage of Swift might make it harder to integrate this target into the GN so that we can build all engine targets in one go.

Once Swift in GN is figured out, a good unit test with minimum language-polyglot overhead would be one that imports the Flutter module to make sure it can be imported in Swift.

import Flutter

class SwiftModuleTests: XCTestCase {
    func testModuleImport() {
        // This test will fail to compile if the Swift module cannot be imported.
    }
}

xster · 2019-09-10T00:30:57Z

Thanks for reviewing

jmagman · 2019-09-10T03:01:03Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+  // Expected sequence from showing the FlutterViewController is inactive and resumed.
+  BOOL lifecyclesAsExpected = [lifecycleEvents
+      isEqualToArray:@[ @"AppLifecycleState.inactive", @"AppLifecycleState.resumed" ]];
+  XCTAssert(lifecyclesAsExpected,


XCTAssertEqualObjects

dnfield

Few nits on the Dart code. I haven't looked at the ObjC too closely since it seems like @jmagman has that covered and probably is finding far more than I would have :)

dnfield · 2019-09-10T15:51:13Z

testing/scenario_app/lib/src/platform_echo_mixin.dart

+  ) {
+    window.sendPlatformMessage(name, data, null);
+  }
+}


nit: new line at EOF.

dnfield · 2019-09-10T15:52:25Z

testing/scenario_app/lib/src/poppable_screen.dart

+    canvas.drawRect(
+      Rect.fromLTWH(0, 0, window.physicalSize.width - 1, window.physicalSize.height - 1),
+      Paint()..color = const Color.fromARGB(255, 255, 255, 255),
+    );


Maybe just canvas.drawPaint?

oh cool, never noticed this API. Neat

dnfield · 2019-09-10T15:53:47Z

testing/scenario_app/lib/src/poppable_screen.dart

+
+  @override
+  void onDrawFrame() {
+    window.scheduleFrame();


I think you might want this to go to the end of onMetricsChanged and _pop. Right now this is drawing the same frames over and over again even if nothing has changed.

ya, makes sense

jmagman · 2019-09-11T22:04:29Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

+              initWithDescription:@"Current implementation sends another AppLifecycleState event"]];
+  [[NSNotificationCenter defaultCenter]
+      postNotificationName:UIApplicationWillResignActiveNotification
+                    object:nil];


Well you would set up the expectation, post, then wait. We can talk about it in the next PR.

This reverts commit 46ff053 as it breaks mac tests https://ci.chromium.org/p/flutter/builders/prod/Mac%20Host%20Engine/2156.

This reverts commit 850d80c.

xster requested review from dnfield and jmagman September 5, 2019 03:43

googlebot added the cla: yes label Sep 5, 2019

xster mentioned this pull request Sep 6, 2019

Added new lifecycle enum #11913

Merged

xster force-pushed the lifecycle-test branch from 8971c8f to e51d95c Compare September 7, 2019 12:23

xster added the Work in progress (WIP) Not ready (yet) for review! label Sep 7, 2019

xster force-pushed the lifecycle-test branch from e51d95c to 5c8ca08 Compare September 9, 2019 20:47

xster removed the Work in progress (WIP) Not ready (yet) for review! label Sep 9, 2019

xster added the CQ+1 label Sep 9, 2019

jmagman suggested changes Sep 9, 2019

View reviewed changes

jmagman reviewed Sep 9, 2019

View reviewed changes

jmagman reviewed Sep 10, 2019

View reviewed changes

dnfield reviewed Sep 10, 2019

View reviewed changes

xster added 7 commits September 10, 2019 11:16

Add some lifecycle channel tests

e520394

debating between xctests and xcuitests

06e9f3f

more scenario scaffolding

4c677cd

fix draw frame crash

cef3a9f

debugging test

cff4afd

fix the tests

ac27cdd

get rid of the uitest stuff since I'm not doing it

3d01370

xster added 11 commits September 10, 2019 11:16

autoformat

bd4b352

small clean up

f56e347

add a doc comment

c86ee0f

move 2 swift files to objc

603dbc2

objective-c melts my brain too much to finish in one day

bc3174f

finish retranslating obj-c tests

83c6408

clean up some leftover crap

419c096

review

f9ee750

_nullable -> nullable

e24ce5d

use xctassertequalobjects

230fcce

review

8a404a5

xster force-pushed the lifecycle-test branch from dd62a78 to 8a404a5 Compare September 10, 2019 18:30

jmagman mentioned this pull request Sep 11, 2019

Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible #12128

Merged

jmagman approved these changes Sep 11, 2019

View reviewed changes

xster merged commit 46ff053 into flutter:master Sep 12, 2019

xster deleted the lifecycle-test branch September 12, 2019 23:36

aam added a commit to aam/engine that referenced this pull request Sep 13, 2019

Revert "Add some AppLifecycleTests (flutter#11890)"

3f4f391

This reverts commit 46ff053 as it breaks mac tests https://ci.chromium.org/p/flutter/builders/prod/Mac%20Host%20Engine/2156.

aam added a commit that referenced this pull request Sep 13, 2019

Revert "Add some AppLifecycleTests (#11890)" (#12264)

850d80c

This reverts commit 46ff053 as it breaks mac tests https://ci.chromium.org/p/flutter/builders/prod/Mac%20Host%20Engine/2156.

xster added a commit that referenced this pull request Sep 13, 2019

Revert "Revert "Add some AppLifecycleTests (#11890)" (#12264)"

fd98f48

This reverts commit 850d80c.

Add some AppLifecycleTests #11890

Add some AppLifecycleTests #11890

Uh oh!

Conversation

xster commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Sep 5, 2019

Uh oh!

xster commented Sep 5, 2019

Uh oh!

dnfield commented Sep 5, 2019

Uh oh!

chinmaygarde commented Sep 5, 2019

Uh oh!

xster commented Sep 6, 2019

Uh oh!

xster commented Sep 9, 2019

Uh oh!

xster commented Sep 9, 2019

Uh oh!

chinmaygarde commented Sep 9, 2019

Uh oh!

jmagman Sep 9, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 9, 2019 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Sep 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

xster commented Sep 5, 2019 •

edited

Loading

jmagman Sep 9, 2019 •

edited

Loading

jmagman Sep 9, 2019 •

edited

Loading

jmagman commented Sep 9, 2019 •

edited

Loading