Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[local_auth] Convert native unit tests to Swift #6779

Merged
merged 4 commits into from
May 22, 2024
Merged

[local_auth] Convert native unit tests to Swift #6779

merged 4 commits into from
May 22, 2024

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented May 21, 2024 •
edited
Loading

Converts native unit tests from Objective-C to Swift, as a first step toward an eventual plugin conversion. Since OCMock usage was removed in a previous PR, the tests are converted essentially directly (the rewrites were largely mechanical syntax replacement), without any changes needed to production code.

There are a few places where interacting with the Pigeon-generated Obj-C is somewhat awkward because the NSError signature isn't auto-converting to throw in cases where we use NSNumber wrapping, but all that will get cleaned up when we switch the plugin over to Swift Pigeon generation and have idiomatic Swift APIs that the tests will be calling instead.

Fixes a couple of false positives in the repo tooling surfaced by this PR:

  • darwin/Tests/ wasn't recognized as a test directory.
  • Swift tests weren't recognized as exempt from requiring Swift entries in the podspec.

Part of flutter/flutter#119104

Pre-launch Checklist

@stuartmorgan
[local_auth] Convert native unit tests to Swift
b6a9e65 
Converts native unit tests from Objective-C to Swift, as a first step
toward an eventual plugin conversion. Since OCMock usage was removed in
a previous PR, the tests are converted essentially directly (the
rewrites were largely mechanical syntax replacement), without any
changes needed to production code.

Part of flutter/flutter#119104
@stuartmorgan stuartmorgan requested a review from hellohuanlin May 21, 2024 20:39
@stuartmorgan stuartmorgan requested a review from LouiseHsu as a code owner May 21, 2024 20:39
@stuartmorgan
Copy link
Contributor Author

That check_podspec failure is a false positive; I'll just need to update the repo tooling to fix that.

hellohuanlin
hellohuanlin approved these changes May 22, 2024
View reviewed changes
packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift Outdated
@testable import local_auth_darwin

// Set a long timeout to avoid flake due to slow CI.
let timeout: TimeInterval = 30.0
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can be fileprivate (unless you want to make this timeout available in the whole test module)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift Outdated
XCTAssert(self.contexts.count > 0, "Insufficient test contexts provided")
let context = self.contexts.first!
contexts.remove(at: 0)
return context
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

return self.contexts.removeFirst()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, I didn't realize Swift arrays had fixed some of the annoying API limitations of NSArray (like not having a remove-and-return method).

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift Outdated
var evaluateError: NSError?

// Overridden as read-write to allow stubbing.
var biometryType: LABiometryType = LABiometryType.none
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: type can be inferred

  var biometryType: LABiometryType = .none

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed throughout.

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift Outdated
class FLALocalAuthPluginTests: XCTestCase {

override func setUp() {
continueAfterFailure = false
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why? (also maybe add comment)

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 clue, it looks like it dates back to my adding the original test file. Copypasta from an XCUITest maybe? Removed.

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift Outdated
let timeout: TimeInterval = 30.0

/// A context factory that returns preset contexts.
class StubAuthContextFactory: NSObject, FLADAuthContextFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can be final class (unless we intend to subclass it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented May 22, 2024

auto label is removed for flutter/packages/6779, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label May 22, 2024
@stuartmorgan
Copy link
Contributor Author

Changelog override: only affects tests.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2024
@auto-submit auto-submit bot merged commit 6525441 into flutter:main May 22, 2024
74 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2024
@engine-flutter-autoroll
6525441 [local_auth] Convert native unit tests to Swift (flutter/pack…
854d62f 
…ages#6779)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 22, 2024
@engine-flutter-autoroll
Roll Packages from ba19b24 to 6525441 (12 revisions) (#148864)
8f227fe 
flutter/packages@ba19b24...6525441

2024-05-22 stuartmorgan@google.com [local_auth] Convert native unit tests to Swift (flutter/packages#6779)
2024-05-22 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.5.0 to 1.8.0 in /packages/interactive_media_ads/android (flutter/packages#6771)
2024-05-22 vongrejadam@gmail.com [in_app_purchase_android] Introduced new ReplacementMode for Android's billing client (flutter/packages#6515)
2024-05-21 hashirshoaeb@gmail.com [go_router] New feature improve debug full path (flutter/packages#6714)
2024-05-21 stuartmorgan@google.com [interactive_media_ads] Add SPM support (flutter/packages#6756)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter from 02a6c91 to d02292d (22 revisions) (flutter/packages#6778)
2024-05-21 stuartmorgan@google.com [local_auth] Remove use of OCMock (flutter/packages#6757)
2024-05-21 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.3 to 3.25.6 (flutter/packages#6777)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/file_selector/file_selector_android/android (flutter/packages#6769)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#6765)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/url_launcher/url_launcher_android/android (flutter/packages#6762)
2024-05-20 engine-flutter-autoroll@skia.org Roll Flutter from adf279f to 02a6c91 (8 revisions) (flutter/packages#6776)

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 flutter-ecosystem@google.com,rmistry@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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@stuartmorgan stuartmorgan deleted the local-auth-swift-tests branch May 22, 2024 19:54
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
@engine-flutter-autoroll @victorsanni
Roll Packages from ba19b24 to 6525441 (12 revisions) (flutter#148864)
8959014 
flutter/packages@ba19b24...6525441

2024-05-22 stuartmorgan@google.com [local_auth] Convert native unit tests to Swift (flutter/packages#6779)
2024-05-22 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.5.0 to 1.8.0 in /packages/interactive_media_ads/android (flutter/packages#6771)
2024-05-22 vongrejadam@gmail.com [in_app_purchase_android] Introduced new ReplacementMode for Android's billing client (flutter/packages#6515)
2024-05-21 hashirshoaeb@gmail.com [go_router] New feature improve debug full path (flutter/packages#6714)
2024-05-21 stuartmorgan@google.com [interactive_media_ads] Add SPM support (flutter/packages#6756)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter from 02a6c91 to d02292d (22 revisions) (flutter/packages#6778)
2024-05-21 stuartmorgan@google.com [local_auth] Remove use of OCMock (flutter/packages#6757)
2024-05-21 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.3 to 3.25.6 (flutter/packages#6777)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/file_selector/file_selector_android/android (flutter/packages#6769)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#6765)
2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/url_launcher/url_launcher_android/android (flutter/packages#6762)
2024-05-20 engine-flutter-autoroll@skia.org Roll Flutter from adf279f to 02a6c91 (8 revisions) (flutter/packages#6776)

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 flutter-ecosystem@google.com,rmistry@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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@stuartmorgan @arc-yong
[local_auth] Convert native unit tests to Swift (flutter#6779)
95e4a92 
Converts native unit tests from Objective-C to Swift, as a first step toward an eventual plugin conversion. Since OCMock usage was removed in a previous PR, the tests are converted essentially directly (the rewrites were largely mechanical syntax replacement), without any changes needed to production code.

There are a few places where interacting with the Pigeon-generated Obj-C is somewhat awkward because the NSError signature isn't auto-converting to `throw` in cases where we use `NSNumber` wrapping, but all that will get cleaned up when we switch the plugin over to Swift Pigeon generation and have idiomatic Swift APIs that the tests will be calling instead.

Fixes a couple of false positives in the repo tooling surfaced by this PR:
- `darwin/Tests/` wasn't recognized as a test directory.
- Swift tests weren't recognized as exempt from requiring Swift entries in the podspec.

Part of flutter/flutter#119104
@reidbaker reidbaker mentioned this pull request Jul 2, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellohuanlin hellohuanlin hellohuanlin approved these changes

@LouiseHsu LouiseHsu Awaiting requested review from LouiseHsu LouiseHsu is a code owner

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App override: no changelog needed Override the check requiring CHANGELOG updates for most changes p: local_auth platform-ios platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stuartmorgan @hellohuanlin