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.

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

Remove dependency on Quick/Nimble #157

Merged
merged 8 commits into from
Aug 20, 2021

Conversation

yoichitgy
Copy link
Member

@yoichitgy yoichitgy commented Jul 2, 2021

This PR addresses the same issue of Swinject Issue #472, and performs the migration from Quick/Nimble to XCTest in the same way as Swinject PR #473.

  • Replaced XxxSpec using Quick/Nimble with XxxTests using XCTest.
  • Removed dependency on Quick/Nimble. (We still need Carthage because of the dependency on Swinject.)

Note
Unit tests corresponding to those failed with Quick/Nimble still fail for macOS target. We need investigation later.
Screen Shot

@yoichitgy yoichitgy requested a review from mpdifran July 2, 2021 18:39
// We need to have test bundle deployment target on iOS 9.0 in order to compile storyboards with references.
// However, we need to disable these tests when running on iOS <9.0
// Using #available(iOS 9.0, *) produces compiler warning for the reasons above
if ProcessInfo().isOperatingSystemAtLeast(OperatingSystemVersion(majorVersion: 9, minorVersion: 0, patchVersion: 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove the iOS version check because we no longer support iOS 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although the current iOS deployment target is set to 12.0, it looks the target was increased by accident in the following PR. It looks good to revert the target to 9.0 later.
#156 (review)

@mpdifran
Copy link
Member

mpdifran commented Jul 4, 2021

Do the failing tests also fail for macOS on the current master (using Quick and Nimble)? If so, that likely indicates that our library is broken on macOS for some use cases.

@yoichitgy
Copy link
Member Author

The failing tests also fail for macOS using Quick and Nimble for the same conditions.

The tests fail with nil where a value is expected. I'm assuming Storyboard setting or some settings are broken in the macOS configuration of SwinijectStoryboard.

@mpdifran
Copy link
Member

mpdifran commented Jul 5, 2021

Gotcha. Are you cool with merging this PR after I do the release? Or do you think it's safe enough to go into the release?

@yoichitgy
Copy link
Member Author

yoichitgy commented Jul 5, 2021

I don't mind merging this PR after you make the release👍 Meanwhile, I'll check the cause of the test failure more😉

So far, I tried updating the dependency on Swinject to the latest commit, but the tests still failed...

github "Swinject/Swinject" "190a6d1e4c7c60258034d9b3b8098d87a2daa8fd"

@yoichitgy
Copy link
Member Author

I noticed unit tests failed if a vew/window controller was instantiated with instantiateInitialController, but still don't know why the test fail😥

@mpdifran
Copy link
Member

mpdifran commented Jul 5, 2021

From what I remember we employed some hacks to get that to work. Perhaps newer versions of the iOS SDK broke those hacks.

@yoichitgy
Copy link
Member Author

yoichitgy commented Jul 5, 2021

It looks the cause relates to the new NSStoryboard APIs Apple introduced for macOS 10.15+ and Xcode 11.0+

I'm still investigating the issue.

@yoichitgy
Copy link
Member Author

I made the investigation, but still not sure why instantiateInitialController doesn't work for macOS target...

@mpdifran
Copy link
Member

@yoichitgy release has been completed! Feel free to merge when you're ready!

# Conflicts:
#	SwinjectStoryboard.xcodeproj/project.pbxproj
@yoichitgy
Copy link
Member Author

Thanks @mpdifran for the review😃

I made a ticket #166 for the macOS issue found during the work for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants