Rightpoint / BonMot

Beautiful, easy attributed strings in Swift
MIT License
3.54k stars 196 forks source link

Working on Xcode 13 fixes and SPM tests #412

Closed chrisballinger closed 3 years ago

chrisballinger commented 3 years ago

@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

2) 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:

raizlabs-oss-bot commented 3 years ago
1 Warning
:warning: Big PR
2 Messages
:book: Test Results
:book: Code Coverage

Current coverage for BonMot.framework is 76.71%

Files changed - -
Platform.swift 25.00% :no_entry_sign:
AdaptableTextContainer.swift 42.00% :no_entry_sign:
AdaptiveStyle.swift 73.99% :warning:
StyleableUIElement.swift 74.62% :warning:

Powered by xcov

Generated by :no_entry_sign: Danger

ZevEisenberg commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.