iOS: Re-enable iOS unittests#167893
Conversation
| if (enable_unittests) { | ||
| if (is_ios) { | ||
| public_deps += [ "//flutter/shell/platform/darwin/ios:ios_test_flutter" ] | ||
| } |
There was a problem hiding this comment.
In #167530, this block was moved inside the enable_unittests conditional here:
https://github.com/flutter/flutter/pull/167530/files#diff-0009a2dc7028a9776b47e2251ca6b7154d4cd374466a074996d29cf881ab940aL181
This caused iOS tests to be disabled, since enable_unittests is never true for is_ios or is_android. See testing.gni below, and note it's defined as:
enable_unittests = current_toolchain == host_toolchain || is_fuchsia || is_macThere was a problem hiding this comment.
This is... so confusing. may be add a comment here or rename it? nvm i saw your another comment.
| # * host_toolchain: non-cross-compile. We can run tests on the host. | ||
| # * Fuchsia: build unittests for testing on device. | ||
| # * macOS: arm64 builds can run x64 binaries. | ||
| enable_unittests = current_toolchain == host_toolchain || is_fuchsia || is_mac |
There was a problem hiding this comment.
I think a reasonable followup might be to rename enable_unittests to something like enable_portable_unittests or similar, since these are used to build our generic FML, embedder API, Impeller, etc. tests into binaries that are runnable on general-purpose OSes like Linux, macOS, including non-host OSes such as Fuchsia.
iOS and Android tests, on the other hand, need to be built as installable app bundles, which is a bit of a different usecase.
I think it's worthwhile treating these as separate variables to avoid having to litter the build with enable_unittests && !(is_ios || is_android). I also think it's worth having a specific enable_ios_unittests and probably enable_android_unittests for the same reason, that way we avoid enable_target_device_unittests && is_ios etc. all over the place.
Would love any suggestions on better names for enable_unittests that emphasise that this is specifically for platform-portable stuff that can be run on general-purpose OSes.
There was a problem hiding this comment.
... on general-purpose OSes like Linux, macOS, including non-host OSes such as Fuchsia.
by general purpose do you mean just host builds (except fushsia which is the odd one out)?
There was a problem hiding this comment.
Right -- "host" refers to the OS that is doing the compilation and "target" refers to the target OS that we're building binaries for. Many of our unit tests (like the embedder API tests) are totally generic, portable tests. When there's no platform-specific behaviour, we can just run them as general tests on the host OS.
Other tests (like Impeller-specific tests) have some platform-specific behaviour (e.g. Metal requires Apple platforms [I'm ignoring swiftshader/swangle here]) but it's configuration that is supported on at least one of our host platforms (e.g. Metal on macOS) so doesn't necessarily have to run on iOS.
iOS and Android don't really have the facility to run arbitrary ELF or Mach-O binaries (e.g. from a command-line), and generally require apps to be built into some form of package, deployed etc. so there's not a huge value in e.g. running the generic C++ embedder API tests on iOS or Android.
Fuchsia is a bit special because we always cross-compile for Fuchsia from Linux or macOS, but Fuchsia is a general-purpose OS that does have the ability to run binaries from a command-line. As such, we build a bunch of these for Fuchsia. So in effect these aren't so much host tests as really generic tests we can run on general-purpose OSes.
In a bunch of cases (maybe even most cases), we could actually build them into an IPA for iOS or an APK for Android, but we don't because we get very little added value in doing so, but would take the non-zero added cost of having to wrap them up in app packages and deal with host-target launch, logging, and result-checking.
Re-enables iOS unittests, which were inadvertently disabled as part of flutter#167530. This extracts out `enable_ios_unittests` for readability/re-usability across the build system, and better documents `enable_unittests` as specifically being for platform-portable unit tests that can be run on general-purpose operating systems such as macOS, Fuchsia, Linux, but not specific target OSes such as iOS/Android that require executables to be built against platform SDKs as installable apps. Issue: flutter#144791
e2d02b1 to
616df92
Compare
| if (enable_unittests) { | ||
| if (is_ios) { | ||
| public_deps += [ "//flutter/shell/platform/darwin/ios:ios_test_flutter" ] | ||
| } |
There was a problem hiding this comment.
This is... so confusing. may be add a comment here or rename it? nvm i saw your another comment.
| ] | ||
| } | ||
|
|
||
| platform_frameworks_path = |
There was a problem hiding this comment.
github is confused with this diff - all these lines are just indented because of the if right?
| # * host_toolchain: non-cross-compile. We can run tests on the host. | ||
| # * Fuchsia: build unittests for testing on device. | ||
| # * macOS: arm64 builds can run x64 binaries. | ||
| enable_unittests = current_toolchain == host_toolchain || is_fuchsia || is_mac |
There was a problem hiding this comment.
... on general-purpose OSes like Linux, macOS, including non-host OSes such as Fuchsia.
by general purpose do you mean just host builds (except fushsia which is the odd one out)?
Re-enables iOS unittests, which were inadvertently disabled as part of flutter#167530. This extracts out `enable_ios_unittests` for readability/re-usability across the build system, and better documents `enable_unittests` as specifically being for platform-portable unit tests that can be run on general-purpose operating systems such as macOS, Fuchsia, Linux, but not specific target OSes such as iOS/Android that require executables to be built against platform SDKs as installable apps. Issue: flutter#144791 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Re-enables iOS unittests, which were inadvertently disabled as part of flutter#167530. This extracts out `enable_ios_unittests` for readability/re-usability across the build system, and better documents `enable_unittests` as specifically being for platform-portable unit tests that can be run on general-purpose operating systems such as macOS, Fuchsia, Linux, but not specific target OSes such as iOS/Android that require executables to be built against platform SDKs as installable apps. Issue: flutter#144791 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Re-enables iOS unittests, which were inadvertently disabled as part of #167530.
This extracts out
enable_ios_unittestsfor readability/re-usability across the build system, and better documentsenable_unittestsas specifically being for platform-portable unit tests that can be run on general-purpose operating systems such as macOS, Fuchsia, Linux, but not specific target OSes such as iOS/Android that require executables to be built against platform SDKs as installable apps.Issue: #144791
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.