Skip to content

Commit

Permalink
[macOS] Clean up resources in ViewController tests (#47792)
Browse files Browse the repository at this point in the history
Wraps all FlutterViewController tests in an autorelease pool to ensure
resources are cleaned up.

Adds a MockFlutterEngineTest subclass of AutoreleasePoolTest that
creates an OCPartialMock FlutterEngine and shuts it down at the end of
the test. Previously we were not shutting down any FlutterEngine
instances we allocated, resulting in potentially thousands of threads
and graphics contexts being allocated.

Prior to this change, running these tests via:

    ../out/host_debug_unopt_arm64/flutter_desktop_darwin_unittests \
        --gtest_filter='FlutterViewController.*' --gtest_repeat=1000

resulted in test failures and sometimes segfaults. This ensures
resources are cleaned up

Eventually all unit tests should configure their FlutterEngine via
either FlutterEngineTest (which should be an AutoreleasePoolTest) or
MockFlutterEngineTest, and the CreateMockFlutterEngine function moved to
a static used in the implementation of these.

Issue: flutter/flutter#104789
Issue: flutter/flutter#127441
Issue: flutter/flutter#124840

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
cbracken authored Nov 8, 2023
1 parent 692f131 commit e687d57
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"

#import <OCMock/OCMock.h>

#include "flutter/testing/autoreleasepool_test.h"
#include "flutter/testing/test_dart_native_resolver.h"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -36,6 +38,30 @@ class FlutterEngineTest : public ::testing::Test {

// Returns a mock FlutterEngine that is able to work in environments
// without a real pasteboard.
//
// Callers MUST call [mockEngine shutDownEngine] when finished with the returned engine.
id CreateMockFlutterEngine(NSString* pasteboardString);

class MockFlutterEngineTest : public AutoreleasePoolTest {
public:
MockFlutterEngineTest();

void SetUp() override;
void TearDown() override;

id GetMockEngine() { return engine_mock_; }

void ShutDownEngine();

~MockFlutterEngineTest() {
[engine_mock_ shutDownEngine];
[engine_mock_ stopMocking];
}

private:
id engine_mock_;

FML_DISALLOW_COPY_AND_ASSIGN(MockFlutterEngineTest);
};

} // namespace flutter::testing
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,21 @@ id CreateMockFlutterEngine(NSString* pasteboardString) {
}
}

MockFlutterEngineTest::MockFlutterEngineTest() = default;

void MockFlutterEngineTest::SetUp() {
engine_mock_ = CreateMockFlutterEngine(@"");
}

void MockFlutterEngineTest::TearDown() {
[engine_mock_ shutDownEngine];
[engine_mock_ stopMocking];
engine_mock_ = nil;
}

void MockFlutterEngineTest::ShutDownEngine() {
[engine_mock_ shutDownEngine];
engine_mock_ = nil;
}

} // namespace flutter::testing
Loading

0 comments on commit e687d57

Please sign in to comment.