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

Fix iOS 15 tests #4320

Merged
merged 7 commits into from
Sep 30, 2024
Merged

Fix iOS 15 tests #4320

merged 7 commits into from
Sep 30, 2024

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Sep 27, 2024

This pull request includes changes to the MockOperationDispatcher class in the Tests/UnitTests/Mocks/MockOperationDispatcher.swift file to improve the handling of asynchronous tasks. The most important change is the replacement of a semaphore with a dispatch group to ensure the async task completes before proceeding.

It looks like the semaphore was causing issues in iOS 15, making the test forever

@vegaro vegaro requested review from joshdholtz and a team September 27, 2024 17:11
@vegaro

This comment was marked as outdated.

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Looks good to me if it works 😇 Thanks for fixing! 🫶

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Ohh interesting... didn't notice the issues with the semaphore. Thanks for fixing!

@vegaro

This comment was marked as outdated.


self.mockDiagnosticsTracker = MockDiagnosticsTracker()
}

func testFetchProductsWithIdentifiersSK2TracksCorrectly() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isStoreKit2Available is always false for iOS < 16, so this test was running with storeKit1 even after configuring the manager using SK2

@vegaro
Copy link
Contributor Author

vegaro commented Sep 30, 2024

@RCGitBot please test

@vegaro vegaro changed the title Fix MockOperationDispatcher in iOS 15 Fix iOS 15 tests Sep 30, 2024
@vegaro vegaro requested review from joshdholtz and a team September 30, 2024 10:14
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Just a comment but looks good!

@@ -159,14 +159,12 @@ private extension DiagnosticsTracker {
func clearDiagnosticsFileIfTooBig() async {
if await self.diagnosticsFileHandler.isDiagnosticsFileTooBig() {
await self.diagnosticsFileHandler.emptyDiagnosticsFile()
self.trackMaxEventsStoredLimitReached()
await self.clearDiagnosticsFileIfTooBig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is this needed? I guess when calling this, since we just emptied the file in the line above, it shouldn't do anything, but shouldn't be needed normally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not needed right. I forgot to remove this line when copying the content of track

@vegaro vegaro enabled auto-merge (squash) September 30, 2024 10:20
@vegaro vegaro merged commit 37e197b into main Sep 30, 2024
5 checks passed
@vegaro vegaro deleted the fix-ios-15 branch September 30, 2024 10:29
nyeu pushed a commit that referenced this pull request Oct 2, 2024
MojtabaHs pushed a commit to MojtabaHs/purchases-ios that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants