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

implements new Storage API for handleEventsForBackgroundURLSession #2481

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brennanMKE
Copy link
Contributor

Issue #, if available:

N/A

Description of changes:

Check points: (check or cross out if not relevant)

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@brennanMKE brennanMKE added the storage Issues related to the Storage category label Oct 19, 2022
@brennanMKE brennanMKE requested a review from a team as a code owner October 19, 2022 19:33
@brennanMKE brennanMKE self-assigned this Oct 19, 2022
@@ -7,30 +7,44 @@

import Foundation

extension Notification.Name {
Copy link
Member

Choose a reason for hiding this comment

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

This notification was setup just for unit test? Looks like this will be fired in the production app as well. We should think about a better way to make this class testable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other options do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

The test worked without the notification expectation:

    func testRegisteringAndUnregister() async throws {
        let identifier = UUID().uuidString
        let otherIdentifier = UUID().uuidString
        await StorageBackgroundEventsRegistry.shared.register(identifier: identifier)

        let done = asyncExpectation(description: "done")
        Task {
            let handled = await StorageBackgroundEventsRegistry.shared.handleEventsForBackgroundURLSession(identifier: identifier)
            await done.fulfill()
            XCTAssertTrue(handled)
        }

        let didContinue = await handleEvents(for: identifier)
        XCTAssertTrue(didContinue)
        await waitForExpectations([done])

        let otherDone = asyncExpectation(description: "other done")

        Task {
            let otherHandled = await StorageBackgroundEventsRegistry.shared.handleEventsForBackgroundURLSession(identifier: otherIdentifier)
            await otherDone.fulfill()
            XCTAssertFalse(otherHandled)
        }

        let didNotContinue = await handleEvents(for: otherIdentifier)
        XCTAssertFalse(didNotContinue)
        await waitForExpectations([otherDone])
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to run it on macOS many times repeatedly to get it to fail. Without the notification it is a flaky test which will fail based on timing.

Copy link
Member

Choose a reason for hiding this comment

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

This issue can then happen in the real app as well right? What if the URLSession call back reach before we setup the continuation?

Copy link
Member

@royjit royjit Oct 31, 2022

Choose a reason for hiding this comment

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

Another option could be to create an overloaded function just for test:

    func handleEventsForBackgroundURLSession(identifier: String) async -> Bool {
        return await handleEventsForBackgroundURLSession(identifier: identifier, notifyTest: nil)
    }

    func handleEventsForBackgroundURLSession(identifier: String, notifyTest: ((String) -> Void)? ) async -> Bool {
        guard self.identifier == identifier else { return false }
        return await withCheckedContinuation { (continuation: CheckedContinuation<Bool, Never>) in
            self.continuation = continuation
            if let notifyTest = notifyTest {
                notifyTest(identifier)
            }
        }
    }

@akshaaatt
Copy link

akshaaatt commented Mar 10, 2023

When can we expect this to be released?

@brennanMKE brennanMKE removed their assignment Mar 28, 2023
@jcjimenez jcjimenez removed their assignment Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage Issues related to the Storage category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants