Skip to content

Conversation

@chrisballinger
Copy link
Contributor

@ZevEisenberg Building on #411 I had to revert your changes to featureIdentifier and typeIdentifier to get things to compile on Xcode 12 again. It crashes at runtime in Xcode 13b2 but it appears that it's a known bug (79090498) that will probably be fixed soon in a later release. Not sure if there's another workaround.

While cleaning things up I also added the ability to run tests via Swift Package Manager (superceding #382), however there are still issues there:

  1. when running via swift test it will crash because it can't find a resource
BonMotTests/ComposableTests.swift:23: Fatal error: Unexpectedly found nil while unwrapping an Optional value
  1. when opening up Package.swift in Xcode and running SPM tests that way, it will run the tests, but a few of them are failing (FontInspectorTests for iOS, and also ImageTintingTests for macOS)

This PR also contains breaking changes to the minimum requirements:

  • Swift 5.0 or higher
  • iOS 11 or higher
  • Xcode 12 or higher

@raizlabs-oss-bot
Copy link
Collaborator

raizlabs-oss-bot commented Jun 26, 2021

1 Warning
⚠️ Big PR
2 Messages
📖 Test Results
📖 Code Coverage

Current coverage for BonMot.framework is 76.71%

Files changed - -
Platform.swift 25.00% 🚫
AdaptableTextContainer.swift 42.00% 🚫
AdaptiveStyle.swift 73.99% ⚠️
StyleableUIElement.swift 74.62% ⚠️

Powered by xcov

Generated by 🚫 Danger

@ZevEisenberg
Copy link
Collaborator

Let's wait for the next beta and see if they fix that known issue. In terms of workarounds, maybe there's something we can do with rawValue?

I think the test is crashing because the test resources aren't included. I think there's an issue or PR open about it already, but it hasn't been touched since package resources became a thing. Worth a revisit.

The failing tests you mention are currently disabled on the platforms you mention, I believe. I couldn't find a good way to make them pass everywhere. I don't know what control we have over that in SPM tests. Maybe we can use XCTSkip, if that's available?

I think SPM supports executable targets with @main now. Any chance we can port our sample project over as well, and lose the .xcodeproj entirely? SwiftLint did it, but they don't have a GUI sample project. Not sure if possible.

The breaking version changes sound acceptable. Loving it!

@ZevEisenberg
Copy link
Collaborator

I see you're already doing the resources stuff. Not sure what's wrong, then. Check the resulting bundle. I think the parent folder is copied in intact, and that needs to be included in the search for a particular file.

Also, I deprecated something recently and renamed it because of a typo. Since this is a breaking change, let's go ahead and delete the deprecated one just to clean things up.

And finally, I'd prefer not to bump the version number in this PR. Version bumps should be separate IMO. But it's not a strong preference, so stick with this if you prefer to avoid the extra paperwork, as it were.

@chrisballinger
Copy link
Contributor Author

The reason I added the version bump in this PR was because the podspec itself required a minimum iOS version change as well. I agree that typically that part should be handled separately.

XCTSkip might be the ticket, because I don't think SPM has a way to skip tests without it.

As far as the SPM test resource bundle issue, it should be working, and does work when using Xcode's SPM integration, just not the command line SPM via swift test. Possibly a SPM bug, will poke around a bit more.

@chrisballinger
Copy link
Contributor Author

@ZevEisenberg That resource issue is that xcasset catalogs are not compiled into Assets.car when using swift test, but they are when using Xcode's SPM integration.

Had to add some more XCTSkip to a few spots where there were inconsistent results, mainly the image tinting tests.

@chrisballinger chrisballinger merged commit b156847 into xcode-13 Jun 30, 2021
@chrisballinger chrisballinger deleted the xcode-13-redux branch June 30, 2021 00:20
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.

4 participants