Made it so unit tests can be written against all ios engine code. #17624
Conversation
4ffc767 to
1b8f117
Compare
shell/platform/darwin/ios/BUILD.gn
Outdated
| configs -= [ "//build/config/compiler:chromium_code" ] | ||
| cflags = [ | ||
| "-fvisibility=default", | ||
| "-F/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform//Developer/Library/Frameworks/", |
There was a problem hiding this comment.
- What if Xcode is at a different path?
- What about the non-simulator frameworks?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is still a draft PR =).
Sorry, got excited!
1b8f117 to
0fa69a6
Compare
| public = _flutter_framework_headers | ||
|
|
||
| source_set("flutter_framework_source") { | ||
| cflags_objc = flutter_cflags_objc |
There was a problem hiding this comment.
we can add a non-public visibility clause here to be clean
shell/platform/darwin/ios/BUILD.gn
Outdated
| "-Wno-misleading-indentation", | ||
| ] | ||
| sources = [ | ||
| "../../../../../third_party/ocmock/Source/OCMock/NSInvocation+OCMAdditions.h", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
nope, we are piggy backing on ./flutter/tools/gn
| include_dirs = [ "../../../../../third_party/ocmock/Source/" ] | ||
| } | ||
|
|
||
| ios_test_flutter_path = rebase_path("$root_out_dir/libios_test_flutter.dylib") |
There was a problem hiding this comment.
perhaps still mention that it's for simulators in the name
|
This PR depends on LUCI updating: https://chromium-review.googlesource.com/c/chromium/tools/build/+/2145769 |
5dd5ddb to
45c4878
Compare
45c4878 to
b29dbaf
Compare
shell/platform/darwin/ios/BUILD.gn
Outdated
|
|
||
| ocmock_path = "../../../../../third_party/ocmock/Source" | ||
|
|
||
| # TODO: Clone the OCMock repository so we can add a BUILD.gn to it. |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
just use autoninja here, so it knows if it should use goma or not.
shell/platform/darwin/ios/BUILD.gn
Outdated
| 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. |
There was a problem hiding this comment.
ultra nit: s/support/supports/
| "framework/Source/FlutterPluginAppLifeCycleDelegateTest.m", | ||
| "framework/Source/FlutterTextInputPluginTest.m", | ||
| "framework/Source/FlutterViewControllerTest.mm", | ||
| "framework/Source/SemanticsObjectTest.mm", |
There was a problem hiding this comment.
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.
chinmaygarde
left a comment
There was a problem hiding this comment.
Some minor nits about GN rules but looks good overall.
| ocmock_path = "../../../../../third_party/ocmock/Source" | ||
|
|
||
| # TODO: Clone the OCMock repository so we can add a BUILD.gn to it. | ||
| static_library("ocmock") { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Also, while you at it, make this a source_set as that skips the archiving step.
There was a problem hiding this comment.
This needs to be a static_library because it is linked by xcode's build system when building the testing bundle.
There was a problem hiding this comment.
Thanks for clarifying.
There was a problem hiding this comment.
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.
shell/platform/darwin/ios/BUILD.gn
Outdated
| configs -= [ "//build/config/compiler:chromium_code" ] | ||
| cflags = [ | ||
| "-fvisibility=default", | ||
| "-mios-simulator-version-min=13.0", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # 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") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
shell/platform/darwin/ios/BUILD.gn
Outdated
| 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 |
There was a problem hiding this comment.
Here and elsewhere: Please file issues for TODOs and cross-reference them by number here.
|
|
||
| - (void)testItReportsDarkPlatformBrightnessWhenTraitCollectionRequestsIt { | ||
| if (!@available(iOS 13, *)) { | ||
| if (@available(iOS 13, *)) { |
There was a problem hiding this comment.
Good catch. How did we miss this earlier? I expect clang had warnings for these.
There was a problem hiding this comment.
"-Werror" isn't on by default in xcode projects, we were previously using xcode to build them.
| #include "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h" | ||
| #import "flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.h" | ||
|
|
||
| using namespace flutter; |
There was a problem hiding this comment.
Avoid using namespace https://google.github.io/styleguide/cppguide.html#Namespaces
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
lol. WDYT about having our own version of the guidelines? We ought to be able to edit or amend them for clarity.
There was a problem hiding this comment.
If we want to go that way we should make it an addendum that clarifies ambiguities like this as they show up.
|
Landed on red to clear out luci errors. (There were warnings as errors as a result to a luci change). |
relevant pr: flutter/flutter#54324
This introduces a new shared_library that has all the contents of
Fluttter.frameworkand the compiled unit tests. This is the library the test runner links against instead ofFlutter.framework. That allows us to bypass the visibility problem since the tests are in the same binary as the code.