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 22 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
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ allowed_hosts = [
]

deps = {
'src': 'https://github.com/flutter/buildroot.git' + '@' + 'a6c0959d1ac8cdfe6f9ff87892bc4905a73699fe',
'src': 'https://github.com/flutter/buildroot.git' + '@' + '940dcf6e85301c049576da58b193cf2d903539a2',

# Fuchsia compatibility
#
Expand Down
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,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 @@ -39,6 +39,11 @@ FLUTTER_EXPORT
*/
@property(nonatomic) FlutterMouseTrackingMode mouseTrackingMode;

/**
* This just returns the NSPasteboard so that it can be mocked in the tests.
*/
@property(nonatomic, readonly, nonnull) NSPasteboard* _pasteboard;
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be moved to the private header.

Also, a property name should not start with _; the property and the ivar are distinct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where exactly should this be moved to? Sorry for my lack of Objective C knowledge. I tried moving this inside the .mm file, but then I couldn't mock it from the test.

👍 About the underscore.

Copy link
Contributor

Choose a reason for hiding this comment

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

FlutterViewController_Internal.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, I missed that.

Though it seems to also give me errors when the test tries to mock it. I just copied this exactly over to the _Internal header. Is there something else I need to do to make it accessible to the test?

FlutterViewControllerTest.mm:32:31: error: no known instance method for selector 'pasteboard'

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you include that header in the test file?

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 was it, thanks.


/**
* Initializes a controller that will run the given project.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,15 @@ - (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.

I guess I didn't explain my earlier question very well: what information is this adding that helps understand the method? The description in the comment seems clear and unambiguous; following that link doesn't tell me anything new AFAICT, so why have it?

It also seems odd to document it in terms of another platform's implementation, given than a) we have a lot of platforms, so referring to one of them is pretty arbitrary, b) that implementation is not relevant here since there's no such method on macOS, and c) that anyone who wants to know how it's done on iOS can easily just go look at the iOS source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, removed.

* which is the equivalent method that Flutter utilizes in iOS.
*/
- (BOOL)clipboardHasStrings;

@end

#pragma mark - FlutterViewController implementation.
Expand Down Expand Up @@ -505,6 +514,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(@{@"value" : @([self clipboardHasStrings])});
} else {
result(FlutterMethodNotImplemented);
}
Expand All @@ -517,7 +528,7 @@ - (void)playSystemSound:(NSString*)soundType {
}

- (NSDictionary*)getClipboardData:(NSString*)format {
NSPasteboard* pasteboard = [NSPasteboard generalPasteboard];
NSPasteboard* pasteboard = [self _pasteboard];
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer accessing properties with dot syntax: self.pasteboard.

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

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

- (BOOL)clipboardHasStrings {
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.

return string.length > 0;
}

- (NSPasteboard*)_pasteboard {
return [NSPasteboard generalPasteboard];
}

#pragma mark - FlutterViewReshapeListener

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// 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 "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/Headers/FlutterEngine.h"
#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"
#import "third_party/ocmock/Source/OCMock/OCMock.h"

namespace flutter::testing {

// Returns a mock FlutterViewController that is able to work in environments
// without a real pasteboard.
id mockViewController() {
NSString* fixtures = @(testing::GetFixturesPath());
FlutterDartProject* project = [[FlutterDartProject alloc]
initWithAssetsPath:fixtures
ICUDataPath:[fixtures stringByAppendingString:@"/icudtl.dat"]];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];

// Mock _pasteboard so that this test will work in environments without a
// real pasteboard.
id pasteboardMock = OCMClassMock([NSPasteboard class]);
__block NSString* clipboardString = @"";
OCMStub([pasteboardMock setString:[OCMArg any] forType:[OCMArg any]])
.andDo(^(NSInvocation* invocation) {
[invocation getArgument:&clipboardString atIndex:2];
});
OCMExpect([pasteboardMock stringForType:[OCMArg any]]).andDo(^(NSInvocation* invocation) {
[invocation setReturnValue:&clipboardString];
});
id viewControllerMock = OCMPartialMock(viewController);
OCMStub([viewControllerMock _pasteboard]).andReturn(pasteboardMock);
return viewControllerMock;
}

TEST(FlutterViewControllerTest, HasStringsWhenPasteboardEmpty) {
id viewControllerMock = mockViewController();

// Call setData to make sure that the pasteboard is empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Your test controls the fake clipboard; there is no reason to involve Clipboard.setData in the test of Clipboard.hasStrings.

It would be good to have a test of setData, but that should be a separate 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.

Done, now I just set up the mock with some given pasteboard string instead of letting the test call setString.

__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.


TEST(FlutterViewControllerTest, HasStringsWhenPasteboardFull) {
id viewControllerMock = mockViewController();

// Call setClipboardData to make sure there's a string on the pasteboard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

__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);
}

} // flutter::testing
1 change: 1 addition & 0 deletions testing/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,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