Clean up after AppLifecycleTests #12273
Conversation
| @@ -140,5 +140,13 @@ - (void)testLifecycleChannel { | |||
| XCTAssertEqualObjects( | |||
| lifecycleEvents, expectedStates, | |||
| @"AppLifecycleState transitions while presenting a second time not as expected"); | |||
|
|
|||
| // Dismantle. | |||
| [engine.lifecycleChannel setMessageHandler:nil]; | |||
There was a problem hiding this comment.
Does this belong in tearDown?
/cc @jmagman
There was a problem hiding this comment.
I'd have to move all their creations to setup if it was in tearDown
There was a problem hiding this comment.
more specifically, the message handler is the assertion mechanism in this test. It makes the test fail if I did dismiss the FlutterVC before disconnecting it. And I wouldn't create the FlutterVC in setup (because that's half of the test).
There was a problem hiding this comment.
Ahh I see, there's only one test in here anyway. I was assuming there were multiple tests in this file.
|
Ran 20 times. This should fix it |
|
Merging on red to un-red the tree. |
| // Dismantle. | ||
| [engine.lifecycleChannel setMessageHandler:nil]; | ||
| [flutterVC dismissViewControllerAnimated:NO completion:nil]; | ||
| flutterVC = nil; |
There was a problem hiding this comment.
flutterVC and rootVC shouldn't need to be set to nil, it should dealloc after this test returns? Is there some retain cycle that needs to be fixed?
What exactly was the problem here? Was state bleeding between tests?
There was a problem hiding this comment.
ya, setting them to nil was just voodooism. Removed in the next PR.
The real meat was getting rid of the assert closure on top which would have failed after the test ended if more things happened with the vc as it's dismissed. The VC is also held onto by the engine unless removed explicitly.
git@github.com:flutter/engine.git/compare/7d8c6d9c1a5c...e12decf git log 7d8c6d9..e12decf --no-merges --oneline 2019-09-13 xster@google.com Clean up after AppLifecycleTests (flutter/engine#12273) 2019-09-13 dnfield@google.com Close the tree (flutter/engine#12268) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
git@github.com:flutter/engine.git/compare/7d8c6d9c1a5c...e12decf git log 7d8c6d9..e12decf --no-merges --oneline 2019-09-13 xster@google.com Clean up after AppLifecycleTests (flutter/engine#12273) 2019-09-13 dnfield@google.com Close the tree (flutter/engine#12268) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
No description provided.