hasStrings Mac #20531

justinmc · 2020-08-15T00:10:30Z

Description

Adding the hasStrings method to Mac, similar to #19859.

Related Issues

Depends on flutter/buildroot#399
#19859
flutter/flutter#60145

Tests

I added the following tests:

hasStrings returns true after setting a string to the clipboard.
hasStrings returns false after clearing the clipboard.

justinmc · 2020-08-27T21:11:52Z

What's the status of unit testing on Mac?
I see one unit test, and it doesn't seem to be run by run_tests.py: https://github.com/flutter/engine/blob/master/shell/platform/darwin/macos/framework/Source/FlutterEngineUnittests.mm

flutter-dashboard · 2020-08-27T21:27:53Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke

I think you forgot to add the FlutterViewControllerTest.mm file?

justinmc · 2020-08-27T22:17:37Z

Whoops, actually I forgot to remove the line referencing it when I was messing around with trying to figure out the Mac unit tests.

Do we actually have functioning unit tests on Mac? If so, how do I run them?

gaaclarke · 2020-08-27T22:27:34Z

Do we actually have functioning unit tests on Mac? If so, how do I run them?

Never looked into it. @stuartmorgan would be the person to ask.

justinmc · 2020-08-27T22:29:51Z

Thanks for the pointer! I couldn't figure this out and didn't know who to ask.

stuartmorgan-g · 2020-08-28T14:08:39Z

The current state of testing for macOS is not good, certainly. @chinmaygarde added that test so could answer questions about it, or I can look next week when I'm not OOO

justinmc · 2020-08-31T17:11:20Z

@stuartmorgan Thanks for the pointer, I was able to get the tests running with help from Chinmay. I will attempt to add the Mac unit tests into run_tests.py in this PR.

For posterity, the way to directly run the Mac unit tests is to compile for Mac, then run:

./out/host_debug_unopt/flutter_desktop_darwin_unittests

justinmc · 2020-08-31T21:31:27Z

@gaaclarke Ready for re-review. I was able to add a test and get the Mac unit tests running as a part of run_tests.py.

stuartmorgan-g · 2020-08-31T23:04:17Z

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

+
+namespace flutter::testing {
+
+TEST(FlutterViewControllerTest, MacOSTestTest) {


Why is this called MacOSTestTest? The name should describe what the test is covering.

Thanks, forgot to update that.

stuartmorgan-g · 2020-08-31T23:09:57Z

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

+      [FlutterMethodCall methodCallWithMethodName:@"Clipboard.hasStrings" arguments:nil];
+  [viewController handleMethodCall:methodCall result:result];
+  ASSERT_TRUE(called);
+  ASSERT_TRUE(value);


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.

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.

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.

justinmc · 2020-09-08T23:15:52Z

Ah you're right, thanks! Running in profile mode fails with the same error locally.

If you've got any idea how to get this working let me know. The original FlutterEngineUnittests.mm also fails in profile mode.

stuartmorgan-g · 2020-09-08T23:38:10Z

If you've got any idea how to get this working let me know. The original FlutterEngineUnittests.mm also fails in profile mode.

I'm not familiar with GetFixturesPath() and the associated scaffolding, but from a quick look I don't see any evidence that it's even been used with the iOS embedding either, so it may need non-trivial retooling to create AOT snapshots in the format macOS and iOS expect them; I'd have to dig deeper into it than I have time for at the moment to be sure. I'm also not sure that's a good use of effort at this point since for the desktop embedding tests in general I'd like to move toward stubbing out at the embedder.h layer (although we'll probably want some more full-stack tests for which GetFixturesPath() could still be useful).

At this point I think the best option for this PR would be to revert the addition of the test binary to run_tests.py. Needing to get it running on CI is a pre-existing issue, and you've added tests (and run them locally) covering your changes that are ready to come online when they are made to work on CI, so the testing situation has still improved incrementally as a result of the PR even if it's not at the ideal outcome yet.

stuartmorgan-g · 2020-09-08T23:40:15Z

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

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


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.

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.

FlutterViewController_Internal.h

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'

Did you include that header in the test file?

That was it, thanks.

stuartmorgan-g · 2020-09-08T23:44:04Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

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


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.

Sounds good, removed.

stuartmorgan-g · 2020-09-08T23:44:45Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

@@ -517,7 +528,7 @@ - (void)playSystemSound:(NSString*)soundType {
 }

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


Prefer accessing properties with dot syntax: self.pasteboard.

stuartmorgan-g · 2020-09-08T23:48:02Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

    [pasteboard setString:text forType:NSPasteboardTypeString];
  }
 }

+- (BOOL)clipboardHasStrings {
+  NSDictionary* data = [self getClipboardData:[NSString stringWithFormat:@"%s", kTextPlainFormat]];
+  NSString* string = data[@"text"];


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;?

Good point, done.

stuartmorgan-g · 2020-09-08T23:50:06Z

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

+TEST(FlutterViewControllerTest, HasStringsWhenPasteboardEmpty) {
+  id viewControllerMock = mockViewController();
+
+  // Call setData to make sure that the pasteboard is empty.


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.

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

stuartmorgan-g · 2020-09-08T23:51:09Z

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm

+TEST(FlutterViewControllerTest, HasStringsWhenPasteboardFull) {
+  id viewControllerMock = mockViewController();
+
+  // Call setClipboardData to make sure there's a string on the pasteboard.


justinmc · 2020-09-09T18:38:31Z

@stuartmorgan That sounds good to me, I've reverted the change to run_tests.py. I'm happy we were able to improve the testing situation a bit even if not quite all the way.

justinmc · 2020-09-11T18:19:14Z

Waiting on #21111 to fix the Mac iOS Engine test.

…is gone

justinmc · 2020-09-12T01:13:41Z

Finally green 🎉

@stuartmorgan Ready for final review when we're back on Monday.

justinmc · 2020-09-15T00:23:41Z

@stuartmorgan Reminder that this needs a final review when you have a chance.

Gets ocmock set up so that it can be used by the Mac unit tests (in flutter/engine#20531).

justinmc added 2 commits August 14, 2020 15:44

hasStrings on mac

bb0f4be

WIP test

97acb46

justinmc added the Work in progress (WIP) Not ready (yet) for review! label Aug 15, 2020

justinmc self-assigned this Aug 15, 2020

justinmc marked this pull request as draft August 15, 2020 00:10

googlebot added the cla: yes label Aug 15, 2020

auto-assign bot requested a review from iskakaushik August 15, 2020 00:12

justinmc mentioned this pull request Aug 24, 2020

iOS hasStrings and the clipboard notification flutter/flutter#61774

Closed

Clean up and remove test since unit testing on mac not set up?

660092e

justinmc force-pushed the has-strings-mac branch from bce804f to 660092e Compare August 27, 2020 21:27

justinmc changed the title ~~(WIP) hasStrings Mac~~ hasStrings Mac Aug 27, 2020

justinmc removed the Work in progress (WIP) Not ready (yet) for review! label Aug 27, 2020

justinmc marked this pull request as ready for review August 27, 2020 21:27

justinmc requested review from LongCatIsLooong and gaaclarke August 27, 2020 21:28

gaaclarke reviewed Aug 27, 2020

View reviewed changes

justinmc added 5 commits August 31, 2020 11:23

Test hasStrings

ffc7af4

Test hasStrings after clearing the clipboard as well

98adfc9

Run Mac unit tests in run_tests.py

59ad159

Formatting

084c926

Build.gn formatting

1a22b66

stuartmorgan-g reviewed Aug 31, 2020

View reviewed changes

stuartmorgan-g suggested changes Sep 8, 2020

View reviewed changes

justinmc added 3 commits September 9, 2020 09:15

_pasteboard -> pasteboard

3c9ec6e

Mock pasteboard value directly instead of mocking setString

a52595d

Don't run the mac tests with run_test.py, because they fail in AOT mode

d14a117

justinmc added 3 commits September 9, 2020 13:07

Merge branch 'master' into has-strings-mac

3490d8a

pasteboard in the private header

e1566ac

Merge branch 'master' into has-strings-mac

491a585

This was referenced Sep 11, 2020

Fix OCMock static_library target flutter/buildroot#402

Merged

Update OCMock to version 3.7.1 #21111

Merged

cbracken mentioned this pull request Sep 11, 2020

Re-split ocmock into source set and static lib flutter/buildroot#403

Merged

justinmc added 2 commits September 11, 2020 16:27

Merge branch 'master' into has-strings-mac

8c2d240

Use ocmock now that it has been made to work with mac and ocmock_src …

06a6034

…is gone

stuartmorgan-g approved these changes Sep 15, 2020

View reviewed changes

justinmc merged commit 3c9308f into flutter:master Sep 15, 2020

justinmc deleted the has-strings-mac branch September 15, 2020 15:40

engine-flutter-autoroll mentioned this pull request Sep 15, 2020

Roll Engine from c6121c5d515c to 3c9308faad0b (4 revisions) flutter/flutter#65870

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 15, 2020

3c9308f hasStrings Mac (flutter/engine#20531)

9e1b1c3

justinmc mentioned this pull request Sep 24, 2020

hasStrings Linux #21388

Merged

gspencergoog mentioned this pull request Sep 25, 2020

flutter_desktop_darwin_unittests fails in AOT modes (profile and release) flutter/flutter#66664

Closed

justinmc mentioned this pull request Sep 25, 2020

Unsolicited iOS14 clipboard permission prompt flutter/flutter#60145

Closed

gspencergoog mentioned this pull request Sep 25, 2020

Enable delayed event delivery for macOS #21231

Merged

1 task

chinmaygarde pushed a commit to chinmaygarde/flutter_buildroot that referenced this pull request May 4, 2021

OCMock for Mac tests (flutter#399)

d25e590

Gets ocmock set up so that it can be used by the Mac unit tests (in flutter/engine#20531).

justinmc mentioned this pull request Jul 27, 2021

hasStrings on Windows #27749

Merged

justinmc mentioned this pull request Aug 4, 2021

hasStrings support for eliminating clipboard notifications flutter/flutter#87678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hasStrings Mac #20531

hasStrings Mac #20531

justinmc commented Aug 15, 2020 •

edited

Loading

justinmc commented Aug 27, 2020

flutter-dashboard bot commented Aug 27, 2020

gaaclarke left a comment

justinmc commented Aug 27, 2020

gaaclarke commented Aug 27, 2020

justinmc commented Aug 27, 2020

stuartmorgan-g commented Aug 28, 2020 •

edited

Loading

justinmc commented Aug 31, 2020

justinmc commented Aug 31, 2020

stuartmorgan-g Aug 31, 2020

justinmc Sep 1, 2020

stuartmorgan-g Aug 31, 2020

justinmc Sep 1, 2020

stuartmorgan-g Sep 1, 2020

justinmc commented Sep 8, 2020

stuartmorgan-g commented Sep 8, 2020

stuartmorgan-g Sep 8, 2020

justinmc Sep 9, 2020

stuartmorgan-g Sep 9, 2020

justinmc Sep 9, 2020

stuartmorgan-g Sep 10, 2020

justinmc Sep 10, 2020

stuartmorgan-g Sep 8, 2020

justinmc Sep 9, 2020

stuartmorgan-g Sep 8, 2020

stuartmorgan-g Sep 8, 2020

justinmc Sep 9, 2020

stuartmorgan-g Sep 8, 2020

justinmc Sep 9, 2020

stuartmorgan-g Sep 8, 2020

justinmc commented Sep 9, 2020

justinmc commented Sep 11, 2020

justinmc commented Sep 12, 2020

justinmc commented Sep 15, 2020


		namespace flutter::testing {

		TEST(FlutterViewControllerTest, MacOSTestTest) {

hasStrings Mac #20531

hasStrings Mac #20531

Conversation

justinmc commented Aug 15, 2020 • edited Loading

Description

Related Issues

Tests

justinmc commented Aug 27, 2020

flutter-dashboard bot commented Aug 27, 2020

gaaclarke left a comment

Choose a reason for hiding this comment

justinmc commented Aug 27, 2020

gaaclarke commented Aug 27, 2020

justinmc commented Aug 27, 2020

stuartmorgan-g commented Aug 28, 2020 • edited Loading

justinmc commented Aug 31, 2020

justinmc commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc commented Sep 8, 2020

stuartmorgan-g commented Sep 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc commented Sep 9, 2020

justinmc commented Sep 11, 2020

justinmc commented Sep 12, 2020

justinmc commented Sep 15, 2020

justinmc commented Aug 15, 2020 •

edited

Loading

stuartmorgan-g commented Aug 28, 2020 •

edited

Loading