Skip to content

Commit

Permalink
[macOS] Delete FlutterCompositor tests (flutter#47527)
Browse files Browse the repository at this point in the history
The tests for FlutterCompositor are not useful. The current tests test two things:
1. That the mocks we set up behave the way we set them up to behave.
2. That the implementation of FlutterCompositor is the current implementation of FlutterCompositor.

As an example, consider FlutterCompositorTest.TestPresent: https://github.com/flutter/engine/blob/89e8de970cb99aa82e067bbdb4a8e927e53f0b28/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm#L107-L137

Ostensibly, this test verifies that the onPresent callback configured in our fake FlutterViewProvider implementation is called when FlutterCompositor::Present() is called.

However, taking a look at the mocking setup:
https://github.com/flutter/engine/blob/89e8de970cb99aa82e067bbdb4a8e927e53f0b28/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm#L47-L85

We do the following:
1. Mock out FlutterSurfaceManager such that when a surface is requested, we hand back a mock surface. A little gross since we're relying on some knowledge of implementation details of the compositor, but let's take this as reasonable for now.
2. We mock out `FlutterSurface asFlutterMetalTexture` to return a mock texture. Again, we're getting a bit deep into implementation details that the test shouldn't know about, but let's assume this gets us somewhere.
3. We mock out `FlutterSurfaceManager present:notify:` to actually call the `onPresent` callback if it's passed in.

In effect, we're testing that:
1. We configured our mock for `FlutterSurfaceManager present:notify:` to call onPresent.
2. That `FlutterCompositor::Present` actually calls `FlutterSurfaceManager present:notify:` despite that being a simple implementation detail of that call.

This removes these tests. I have filed the following issue to track refactoring this class for testability and adding tests: flutter/flutter#137648

Encountered these tests as part of deflaking and cleaning up memory allocations throughout the macOS desktop tests.

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

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
cbracken authored Nov 1, 2023
1 parent 43ac4ac commit 2a6f1a3
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 142 deletions.
2 changes: 0 additions & 2 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -4669,7 +4669,6 @@ ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCha
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterChannelKeyResponderTest.mm + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject.mm + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterEmbedderExternalTextureTest.mm + ../../../flutter/LICENSE
Expand Down Expand Up @@ -7473,7 +7472,6 @@ FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterChann
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterChannelKeyResponderTest.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterEmbedderExternalTextureTest.mm
Expand Down
1 change: 0 additions & 1 deletion shell/platform/darwin/macos/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ executable("flutter_desktop_darwin_unittests") {
"framework/Source/FlutterAppDelegateTest.mm",
"framework/Source/FlutterAppLifecycleDelegateTest.mm",
"framework/Source/FlutterChannelKeyResponderTest.mm",
"framework/Source/FlutterCompositorTest.mm",
"framework/Source/FlutterEmbedderExternalTextureTest.mm",
"framework/Source/FlutterEmbedderKeyResponderTest.mm",
"framework/Source/FlutterEngineTest.mm",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace flutter {
// FlutterCompositor creates and manages the backing stores used for
// rendering Flutter content and presents Flutter content and Platform views.
// Platform views are not yet supported.
//
// TODO(cbracken): refactor for testability. https://github.com/flutter/flutter/issues/137648
class FlutterCompositor {
public:
// Create a FlutterCompositor with a view provider.
Expand Down
139 changes: 0 additions & 139 deletions shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm

This file was deleted.

0 comments on commit 2a6f1a3

Please sign in to comment.