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
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 all 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
4 changes: 4 additions & 0 deletions BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ group("unittests") {
[ "//flutter/shell/platform/android:flutter_shell_native_unittests" ]
}

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

# Compile all unittests targets if enabled.
if (enable_unittests) {
public_deps += [
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ source_set("ios_test_flutter_mrc") {

shared_library("ios_test_flutter") {
testonly = true
Copy link
Member

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.

Copy link
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

Copy link
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.)

Copy link
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" ]
}

Copy link
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?

Copy link
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 = [ "*" ]
cflags = [
"-fvisibility=default",
"-F$platform_frameworks_path",
Expand Down