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

cyanglaz · 2023-09-06T23:02:35Z

Reland #44821

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz · 2023-09-08T18:47:10Z

@stuartmorgan Could you help me review the clang-tidy error fixes in Objective-C?

cyanglaz · 2023-09-08T18:47:24Z

.ci.yaml

@@ -393,7 +393,7 @@ targets:

  - name: Mac mac_clang_tidy
    recipe: engine_v2/engine_v2
-    presubmit: false
+    presubmit: true


Revert before committing.

…lutter#44356)"" (flutter#45346)" This reverts commit ca513c9. fix 1 fix fix temp ci change fix name space fix fix fix format revert fix draft fix unittest fix fromat eof line

stuartmorgan

Minor comments, but mostly it all looks good. It does seem to be flagging one more failure though.

stuartmorgan · 2023-09-08T19:48:58Z

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

@@ -18,9 +18,12 @@

 namespace {
 API_AVAILABLE(ios(13.4))
+// NOLINTNEXTLINE(readability-identifier-naming)


Why do want to suppress this rather than fix the naming?

Done. There is actually a constant for it, I removed this declaration.

stuartmorgan · 2023-09-08T19:50:04Z

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

@@ -130,6 +130,7 @@ - (void)testReleasesProjectOnDealloc {
  __weak FlutterDartProject* weakProject;
  @autoreleasepool {
    FlutterDartProject* mockProject = OCMClassMock([FlutterDartProject class]);
+    // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores)


Maybe just XCTAssertNotNil(group) instead of doing this?

stuartmorgan · 2023-09-08T19:50:13Z

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

@@ -130,6 +130,7 @@ - (void)testReleasesProjectOnDealloc {
  __weak FlutterDartProject* weakProject;
  @autoreleasepool {
    FlutterDartProject* mockProject = OCMClassMock([FlutterDartProject class]);
+    // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores)


Maybe just XCTAssertNotNil(group) instead of doing this?

stuartmorgan · 2023-09-08T19:53:07Z

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

@@ -1575,6 +1575,7 @@ - (void)testWillDeallocNotification {
      [[XCTestExpectation alloc] initWithDescription:@"notification called"];
  id engine = [[MockEngine alloc] init];
  @autoreleasepool {
+    // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores)


Similarly we could assert that this isn't nil just before niling it out.

stuartmorgan · 2023-09-08T19:54:30Z

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

@@ -12,6 +12,12 @@

 const CGRect kScreenSize = CGRectMake(0, 0, 600, 800);

+// copied from clang document: https://clang-analyzer.llvm.org/faq.html#unlocalized_string


Couldn't we just use NSLocalizedString everywhere? It's a test, so it's not like that's going to affect a shipped binary.

cyanglaz · 2023-09-08T20:42:47Z

@stuartmorgan Updated per review comment, PTAL!

format fix fix fix

stuartmorgan

LGTM! Don't forget to revert .ci.yaml :)

cyanglaz · 2023-09-12T18:40:37Z

Don't forget to revert .ci.yaml :)

Thanks for the reminder! Juggling between several PRs and reviews and I've already forgotten about it.

…#44356)"" (flutter#45346)" (flutter/engine#45519)

…ions) (#134589) Manual roll requested by bdero@google.com flutter/engine@496ef6a...c90fadf 2023-09-12 skia-flutter-autoroll@skia.org Manual roll Dart SDK from 43d4b1373788 to 1ee7ef8bbc65 (6 revisions) (flutter/engine#45726) 2023-09-12 ychris@google.com Reland "Build iOS unittest target in unopt builds" (#44356)"" (#45346)" (flutter/engine#45519) 2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from a4f8f5177c8b to 211d63b1e1f5 (2 revisions) (flutter/engine#45724) 2023-09-12 jacksongardner@google.com Fix JS interop signatures to use only JS types. (flutter/engine#45668) 2023-09-12 ychris@google.com [ios] Fix testDeallocated failing locally. (flutter/engine#45663) 2023-09-12 ychris@google.com [iOS] move arm64 builds to arm machines (flutter/engine#45721) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ions) (flutter#134589) Manual roll requested by bdero@google.com flutter/engine@496ef6a...c90fadf 2023-09-12 skia-flutter-autoroll@skia.org Manual roll Dart SDK from 43d4b1373788 to 1ee7ef8bbc65 (6 revisions) (flutter/engine#45726) 2023-09-12 ychris@google.com Reland "Build iOS unittest target in unopt builds" (flutter#44356)"" (flutter#45346)" (flutter/engine#45519) 2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from a4f8f5177c8b to 211d63b1e1f5 (2 revisions) (flutter/engine#45724) 2023-09-12 jacksongardner@google.com Fix JS interop signatures to use only JS types. (flutter/engine#45668) 2023-09-12 ychris@google.com [ios] Fix testDeallocated failing locally. (flutter/engine#45663) 2023-09-12 ychris@google.com [iOS] move arm64 builds to arm machines (flutter/engine#45721) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the platform-ios label Sep 6, 2023

cyanglaz force-pushed the reland_unittest branch 2 times, most recently from 4e4a48e to 9c7fcd8 Compare September 8, 2023 18:46

cyanglaz requested review from zanderso and stuartmorgan September 8, 2023 18:46

cyanglaz marked this pull request as ready for review September 8, 2023 18:46

cyanglaz commented Sep 8, 2023

View reviewed changes

Revert "Revert "Reland "Build iOS unittest target in unopt builds" (f…

b3cfd92

…lutter#44356)"" (flutter#45346)" This reverts commit ca513c9. fix 1 fix fix temp ci change fix name space fix fix fix format revert fix draft fix unittest fix fromat eof line

cyanglaz force-pushed the reland_unittest branch from 9c7fcd8 to b3cfd92 Compare September 8, 2023 18:48

stuartmorgan reviewed Sep 8, 2023

View reviewed changes

review

16e0803

format fix fix fix

cyanglaz force-pushed the reland_unittest branch from 0170774 to 16e0803 Compare September 8, 2023 21:10

stuartmorgan approved these changes Sep 12, 2023

View reviewed changes

revert presubmit flag

61427e0

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2023

auto-submit bot merged commit eab8b71 into flutter:main Sep 12, 2023
26 checks passed

cyanglaz deleted the reland_unittest branch September 12, 2023 19:21

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter Engine from 496ef6a9c277 to eab8b7113739 (5 revisions) flutter/flutter#134578

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

eab8b7113 Reland "Build iOS unittest target in unopt builds" (flutter…

10db09b

…#44356)"" (flutter#45346)" (flutter/engine#45519)

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter Engine from 496ef6a9c277 to c5ba843ddf72 (7 revisions) flutter/flutter#134581

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

eab8b7113 Reland "Build iOS unittest target in unopt builds" (flutter…

f6d20e1

…#44356)"" (flutter#45346)" (flutter/engine#45519)

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter Engine from 496ef6a9c277 to 4519acd50345 (8 revisions) flutter/flutter#134583

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

eab8b7113 Reland "Build iOS unittest target in unopt builds" (flutter…

4fea78f

…#44356)"" (flutter#45346)" (flutter/engine#45519)

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Manual roll Flutter Engine from 496ef6a9c277 to c90fadf45d44 (6 revisions) flutter/flutter#134589

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023

eab8b7113 Reland "Build iOS unittest target in unopt builds" (flutter…

bfe6dee

…#44356)"" (flutter#45346)" (flutter/engine#45519)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cyanglaz commented Sep 6, 2023

cyanglaz commented Sep 8, 2023

cyanglaz Sep 8, 2023

stuartmorgan left a comment

stuartmorgan Sep 8, 2023

cyanglaz Sep 8, 2023

stuartmorgan Sep 8, 2023

stuartmorgan Sep 8, 2023

stuartmorgan Sep 8, 2023

stuartmorgan Sep 8, 2023

cyanglaz commented Sep 8, 2023

stuartmorgan left a comment

cyanglaz commented Sep 12, 2023 •

edited

		@@ -12,6 +12,12 @@

		const CGRect kScreenSize = CGRectMake(0, 0, 600, 800);

		// copied from clang document: https://clang-analyzer.llvm.org/faq.html#unlocalized_string

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

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

Conversation

cyanglaz commented Sep 6, 2023

Pre-launch Checklist

cyanglaz commented Sep 8, 2023

cyanglaz Sep 8, 2023

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Sep 8, 2023

Choose a reason for hiding this comment

cyanglaz Sep 8, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 8, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 8, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 8, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 8, 2023

Choose a reason for hiding this comment

cyanglaz commented Sep 8, 2023

stuartmorgan left a comment

Choose a reason for hiding this comment

cyanglaz commented Sep 12, 2023 • edited

cyanglaz commented Sep 12, 2023 •

edited