From a6b29cc51c2def79a3a3fce12e6f2bce1733e4b5 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Tue, 31 Oct 2023 15:07:00 -0700 Subject: [PATCH] [macOS] Delete FlutterCompositor tests 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 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: https://github.com/flutter/flutter/issues/137648 Encountered these tests as part of deflaking and cleaning up memory allocations throughout the macOS desktop tests. Issue: https://github.com/flutter/flutter/issues/104789 Issue: https://github.com/flutter/flutter/issues/127441 Issue: https://github.com/flutter/flutter/issues/124840 --- ci/licenses_golden/licenses_flutter | 2 - shell/platform/darwin/macos/BUILD.gn | 1 - .../framework/Source/FlutterCompositor.h | 2 + .../framework/Source/FlutterCompositorTest.mm | 139 ------------------ 4 files changed, 2 insertions(+), 142 deletions(-) delete mode 100644 shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 9a6d66506bf68..7ed7a51cbc2db 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -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 @@ -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 diff --git a/shell/platform/darwin/macos/BUILD.gn b/shell/platform/darwin/macos/BUILD.gn index 56400d2a5b0d7..ea29942f406f8 100644 --- a/shell/platform/darwin/macos/BUILD.gn +++ b/shell/platform/darwin/macos/BUILD.gn @@ -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", diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h index ce22bb9f54916..555add5803a81 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h @@ -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. diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm deleted file mode 100644 index 081c3e22e86eb..0000000000000 --- a/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm +++ /dev/null @@ -1,139 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#import -#import -#import - -#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h" -#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h" -#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterViewProvider.h" -#import "flutter/testing/testing.h" - -@interface FlutterViewMockProvider : NSObject { - FlutterView* _implicitView; -} -/** - * Create a FlutterViewMockProvider with the provided view as the implicit view. - */ -- (nonnull instancetype)initWithImplicitView:(nonnull FlutterView*)view; -@end - -@implementation FlutterViewMockProvider - -- (nonnull instancetype)initWithImplicitView:(nonnull FlutterView*)view { - self = [super init]; - if (self != nil) { - _implicitView = view; - } - return self; -} - -- (nullable FlutterView*)viewForId:(FlutterViewId)viewId { - if (viewId == kFlutterImplicitViewId) { - return _implicitView; - } - return nil; -} - -@end - -namespace flutter::testing { -namespace { - -typedef void (^PresentBlock)(NSArray*); - -id MockViewProvider(PresentBlock onPresent = nil) { - FlutterView* viewMock = OCMClassMock([FlutterView class]); - FlutterSurfaceManager* surfaceManagerMock = OCMClassMock([FlutterSurfaceManager class]); - FlutterSurface* surfaceMock = OCMClassMock([FlutterSurface class]); - __block id textureMock = OCMProtocolMock(@protocol(MTLTexture)); - - OCMStub([viewMock surfaceManager]).andReturn(surfaceManagerMock); - - OCMStub([surfaceManagerMock surfaceForSize:CGSize{}]) - .ignoringNonObjectArgs() - .andDo(^(NSInvocation* invocation) { - CGSize size; - [invocation getArgument:&size atIndex:2]; - OCMStub([textureMock width]).andReturn(size.width); - OCMStub([textureMock height]).andReturn(size.height); - }) - .andReturn(surfaceMock); - - FlutterMetalTexture texture = { - .struct_size = sizeof(FlutterMetalTexture), - .texture_id = 1, - .texture = (__bridge void*)textureMock, - .user_data = (__bridge void*)surfaceMock, - .destruction_callback = nullptr, - }; - - OCMStub([surfaceManagerMock present:[OCMArg any] notify:[OCMArg any]]) - .andDo(^(NSInvocation* invocation) { - NSArray* info; - [invocation getArgument:&info atIndex:2]; - if (onPresent != nil) { - onPresent(info); - } - }); - - OCMStub([surfaceMock asFlutterMetalTexture]).andReturn(texture); - - return [[FlutterViewMockProvider alloc] initWithImplicitView:viewMock]; -} -} // namespace - -TEST(FlutterCompositorTest, TestCreate) { - std::unique_ptr macos_compositor = - std::make_unique(MockViewProvider(), - /*platform_view_controller*/ nullptr); - - FlutterBackingStore backing_store; - FlutterBackingStoreConfig config; - config.struct_size = sizeof(FlutterBackingStoreConfig); - config.size.width = 800; - config.size.height = 600; - macos_compositor->CreateBackingStore(&config, &backing_store); - - ASSERT_EQ(backing_store.type, kFlutterBackingStoreTypeMetal); - ASSERT_NE(backing_store.metal.texture.texture, nil); - id texture = (__bridge id)backing_store.metal.texture.texture; - ASSERT_EQ(texture.width, 800ul); - ASSERT_EQ(texture.height, 600ul); -} - -TEST(FlutterCompositorTest, TestPresent) { - __block NSArray* presentedSurfaces = nil; - - auto onPresent = ^(NSArray* info) { - presentedSurfaces = info; - }; - - std::unique_ptr macos_compositor = - std::make_unique(MockViewProvider(onPresent), - /*platform_view_controller*/ nullptr); - - FlutterBackingStore backing_store; - FlutterBackingStoreConfig config; - config.struct_size = sizeof(FlutterBackingStoreConfig); - config.size.width = 800; - config.size.height = 600; - macos_compositor->CreateBackingStore(&config, &backing_store); - - FlutterLayer layers[] = {{ - .struct_size = sizeof(FlutterLayer), - .type = kFlutterLayerContentTypeBackingStore, - .backing_store = &backing_store, - .offset = {0, 0}, - .size = {800, 600}, - }}; - const FlutterLayer* layers_ptr = layers; - - macos_compositor->Present(kFlutterImplicitViewId, &layers_ptr, 1); - - ASSERT_EQ(presentedSurfaces.count, 1ul); -} - -} // namespace flutter::testing