Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
bb0f4be
hasStrings on mac
justinmc Aug 14, 2020
97acb46
WIP test
justinmc Aug 15, 2020
660092e
Clean up and remove test since unit testing on mac not set up?
justinmc Aug 27, 2020
ffc7af4
Test hasStrings
justinmc Aug 31, 2020
98adfc9
Test hasStrings after clearing the clipboard as well
justinmc Aug 31, 2020
59ad159
Run Mac unit tests in run_tests.py
justinmc Aug 31, 2020
084c926
Formatting
justinmc Aug 31, 2020
1a22b66
Build.gn formatting
justinmc Aug 31, 2020
67f0173
Test name
justinmc Sep 1, 2020
6177e74
WIP trying to get ocmock to work in Mac
justinmc Sep 2, 2020
24d28c8
Use new ocmock target coming in buildroot PR
justinmc Sep 3, 2020
51280f2
Mocked pasteboard working
justinmc Sep 3, 2020
a15798e
handleMethodCall removed from header and pasteboard moved to property…
justinmc Sep 3, 2020
878aa78
Docs clarification
justinmc Sep 3, 2020
c2bca18
clipboardHasStrings returns a bool, caller handles dictionary
justinmc Sep 3, 2020
54301f3
Split tests and some cleanup
justinmc Sep 3, 2020
70286fc
Deduplicate mocking code
justinmc Sep 3, 2020
6c778fe
Comment cleanup
justinmc Sep 3, 2020
b68a594
Analyzer fixes
justinmc Sep 3, 2020
25385b5
Merge branch 'master' into has-strings-mac
justinmc Sep 3, 2020
45b81b4
Reference new buildroot hash after my change to ocmock there
justinmc Sep 4, 2020
2d0728d
Merge branch 'master' into has-strings-mac
justinmc Sep 8, 2020
3c9ec6e
_pasteboard -> pasteboard
justinmc Sep 9, 2020
a52595d
Mock pasteboard value directly instead of mocking setString
justinmc Sep 9, 2020
d14a117
Don't run the mac tests with run_test.py, because they fail in AOT mode
justinmc Sep 9, 2020
3490d8a
Merge branch 'master' into has-strings-mac
justinmc Sep 9, 2020
e1566ac
pasteboard in the private header
justinmc Sep 10, 2020
491a585
Merge branch 'master' into has-strings-mac
justinmc Sep 11, 2020
8c2d240
Merge branch 'master' into has-strings-mac
justinmc Sep 11, 2020
06a6034
Use ocmock now that it has been made to work with mac and ocmock_src …
justinmc Sep 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,7 @@ FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterTextI
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterView.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/fixtures/flutter_desktop_test.dart
FILE: ../../../flutter/shell/platform/darwin/macos/framework/module.modulemap
Expand Down
6 changes: 5 additions & 1 deletion shell/platform/darwin/macos/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ test_fixtures("flutter_desktop_darwin_fixtures") {
executable("flutter_desktop_darwin_unittests") {
testonly = true

sources = [ "framework/Source/FlutterEngineUnittests.mm" ]
sources = [
"framework/Source/FlutterEngineUnittests.mm",
"framework/Source/FlutterViewControllerTest.mm",
]

cflags_objcc = [ "-fobjc-arc" ]

Expand All @@ -114,6 +117,7 @@ executable("flutter_desktop_darwin_unittests") {
"//flutter/testing:dart",
"//flutter/testing:skia",
"//flutter/testing:testing_lib",
"//third_party/ocmock:ocmock_src",
]
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,8 @@ FLUTTER_EXPORT
NS_DESIGNATED_INITIALIZER;
- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)nibNameOrNil NS_DESIGNATED_INITIALIZER;

- (void)handleMethodCall:(nonnull FlutterMethodCall*)call result:(nonnull FlutterResult)result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you making this public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I thought this was necessary to call it from the test, but it seems to work without it. I'll remove it.


- (nonnull NSPasteboard*)getPasteboard;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is non idiomatic objective-c, you don't use "getX" in objective c, you use properties, ex

@property(nonatomic, readonly, nonnull) NSPasteBoard* _pasteboard;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! This way seems cleaner and is just as mockable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding new public API to FVC to implement an internal channel? Unless there's a specific need for this to be public, it should be in the private header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now have this as a property like the comment above recommends. Let me know if how I did the property and getter doesn't look right. I just need to be able to mock it in the tests.

Excuse my lack of Objective C knowledge!


@end
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ - (NSDictionary*)getClipboardData:(NSString*)format;
*/
- (void)setClipboardData:(NSDictionary*)data;

/**
* Returns true iff the clipboard contains nonempty string data.
*
* See also:
* * https://developer.apple.com/documentation/uikit/uipasteboard/1829416-hasstrings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you referencing UIKit documentation in the macOS embedding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no equivalent hasStrings method for Mac. I'll add some details in the comment.

*/
- (NSDictionary*)clipboardHasStrings;

@end

#pragma mark - FlutterViewController implementation.
Expand Down Expand Up @@ -505,6 +513,8 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
} else if ([call.method isEqualToString:@"Clipboard.setData"]) {
[self setClipboardData:call.arguments];
result(nil);
} else if ([call.method isEqualToString:@"Clipboard.hasStrings"]) {
result([self clipboardHasStrings]);
} else {
result(FlutterMethodNotImplemented);
}
Expand All @@ -517,7 +527,7 @@ - (void)playSystemSound:(NSString*)soundType {
}

- (NSDictionary*)getClipboardData:(NSString*)format {
NSPasteboard* pasteboard = [NSPasteboard generalPasteboard];
NSPasteboard* pasteboard = [self getPasteboard];
if ([format isEqualToString:@(kTextPlainFormat)]) {
NSString* stringInPasteboard = [pasteboard stringForType:NSPasteboardTypeString];
return stringInPasteboard == nil ? nil : @{@"text" : stringInPasteboard};
Expand All @@ -526,14 +536,26 @@ - (NSDictionary*)getClipboardData:(NSString*)format {
}

- (void)setClipboardData:(NSDictionary*)data {
NSPasteboard* pasteboard = [NSPasteboard generalPasteboard];
NSPasteboard* pasteboard = [self getPasteboard];
NSString* text = data[@"text"];
[pasteboard clearContents];
if (text && ![text isEqual:[NSNull null]]) {
[pasteboard clearContents];
[pasteboard setString:text forType:NSPasteboardTypeString];
}
}

- (NSDictionary*)clipboardHasStrings {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's a bit odd having a method called clipboardHasStrings return something other than a bool. It is probably better to return bool and wrap it into a dictionary at the call site, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good call, the docs I wrote made this even more confusing actually.

NSDictionary* data = [self getClipboardData:[NSString stringWithFormat:@"%s", kTextPlainFormat]];
NSString* string = data[@"text"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more closely this time: going through a method whose input and output are explicitly designed for the Flutter method channel contract seems like a very convoluted way to check this. Why not just return [self.pasteboard stringForType:NSPasteboardTypeString]].length > 0;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

BOOL hasStrings = string.length > 0;
return @{@"value" : @(hasStrings)};
}

// This is a separate method to allow mocking the pasteboard in the tests.
- (NSPasteboard*)getPasteboard {
return [NSPasteboard generalPasteboard];
}

#pragma mark - FlutterViewReshapeListener

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// 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 "third_party/ocmock/Source/OCMock/OCMock.h"
#include "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"
#import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterViewController.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the primary header for the test, so should be first in its own block.

#include "flutter/shell/platform/darwin/macos/framework/Source/FlutterDartProject_Internal.h"
#include "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
#include "flutter/testing/testing.h"

namespace flutter::testing {

TEST(FlutterViewControllerTest, HasStrings) {
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];

// Mock getPasteboard so that this test will work in environments without a
// real pasteboard.
id pasteboardMock = OCMClassMock([NSPasteboard class]);
__block NSString* clipboardString = @"";
void (^setStringMock)(NSInvocation*) = ^(NSInvocation *invocation) {
[invocation getArgument:&clipboardString atIndex:2];
};
OCMStub([pasteboardMock setString:[OCMArg any] forType:[OCMArg any]]).andDo(setStringMock);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would be a bit easier to follow if you inlined all these blocks instead of declaring them then passing them as variables then using the variables.

void (^stringForTypeMock)(NSInvocation*) = ^(NSInvocation *invocation) {
[invocation setReturnValue:&clipboardString];
};
OCMExpect([pasteboardMock stringForType:[OCMArg any]]).andDo(stringForTypeMock);
id viewControllerMock = OCMPartialMock(viewController);
OCMStub([viewControllerMock getPasteboard]).andReturn(pasteboardMock);

// First call setClipboardData to put a string on the pasteboard.
__block bool calledSet = false;
FlutterResult resultSet = ^(id result) {
calledSet = true;
};
FlutterMethodCall* methodCallSet =
[FlutterMethodCall methodCallWithMethodName:@"Clipboard.setData"
arguments:@{@"text" : @"some string"}];
[viewControllerMock handleMethodCall:methodCallSet result:resultSet];
ASSERT_TRUE(calledSet);

// Call hasStrings and expect it to be true.
__block bool called = false;
__block bool value;
FlutterResult result = ^(id result) {
called = true;
NSNumber* valueNumber = [result valueForKey:@"value"];
value = [valueNumber boolValue];
};
FlutterMethodCall* methodCall =
[FlutterMethodCall methodCallWithMethodName:@"Clipboard.hasStrings" arguments:nil];
[viewControllerMock handleMethodCall:methodCall result:result];
ASSERT_TRUE(called);
ASSERT_TRUE(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the unit tests on the bots are running in a bootstrap context that doesn't have access to the pasteboard server. If that's the case, perhaps the tests should inject a mock NSPasteboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea how to include OCMock in the Mac tests? I tried including it in this test file and adding it to Build.gn in various places, but I keep getting compiler errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same way it's done for iOS? Without knowing what you tried and what exactly didn't work, it's hard to advise further.


// Now call setData again to clear the pasteboard.
__block bool calledSetClear = false;
FlutterResult resultSetClear = ^(id result) {
calledSetClear = true;
};
FlutterMethodCall* methodCallSetClear =
[FlutterMethodCall methodCallWithMethodName:@"Clipboard.setData"
arguments:@{@"text" : [NSNull null]}];
[viewControllerMock handleMethodCall:methodCallSetClear result:resultSetClear];
ASSERT_TRUE(calledSetClear);

// Call hasStrings and expect it to be false.
__block bool calledAfterClear = false;
__block bool valueAfterClear;
FlutterResult resultAfterClear = ^(id result) {
calledAfterClear = true;
NSNumber* valueNumber = [result valueForKey:@"value"];
valueAfterClear = [valueNumber boolValue];
};
FlutterMethodCall* methodCallAfterClear =
[FlutterMethodCall methodCallWithMethodName:@"Clipboard.hasStrings" arguments:nil];
[viewControllerMock handleMethodCall:methodCallAfterClear result:resultAfterClear];
ASSERT_TRUE(calledAfterClear);
ASSERT_FALSE(valueAfterClear);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is doing a lot of different things; please make individual tests that are more targeted. You're already using a fake pasteboard, so there's no need to do the entire cycle in a single test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I split it into two and it's a lot cleaner.


} // flutter::testing
1 change: 1 addition & 0 deletions testing/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ def RunCCTests(build_dir, filter):
# These unit-tests are Objective-C and can only run on Darwin.
if IsMac():
RunEngineExecutable(build_dir, 'flutter_channels_unittests', filter, shuffle_flags)
RunEngineExecutable(build_dir, 'flutter_desktop_darwin_unittests', filter, shuffle_flags)

# https://github.com/flutter/flutter/issues/36296
if IsLinux():
Expand Down