[url_launcher] migrating iOS tests from objc to swift #4758

chrisdlangham · 2023-08-23T04:06:13Z

This PR converts the iOS test of the url_launcher plugin from objc to swift.

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#119102

Pre-launch Checklist

hellohuanlin

Looks clean. Just 2 nits

It's nice that our original ObjC code has good DI practice and does not rely on OCMock in tests. This allows us to migrate to Swift directly without refactoring original code.

hellohuanlin · 2023-08-28T19:37:51Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+
+class URLLauncherTests: XCTestCase {
+  var plugin: FLTURLLauncherPlugin!
+  var launcher: FakeLauncher!


these can be private.

And the class can be final.

hellohuanlin · 2023-08-28T19:38:29Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+
+}
+
+class FakeLauncher: NSObject, FULLauncher {


these 2 fake classes can be fileprivate and final

stuartmorgan

Thanks for the contribution!

stuartmorgan · 2023-08-23T13:50:38Z

packages/url_launcher/url_launcher_ios/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## NEXT

+* Migrates unit tests and UI test from objc to swift


This can be reverted; test-only changes don't need to be mentioned in the CHANGELOG.

stuartmorgan · 2023-08-30T20:28:27Z

packages/url_launcher/url_launcher_ios/ios/url_launcher_ios.podspec

-  s.public_header_files = 'Classes/**/*.h'
+  s.xcconfig = {
+        'LIBRARY_SEARCH_PATHS' => '$(TOOLCHAIN_DIR)/usr/lib/swift/$(PLATFORM_NAME)/ $(SDKROOT)/usr/lib/swift',
+          'LD_RUNPATH_SEARCH_PATHS' => '/usr/lib/swift',


Nit: these shouldn't be indented differently.

stuartmorgan · 2023-09-07T17:19:31Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+
+}
+
+final fileprivate class FakeLauncher: NSObject, FULLauncher {


Do we actually need this to inherit from NSObject?

yes since the plugin is still in objective-c

stuartmorgan · 2023-09-07T17:19:37Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+  }
+}
+
+final fileprivate class FakeFlutterBinaryMessenger: NSObject, FlutterBinaryMessenger {


Same question here.

stuartmorgan · 2023-09-07T17:24:09Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+  }
+}
+
+final fileprivate class FakeFlutterBinaryMessenger: NSObject, FlutterBinaryMessenger {


Actually, why does this class even exist? It's not used anywhere, and it wasn't in the Obj-C version.

stuartmorgan · 2023-09-07T17:27:15Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

+
+final class URLLauncherTests: XCTestCase {
+  private var plugin: FLTURLLauncherPlugin!
+  private var launcher: FakeLauncher!


Why did the test class become stateful in this migration? Stateful test fixtures are generally an anti-pattern, and making the object under test stateful is especially problematic.

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

stuartmorgan · 2023-09-07T17:30:49Z

Overrides: the changes to non-test files are only needed to support running the tests.

stuartmorgan · 2023-09-09T20:02:38Z

Is this ready for re-review, or still in progress?

chrisdlangham · 2023-09-11T04:19:37Z

@stuartmorgan Yes, it is ready.

stuartmorgan

LGTM, thanks!

…r/packages#4758)

flutter/packages@aaae5ef...ef0c65e 2023-09-11 [email protected] Roll Flutter from 7c28e8e to 219efce (16 revisions) (flutter/packages#4901) 2023-09-11 [email protected] [url_launcher] migrating iOS tests from objc to swift (flutter/packages#4758) 2023-09-09 [email protected] Roll Flutter from da676f7 to 7c28e8e (20 revisions) (flutter/packages#4879) 2023-09-09 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 3.1.2 to 3.1.3 (flutter/packages#4863) 2023-09-08 [email protected] Roll Flutter from aea4552 to da676f7 (28 revisions) (flutter/packages#4874) 2023-09-08 [email protected] [webview_flutter_android] Added the functionality to fullscreen html5 video (flutter/packages#3879) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] 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

* main: (83 commits) go_router_builder: support the latest pkg:analyzer (flutter#4921) Replace collection type lints with more general lint (flutter#4912) Roll Flutter from 219efce to 4e7a07a (30 revisions) (flutter#4910) [camerax] Implement `startVideoCapturing` and `onVideoRecordedEvent` (flutter#4815) [tool] Add a package inclusion filter (flutter#4904) [flutter_markdown] Fix changelog regarding minimum supported SDK version (flutter#4851) [ios_platform_images] Add integration tests (flutter#4899) [image_picker] Copy exif tags in categories II and III (flutter#4738) [google_maps_flutter_android] Fix for testToggleInfoWindow persistently flaky (flutter#4768) Roll Flutter from 7c28e8e to 219efce (16 revisions) (flutter#4901) [url_launcher] migrating iOS tests from objc to swift (flutter#4758) Roll Flutter from da676f7 to 7c28e8e (20 revisions) (flutter#4879) Bump actions/upload-artifact from 3.1.2 to 3.1.3 (flutter#4863) Roll Flutter from aea4552 to da676f7 (28 revisions) (flutter#4874) [webview_flutter_android] Added the functionality to fullscreen html5 video (flutter#3879) [tool] Add Android dependency (gradle) option to update dependencies command (flutter#4757) [camerax] Implement resolution configuration (flutter#3799) Manual roll Flutter from 685ce14 to aea4552 (64 revisions) (flutter#4870) [rfw, ci] Regenerate goldens, manually roll flutter#4835 (flutter#4862) Bump actions/checkout from 3.6.0 to 4.0.0 (flutter#4845) ...

chrisdlangham added 4 commits August 22, 2023 12:16

converting tests to swift

2741de4

converting unit tests to swift

495de26

Merge branch 'main' into converting-url-launcher-ios-tests-to-swift

c898698

updating change log

6a6c8d5

chrisdlangham requested a review from stuartmorgan as a code owner August 23, 2023 04:06

github-actions bot added platform-ios p: url_launcher labels Aug 23, 2023

stuartmorgan requested a review from hellohuanlin August 28, 2023 19:24

hellohuanlin approved these changes Aug 28, 2023

View reviewed changes

chrisdlangham added 2 commits August 29, 2023 00:21

making test classes final and private

761c6d9

resolving merge conflicts

968a2f1

stuartmorgan requested changes Sep 7, 2023

View reviewed changes

stuartmorgan added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Sep 7, 2023

chrisdlangham added 4 commits September 7, 2023 21:19

Merge branch 'main' into converting-url-launcher-ios-tests-to-swift

13e339f

removing updates to changelog

11cfe4f

refactoring unit tests

1228851

adjusting indentions

001f0b6

stuartmorgan approved these changes Sep 11, 2023

View reviewed changes

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

auto-submit bot merged commit 3981595 into flutter:main Sep 11, 2023

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

Roll Packages from aaae5ef97a45 to 39815959ade8 (5 revisions) flutter/flutter#134431

Closed

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

3981595 [url_launcher] migrating iOS tests from objc to swift (flutte…

17e355c

…r/packages#4758)

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

Roll Packages from aaae5ef97a45 to ef0c65ea4ccd (6 revisions) flutter/flutter#134445

Merged

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

3981595 [url_launcher] migrating iOS tests from objc to swift (flutte…

01c546d

…r/packages#4758)

chrisdlangham deleted the converting-url-launcher-ios-tests-to-swift branch September 11, 2023 20:05

reidbaker mentioned this pull request Sep 15, 2023

Update annotations deps to 1.7 #4935

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher] migrating iOS tests from objc to swift #4758

[url_launcher] migrating iOS tests from objc to swift #4758

chrisdlangham commented Aug 23, 2023

hellohuanlin left a comment •

edited

Loading

hellohuanlin Aug 28, 2023

hellohuanlin Aug 28, 2023 •

edited

Loading

hellohuanlin Aug 28, 2023 •

edited

Loading

stuartmorgan left a comment

stuartmorgan Aug 23, 2023

stuartmorgan Aug 30, 2023

stuartmorgan Sep 7, 2023

chrisdlangham Sep 8, 2023

stuartmorgan Sep 7, 2023

stuartmorgan Sep 7, 2023

chrisdlangham Sep 8, 2023

stuartmorgan Sep 7, 2023

stuartmorgan commented Sep 7, 2023

stuartmorgan commented Sep 9, 2023

chrisdlangham commented Sep 11, 2023

stuartmorgan left a comment

		@@ -1,5 +1,6 @@
		## NEXT

		* Migrates unit tests and UI test from objc to swift


		}

		final fileprivate class FakeLauncher: NSObject, FULLauncher {

[url_launcher] migrating iOS tests from objc to swift #4758

[url_launcher] migrating iOS tests from objc to swift #4758

Conversation

chrisdlangham commented Aug 23, 2023

Pre-launch Checklist

hellohuanlin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

hellohuanlin Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 7, 2023

stuartmorgan commented Sep 9, 2023

chrisdlangham commented Sep 11, 2023

stuartmorgan left a comment

Choose a reason for hiding this comment

hellohuanlin left a comment •

edited

Loading

hellohuanlin Aug 28, 2023 •

edited

Loading

hellohuanlin Aug 28, 2023 •

edited

Loading