BottleRocketStudios / iOS-Hyperspace

An extremely lightweight wrapper around URLSession to make working with APIs a breeze.
Apache License 2.0
47 stars 17 forks source link

Adjust unit tests to work with `async/await` #162

Closed GrandLarseny closed 1 year ago

GrandLarseny commented 2 years ago

Also started adding some documentation

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

GrandLarseny commented 2 years ago

@wmcginty @tylermilner So I'm running into an Xcode bug for testing that's really, really making me want to jettison the .xcodeproj and .xcodeworkspace files in favor of just opening Package.swift. That brings up the problem of how to expose sample code, which I suggest we handle in much the same way now, just using SPM local packages instead of Cocoapods.

Ultimately, testing a swift package that is also trying to support Cocoapods by having a second project structure is very cumbersome. I believe focusing on SPM will set this project up better for the future.

I might be able to get the project building without errors or warnings as-is, but removing the project files and relying only on SPM will help.

wmcginty commented 2 years ago

What is the particular testing bug?

I've actually gone the opposite way, there are some missing features (at least they were when I moved to the workspace/project model) that made swift test untenable. So the way the current setup was built was that the extent of SPM support was being able to import the package through SPM.

GrandLarseny commented 1 year ago

There's a bug with xcodebuild where it doesn't auto-generate resource_bundle_accessor.swift the same way the SPM commands do. That means that attempting to access resources for testing (ex: certificates and mock JSON files) will throw an error using the SPM method of Bundle.module.url(for:).

Conversely, resources are not available to SPM using Bundle(for:).path so attempting to test the package when opening Package.swift results in execution failure.

I was able to get tests working with swift test, even had them succeed on GitHub. However I do not believe we'll be able to test per platform. But since this library just makes use of the foundation I think we should be good.

At the very least I think it'd be good to note in CONTRIBUTING.md how we intend for development to work moving forward. I'm very open to whatever you want to do!

wmcginty commented 1 year ago

I guess my question is why do we need to run the tests through swift test? On the feature/async branch the tests run fine through xcodebuild (they don't all pass yet, obviously, but they run). In my eyes, for as long we still want to support CP, Carthage and SPM there isn't an issue having the project itself build and test using xcodebuild instead of swift test. Sure, it can be annoying to remember to open the workspace and not the Package.swift, but I don't think thats a huge overhead, especially as it's only for maintainers/collaborators.

I am happy to have the conversation about dropping Carthage/CP support and converting build/test to swift build and swift test (and honestly, I'm all for it), but I'd like that conversation to include all our libraries.

GrandLarseny commented 1 year ago

To answer your question, they don't have to be run through swift test. With my tools, the tests would not run at all with xcodebuild, but I believe I can make sure they're running now that I know the intended development context.

I'm gonna go ahead and start over with a new branch now that I have my hands around what's going on and try to get it where we're comfortable merging this branch in.

wmcginty commented 1 year ago

@GrandLarseny Definitely weird - for reference sake I've got them to run now on latest 14 beta using:

xcodebuild test -workspace Hyperspace.xcworkspace -scheme "Hyperspace iOS" -destination "platform=iOS Simulator,OS=16.0,name=iPhone 13 Pro"

I should have a bit of free time this afternoon, I can take a stab at replicating that behavior on GHA as well.

Edit: The tests on GHA should be now a more accurate reflection of the current state of the PR (I commented out quite a few of the tests to incrementally rebuild them, so "success" is probably misleading, but they are running now).

GrandLarseny commented 1 year ago

Closing for now. It'll be good to discuss more changes moving forward, but this won't work to merge into Hyperspace