Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Merged
auto-submit merged 5 commits into from
Aug 31, 2023
Merged

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e29c94
Revert "Revert "Build iOS unittest target in unopt builds" (#44356)"
Aug 17, 2023
75b5697
review
Aug 18, 2023
26438be
review
Aug 22, 2023
db6cb09
format
Aug 22, 2023
01c77ef
fix
Aug 22, 2023
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
3 changes: 3 additions & 0 deletions shell/platform/darwin/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import("//flutter/shell/platform/config.gni")
group("darwin") {
if (is_ios) {
deps = [ "ios:flutter_framework" ]
if (is_debug) {

@dnfield dnfield Aug 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about just if (enable_unittests)?

@cyanglaz cyanglaz Aug 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

enable_unittests = current_toolchain == host_toolchain || is_fuchsia || is_mac, I will add || is_ios

deps += [ "//flutter/shell/platform/darwin/ios:ios_test_flutter" ]
}
}
if (is_mac) {
deps = []
Expand Down
6 changes: 4 additions & 2 deletions shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,10 @@ source_set("ios_test_flutter_mrc") {
}

shared_library("ios_test_flutter") {
testonly = true

@chinmaygarde chinmaygarde Aug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we getting rid of this? It seems like a useful guard against non-test targets including test TUs.

@cyanglaz cyanglaz Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When this flag is on, because flutter_framework is not testonly, it cannot depend on ios_test_flutter

@dnfield dnfield Aug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not make flutter_framework depend on ios_test_flutter, we should update some higher level BUILD.gn that checks if unit tests are enabled and if so include ios_test_flutter (if ios etc.)

@dnfield dnfield Aug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

e.g. in //BUILD.gn something like

if (enable_unittests && is_ios) {
  public_deps += [ "//flutter/shell/platform/darwin/ios:ios_test_flutter" ]
}

@dnfield dnfield Aug 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add testonly= true back now?

@cyanglaz cyanglaz Aug 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. Or I can make group(darwin) testonly? If that's a problem I can move the dependency to a upper level. Eventually I should be able to find a unbrella build target that is testonly up in the chain.

visibility = [ ":*" ]
visibility = [
":*",
"//flutter/shell/platform/darwin:*",
]
cflags = [
"-fvisibility=default",
"-F$platform_frameworks_path",
Expand Down