Made it so unit tests can be written against all ios engine code. by gaaclarke · Pull Request #17624 · flutter/engine

gaaclarke · 2020-04-10T01:49:27Z

This introduces a new shared_library that has all the contents of Fluttter.framework and the compiled unit tests. This is the library the test runner links against instead of Flutter.framework. That allows us to bypass the visibility problem since the tests are in the same binary as the code.

jmagman · 2020-04-10T20:03:33Z

shell/platform/darwin/ios/BUILD.gn

+  configs -= [ "//build/config/compiler:chromium_code" ]
+  cflags = [
+    "-fvisibility=default",
+    "-F/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform//Developer/Library/Frameworks/",


What if Xcode is at a different path?

What about the non-simulator frameworks?

This is still a draft PR =). I can address where xcode is located. I haven't been able to get rid of the hardcoded iPhoneSimulator path. The problem is that GN special cases all libs that are "X.framework" but it doesn't support XCTest.framework.

Since this is intended for the simulator it isn't the end of the world if I can't get it removed.

This is still a draft PR =).

Sorry, got excited!

xster

awesome!

xster · 2020-04-10T20:20:04Z

shell/platform/darwin/ios/BUILD.gn

-  public = _flutter_framework_headers
-
+source_set("flutter_framework_source") {
  cflags_objc = flutter_cflags_objc


we can add a non-public visibility clause here to be clean

xster · 2020-04-10T20:27:53Z

shell/platform/darwin/ios/BUILD.gn

+    "-Wno-misleading-indentation",
+  ]
+  sources = [
+    "../../../../../third_party/ocmock/Source/OCMock/NSInvocation+OCMAdditions.h",


I don't know if it works, but instead of digging through all these source, can't the dependent just have a deps = [ "//third_party/ocmock" ] here? (searching through other .gn file examples in third_party)

We can't because ocmock doesn't have a GN file that specifies how to build it. We can't add one there either because that isn't a repo we control. We can clone it at some point.

There is one though https://chromium.googlesource.com/chromium/src/+/master/third_party/ocmock/BUILD.gn. It's probably our DEPS needing to get it from chromium rather than the upstream github directly.

We can't use chromium's ocmock because it is a source_set not a static_library. I can try a static_library based on that source_set but i've had problems with static_libraries in the past. I'd prefer to do this as a separate PR if you don't mind.

ah ok, feel free to do it in a separate PR.

That source_set is default public visibility, I suppose we can still make a static_library here that just deps = [ "//third_party/ocmock" ] if we clone the right repo.

xster · 2020-04-10T20:28:46Z

testing/ios/IosUnitTests/build_and_run_tests.sh

 cd ../../../..
 ./flutter/tools/gn --ios --simulator --unoptimized
 ninja -j 100 -C out/ios_debug_sim_unopt
+ninja -j 100 -C out/ios_debug_sim_unopt ios_test_flutter


is this out/ios_test_flutter?

nope, we are piggy backing on ./flutter/tools/gn

xster · 2020-04-10T20:29:07Z

shell/platform/darwin/ios/BUILD.gn

+  include_dirs = [ "../../../../../third_party/ocmock/Source/" ]
+}
+
+ios_test_flutter_path = rebase_path("$root_out_dir/libios_test_flutter.dylib")


perhaps still mention that it's for simulators in the name

added comment

gaaclarke · 2020-04-10T20:37:54Z

This PR depends on LUCI updating: https://chromium-review.googlesource.com/c/chromium/tools/build/+/2145769

xster · 2020-04-10T21:59:20Z

shell/platform/darwin/ios/BUILD.gn


+ocmock_path = "../../../../../third_party/ocmock/Source"
+
+# TODO: Clone the OCMock repository so we can add a BUILD.gn to it.


It already is cloned and patched I think.

DEPS https://chromium.googlesource.com/chromium/src/+/master/third_party/ocmock/BUILD.gn and use like https://chromium.googlesource.com/chromium/src/+/master/ios/testing/BUILD.gn

as commented above

Added a todo issue.

cbracken · 2020-04-10T22:00:12Z

testing/ios/IosUnitTests/build_and_run_tests.sh

 cd ../../../..
-./flutter/tools/gn --ios --simulator --unoptimized
-ninja -j 100 -C out/ios_debug_sim_unopt
+ninja -j 100 -C out/$FLUTTER_ENGINE ios_test_flutter


If we're still building the engine here, is there a reason we're not running the gn step?
Given that $FLUTTER_ENGINE is now a variable, the gn params probably should be too.

Instead of it being something it does, it's a precondition now. I could pass the args through. There isn't really a need other than to help people. I added a check in the script with a helpful message.

just use autoninja here, so it knows if it should use goma or not.

cbracken · 2020-04-10T22:02:13Z

shell/platform/darwin/ios/BUILD.gn

+ios_test_flutter_path = rebase_path("$root_out_dir/libios_test_flutter.dylib")
+platform_frameworks_path = "$ios_sdk_path/../../Library/Frameworks/"
+
+# NOTE: This currently only support simulator targets because of the install_name.


ultra nit: s/support/supports/

xster · 2020-04-10T22:04:52Z

shell/platform/darwin/ios/BUILD.gn

+    "framework/Source/FlutterPluginAppLifeCycleDelegateTest.m",
+    "framework/Source/FlutterTextInputPluginTest.m",
+    "framework/Source/FlutterViewControllerTest.mm",
+    "framework/Source/SemanticsObjectTest.mm",


I wonder if there's anything we can do to make sure people don't forget to add to this when creating new files since it could be easy to miss. We should at least have a testing/ios/IosUnitTests/README.md.

added a readme

chinmaygarde

Some minor nits about GN rules but looks good overall.

chinmaygarde · 2020-04-10T22:04:53Z

shell/platform/darwin/ios/BUILD.gn

+ocmock_path = "../../../../../third_party/ocmock/Source"
+
+# TODO: Clone the OCMock repository so we can add a BUILD.gn to it.
+static_library("ocmock") {


This definition goes in //build/secondary/third_party/ocmock/BUILD.gn. Thats where we maintain target build rules for dependencies that don't maintain their own GN rules (or said rules are incompatible with our buildroot.)

Also, while you at it, make this a source_set as that skips the archiving step.

This needs to be a static_library because it is linked by xcode's build system when building the testing bundle.

Thanks for clarifying.

wrt moving the build file. I added those details to the TODO about cloning the repository if we find we can't use chromium's clone. I'll follow up on this, like the other issue I'd like to do anything that involves an extra repo for another PR since i'm juggling this and LUCI already.

chinmaygarde · 2020-04-10T22:09:17Z

shell/platform/darwin/ios/BUILD.gn

+  configs -= [ "//build/config/compiler:chromium_code" ]
+  cflags = [
+    "-fvisibility=default",
+    "-mios-simulator-version-min=13.0",


Please move this variable to //build/config/ios/ios_sdk.gni and add this flag to sysroot flags in //build/toolchain/mac/BUILD.gn. See where we add -miphoneos-version-min=, this can go right next to that on simulator targets. Adding sysroot flags on a per target basis quickly gets out of hand.

I pulled out the variable, added a todo issue to move it into build. Since this PR already has many moving pieces I want to do it in a separate PR instead of getting another repo involved.

chinmaygarde · 2020-04-10T22:12:21Z

shell/platform/darwin/ios/BUILD.gn

+# NOTE: This currently only support simulator targets because of the install_name.
+# TODO: Switch the install_name and make the test runner copy the dynamic library
+#       into the testing bundle.
+shared_library("ios_test_flutter") {


It might be a good idea now to track decommissioning the unused FlutterTests target if this is the way to go to write tests against engine internals with embedding code.

That could be useful in the future if someone wanted to do the work of moving the whole build system over to GN. This addition doesn't necessarily obsolesce target. I'll add a note. I'm afraid to rip out something I don't have to touch.

chinmaygarde · 2020-04-10T22:12:56Z

shell/platform/darwin/ios/BUILD.gn

+platform_frameworks_path = "$ios_sdk_path/../../Library/Frameworks/"
+
+# NOTE: This currently only support simulator targets because of the install_name.
+# TODO: Switch the install_name and make the test runner copy the dynamic library


Here and elsewhere: Please file issues for TODOs and cross-reference them by number here.

chinmaygarde · 2020-04-10T22:13:24Z

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


 - (void)testItReportsDarkPlatformBrightnessWhenTraitCollectionRequestsIt {
-  if (!@available(iOS 13, *)) {
+  if (@available(iOS 13, *)) {


Good catch. How did we miss this earlier? I expect clang had warnings for these.

"-Werror" isn't on by default in xcode projects, we were previously using xcode to build them.

chinmaygarde · 2020-04-10T22:28:11Z

shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm

+#include "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
+#import "flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.h"
+
+using namespace flutter;


Avoid using namespace https://google.github.io/styleguide/cppguide.html#Namespaces

Done. Checkout out how it says "never use using directives" then about 50 lines later they use one in an example. Other google projects seem to allow them as long as they are inside of a scoped namespace. I just removed it.

lol. WDYT about having our own version of the guidelines? We ought to be able to edit or amend them for clarity.

If we want to go that way we should make it an addendum that clarifies ambiguities like this as they show up.

gaaclarke · 2020-04-13T19:35:33Z

Landed on red to clear out luci errors. (There were warnings as errors as a result to a luci change).

…ode. (flutter/engine#17624)

…utter#17624)

googlebot added the cla: yes label Apr 10, 2020

gaaclarke force-pushed the all-ios-unit-tests branch 5 times, most recently from 4ffc767 to 1b8f117 Compare April 10, 2020 18:54

jmagman reviewed Apr 10, 2020

View reviewed changes

gaaclarke force-pushed the all-ios-unit-tests branch from 1b8f117 to 0fa69a6 Compare April 10, 2020 20:04

xster reviewed Apr 10, 2020

View reviewed changes

gaaclarke force-pushed the all-ios-unit-tests branch 7 times, most recently from 5dd5ddb to 45c4878 Compare April 10, 2020 21:49

Made it so unit tests can be written against all ios engine code.

b29dbaf

gaaclarke force-pushed the all-ios-unit-tests branch from 45c4878 to b29dbaf Compare April 10, 2020 21:51

gaaclarke marked this pull request as ready for review April 10, 2020 21:55

auto-assign bot requested a review from cbracken April 10, 2020 21:55

gaaclarke requested review from chinmaygarde, jmagman and xster April 10, 2020 21:55

xster reviewed Apr 10, 2020

View reviewed changes

cbracken reviewed Apr 10, 2020

View reviewed changes

xster reviewed Apr 10, 2020

View reviewed changes

chinmaygarde reviewed Apr 10, 2020

View reviewed changes

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

61ec840 Made it so unit tests can be written against all ios engine c…

7286974

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..fd1db54e02e7 (5 commits) flutter/flutter#54702

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

4d0c37e

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..053da38449a6 (7 commits) flutter/flutter#54705

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

1a0f70d

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..43e0a984106f (8 commits) flutter/flutter#54709

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

59a01ff

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..b360eab6f7e2 (10 commits) flutter/flutter#54716

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

3af5826

…ode. (flutter/engine#17624)

This was referenced Apr 14, 2020

Roll engine deef2663aca4..6bbdf33b0cee (11 commits) flutter/flutter#54719

Closed

Roll engine deef2663aca4..d3b2da0385ee (12 commits) flutter/flutter#54731

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

9b3e1ff

…ode. (flutter/engine#17624)

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

61ec840 Made it so unit tests can be written against all ios engine c…

3c4a586

…ode. (flutter/engine#17624)

This was referenced Apr 14, 2020

Roll engine deef2663aca4..662d7977a50d (13 commits) flutter/flutter#54738

Closed

Roll engine deef2663aca4..7dccc731b425 (14 commits) flutter/flutter#54743

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

9d8e34e

…ode. (flutter/engine#17624)

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

61ec840 Made it so unit tests can be written against all ios engine c…

69941b1

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..e8d71b4d2ff0 (15 commits) flutter/flutter#54764

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

bad9626

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..dfe13788edd4 (16 commits) flutter/flutter#54782

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

e1b9ffd

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..1dabddf113f2 (17 commits) flutter/flutter#54791

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

222a02f

…ode. (flutter/engine#17624)

engine-flutter-autoroll mentioned this pull request Apr 14, 2020

Roll engine deef2663aca4..6d17ce60cb5d (18 commits) flutter/flutter#54799

Closed

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

61ec840 Made it so unit tests can be written against all ios engine c…

d75247e

…ode. (flutter/engine#17624)

This was referenced Apr 15, 2020

Roll engine deef2663aca4..1192667a7a95 (19 commits) flutter/flutter#54803

Closed

Roll engine deef2663aca4..e6a2534b63ac (20 commits) flutter/flutter#54806

Merged

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

61ec840 Made it so unit tests can be written against all ios engine c…

a17b576

…ode. (flutter/engine#17624)

goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020

Made it so unit tests can be written against all ios engine code. (fl…

7202abd

…utter#17624)


		ocmock_path = "../../../../../third_party/ocmock/Source"

		# TODO: Clone the OCMock repository so we can add a BUILD.gn to it.

Conversation

gaaclarke commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Apr 10, 2020 •

edited

Loading