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

fixes tests for AsyncChannel which fail on iOS #186

Closed
wants to merge 3 commits into from

Conversation

brennanMKE
Copy link

The original expectations features in XCTests are not fully compatible with iOS. The tests for AsyncChannel pass with macOS but not iOS.

This PR includes a new type called AsyncExpectation which provides an async version of XCTestExpectation which has the features used in these tests. Migrating to this type required only small changes. AsyncTesting is a new package with this support for testing which includes tests for this functionality.

With these changes the tests pass quickly on macOS and iOS.

@twittemb
Copy link
Contributor

Hi

Shouldn’t that be part of the XCTest lib ? I guess this issue will happen for a lot of people out there.

@phausler

@phausler
Copy link
Member

This seems like perhaps a bug in XCTest if it passes on macOS but not iOS. @briancroom are the existing XCTest expectations not a supported mechanism here?

@CassiusPacheco
Copy link

I haven't tried this particular piece of code, but I've experienced similar issues with my packages where a test method that's marked as async but does not perform any await call, but instead, is relying on the XCTWaiter's wait method, causes the test to pass on macOS but fail on iOS. The "fix" is to remove the unnecessary/unused async declaration of the method.

Looking at the code change proposed, you wrapped the XCTWaiter's wait in an async/await API and are then calling await on this API in the test which, if my theory is correct, is why the tests now pass.

It does look like a bug on XCTest expectations

@FranzBusch
Copy link
Member

This might be caused due to the fact that the iOS simulator uses a cooperative thread pool size of 1. Using test expectations is then blocking this one thread probably which ends up wedging the test. (I haven't looked deeper into this just thought that this might be the reason)

@brennanMKE
Copy link
Author

This might be caused due to the fact that the iOS simulator uses a cooperative thread pool size of 1. Using test expectations is then blocking this one thread probably which ends up wedging the test. (I haven't looked deeper into this just thought that this might be the reason)

This appears to be the difference. Using a single expectation appears to work on iOS but more than 1 causes failures on iOS. The only async waitForExpectations does not support specifying expectations so it waits on all expectations which are defined to that point. This limits the flexibility of tests. What I've introduced used Swift Concurrency mechanisms so it does not violate the forward progress contract. Adding this type of support to XCTest would be my recommendation.

@brennanMKE
Copy link
Author

@FranzBusch What is causing the iOS simulator to have a thread pool size of 1? I do not see LIBDISPATCH_COOPERATIVE_POOL_STRICT set anywhere in the package.

@FranzBusch
Copy link
Member

@FranzBusch What is causing the iOS simulator to have a thread pool size of 1? I do not see LIBDISPATCH_COOPERATIVE_POOL_STRICT set anywhere in the package.

I think this is just how the iOS simulator is setup right now. I don't know any details why this is the case though.

* moves test support code into XCTest target
* improves implementation of AsyncExpectation
* extends XCTestCase to add async functions
* revises TestChannel tests with improved support code
@brennanMKE
Copy link
Author

I've revised the implementation of the test support to correct some logic bugs and also extended XCTestCase so functions can be used inline so code changes which deviate from XCTest are minimal.

Tests pass on macOS and iOS when run once and also repeatedly 100 times.

@FranzBusch
Copy link
Member

I am closing this PR for now since AsyncChannel has since been reimplemented.

@FranzBusch FranzBusch closed this Sep 21, 2023
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.

None yet

5 participants