Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[MOB-10402] Fix Tests #876

Merged
merged 141 commits into from
Mar 14, 2025
Merged

[MOB-10402] Fix Tests #876

merged 141 commits into from
Mar 14, 2025

Conversation

sumeruchat
Copy link
Contributor

@sumeruchat sumeruchat commented Dec 13, 2024

🔹 Jira Ticket(s)

✏️ Description

Add nice reports for tests

Example https://github.com/Iterable/iterable-swift-sdk/actions/runs/12316457995

@joaodordio
Copy link
Member

@sumeruchat should we remove the other changes other than the part that add the report?

@sumeruchat
Copy link
Contributor Author

@joaodordio Yeah sure I can undo those but those make sense rite? Should i add that in a seperate PR?

@joaodordio
Copy link
Member

@joaodordio Yeah sure I can undo those but those make sense rite? Should i add that in a seperate PR?

absolutely, I've added those here: #872

@sumeruchat
Copy link
Contributor Author

yeah once you merge your branch i will rebase

@sumeruchat sumeruchat force-pushed the feature/MOB-10402-add-tests branch from 3bb2f30 to 7b8e280 Compare December 19, 2024 14:57
@sumeruchat sumeruchat changed the title [MOB-10402] Add test reports [MOB-10402] Fix Tests Mar 3, 2025
@sumeruchat
Copy link
Contributor Author

@joaodordio @evantk91 @Ayyanchira Please note that the following tests are disabled

    "AuthTests\/testAsyncAuthTokenRetrieval()",
    "AuthTests\/testAuthTokenChangeWithSameEmail()",
    "AuthTests\/testAuthTokenChangeWithSameUserId()",
    "AuthTests\/testAuthTokenDeletedOnLogout()",
    "AuthTests\/testAuthTokenRetrievalFailureReset()",
    "AuthTests\/testEmailWithTokenPersistence()",
    "AuthTests\/testLogoutUser()",
    "AuthTests\/testNewEmailAndThenChangeToken()",
    "AuthTests\/testNewUserIdAndThenChangeToken()",
    "AuthTests\/testOnNewAuthTokenCallbackCalled()",
    "AuthTests\/testPushRegistrationAfterAuthTokenRetrieval()",
    "AuthTests\/testRefreshTimerQueueRejection()",
    "AuthTests\/testRetryJwtFailure()",
    "AuthTests\/testUpdateEmailAndThenChangeToken()",
    "AuthTests\/testUpdateEmailWithTokenParam()",
    "AuthTests\/testUserIdWithTokenPersistence()",
    "AutoRegistrationTests\/testCallDisableAndEnable()",
    "RequestCreatorTests\/testRegisterTokenRequestPrefersUserId()"
    
    We need to make a ticket to investigate why these are failing.

@joaodordio
Copy link
Member

@joaodordio @evantk91 @Ayyanchira Please note that the following tests are disabled

    "AuthTests\/testAsyncAuthTokenRetrieval()",
    "AuthTests\/testAuthTokenChangeWithSameEmail()",
    "AuthTests\/testAuthTokenChangeWithSameUserId()",
    "AuthTests\/testAuthTokenDeletedOnLogout()",
    "AuthTests\/testAuthTokenRetrievalFailureReset()",
    "AuthTests\/testEmailWithTokenPersistence()",
    "AuthTests\/testLogoutUser()",
    "AuthTests\/testNewEmailAndThenChangeToken()",
    "AuthTests\/testNewUserIdAndThenChangeToken()",
    "AuthTests\/testOnNewAuthTokenCallbackCalled()",
    "AuthTests\/testPushRegistrationAfterAuthTokenRetrieval()",
    "AuthTests\/testRefreshTimerQueueRejection()",
    "AuthTests\/testRetryJwtFailure()",
    "AuthTests\/testUpdateEmailAndThenChangeToken()",
    "AuthTests\/testUpdateEmailWithTokenParam()",
    "AuthTests\/testUserIdWithTokenPersistence()",
    "AutoRegistrationTests\/testCallDisableAndEnable()",
    "RequestCreatorTests\/testRegisterTokenRequestPrefersUserId()"
    
    We need to make a ticket to investigate why these are failing.

Some of these seem rather important to be skipping right? They also look like simple tests — meaning they shouldn't be failing right?

Do we know what's behind the failure before we commit to skipping them?

Also, should we remove the codecov.io step? We are not using that anymore.

@Ayyanchira
Copy link
Member

Yes Code cov may be worked on when working on reporting tools. But I agree its important to document why those are failing before skipping them

@sumeruchat
Copy link
Contributor Author

I fixed some more tests the final list of skipped tests is now

From unit-tests:
AuthTests.testOnNewAuthTokenCallbackCalled()
AuthTests.testRetryJwtFailure()
InAppTests.testMultipleMesssagesInShortTime()
IterableAPIResponseTests.testRetryOnInvalidJwtPayload()
From offline-events-tests:
HealthMonitorTests.testDeleteAllTasksException()
RequestHandlerTests.testDeleteAllTasksOnLogout()
TaskSchedulerTests.testScheduleTask()

@sumeruchat
Copy link
Contributor Author

Lets look at code coverage in the CI reporting task. Here lets focus on fixing tests. It would be awesome if you guys can help fixing the remaining tests. @Ayyanchira @joaodordio @evantk91

Copy link
Member

@Ayyanchira Ayyanchira left a comment

Choose a reason for hiding this comment

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

Looking good. Authmanager change does make sense. There will not be success handle called if auth decode fails. But at the same time, scheduleing the next auth token is fired.
Looks good to me.

Comment on lines +153 to 166
// Set the new token first
authToken = retrievedAuthToken
storeAuthToken()

if retrievedAuthToken != nil {
let isRefreshQueued = queueAuthTokenExpirationRefresh(retrievedAuthToken, onSuccess: onSuccess)
if !isRefreshQueued {
onSuccess?(authToken)
onSuccess?(retrievedAuthToken) // Use retrievedAuthToken instead of authToken
}
} else {
handleAuthFailure(failedAuthToken: nil, reason: .authTokenNull)
scheduleAuthTokenRefreshTimer(interval: getNextRetryInterval(), successCallback: onSuccess)
}

authToken = retrievedAuthToken

storeAuthToken()
}
Copy link
Member

Choose a reason for hiding this comment

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

This actually makes sense. Might have to match this one in Android later too if thats not happening..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. We should add to Android.

Comment on lines +81 to +93
// Compare each field individually for better error messages
let requestDevice = requestBody["device"] as! [String: Any]
let requestDataFields = requestDevice["dataFields"] as! [String: Any]
let expectedDevice = deviceDict
let expectedDataFields = dataFields

XCTAssertEqual(requestDevice["token"] as? String, expectedDevice["token"] as? String, "token mismatch")
XCTAssertEqual(requestDevice["applicationName"] as? String, expectedDevice["applicationName"] as? String, "applicationName mismatch")
XCTAssertEqual(requestDevice["platform"] as? String, expectedDevice["platform"] as? String, "platform mismatch")

for (key, value) in expectedDataFields {
XCTAssertEqual(requestDataFields[key] as? String, value as? String, "\(key) mismatch")
}
Copy link
Member

Choose a reason for hiding this comment

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

Like this refactor for being able to tell in detail which exact detail is problematic

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like body dict is never used and can be removed due to this refactor.

Comment on lines +104 to +149
let xpectation = expectation(description: "retry on 401 with invalidJWTPayload")

// Mock the dependencies and requestProvider for your test
let authManager = MockAuthManager()

let networkErrorSession = MockNetworkSession() { _ in
MockNetworkSession.MockResponse(statusCode: 401,
data: ["code":"InvalidJwtPayload"].toJsonData(),
delay: 1)
}

let networkSuccessSession = MockNetworkSession() { _ in
MockNetworkSession.MockResponse(statusCode: 200,
data: ["msg": "success"].toJsonData(),
delay: 1)
}

let urlErrorRequest = createApiClient(networkSession: networkErrorSession).convertToURLRequest(iterableRequest: IterableRequest.post(PostRequest(path: "", args: nil, body: [:])))!


let urlSuccessRequest = createApiClient(networkSession: networkSuccessSession).convertToURLRequest(iterableRequest: IterableRequest.post(PostRequest(path: "", args: nil, body: [:])))!

let requestProvider: () -> Pending<SendRequestValue, SendRequestError> = {
if authManager.retryWasRequested {
return RequestSender.sendRequest(urlSuccessRequest, usingSession: networkSuccessSession)
}
return RequestSender.sendRequest(urlErrorRequest, usingSession: networkErrorSession)
}

let result = RequestProcessorUtil.sendRequest(
requestProvider: requestProvider,
authManager: authManager,
requestIdentifier: "TestIdentifier"
)

result.onSuccess { value in
xpectation.fulfill()
XCTAssert(true)
}.onError { error in
if authManager.retryWasRequested {
xpectation.fulfill()
}
}

waitForExpectations(timeout: testExpectationTimeout)
}
Copy link
Member

Choose a reason for hiding this comment

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

Amazing test case for JWT retry!

@@ -180,13 +180,13 @@ class AuthManager: IterableAuthManagerProtocol {
/// schedule a default timer of 10 seconds if we fall into this case
scheduleAuthTokenRefreshTimer(interval: getNextRetryInterval(), successCallback: onSuccess)

return true
return false // Return false since we couldn't queue a valid refresh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Collaborator

@evantk91 evantk91 left a comment

Choose a reason for hiding this comment

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

Let some clarifying comments.

@@ -50,46 +77,46 @@ class NetworkConnectivityManagerTests: XCTestCase {
func testPollingNetworkMonitor() throws {
let expectation1 = expectation(description: "do not fulfill before start")
expectation1.isInverted = true
let monitor = NetworkMonitor()
let monitor = MockNetworkMonitor()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe testNetworkMonitor is missing the MockNetworkMonitor

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the time out period needs to be updated to align with other tests

Comment on lines +81 to +93
// Compare each field individually for better error messages
let requestDevice = requestBody["device"] as! [String: Any]
let requestDataFields = requestDevice["dataFields"] as! [String: Any]
let expectedDevice = deviceDict
let expectedDataFields = dataFields

XCTAssertEqual(requestDevice["token"] as? String, expectedDevice["token"] as? String, "token mismatch")
XCTAssertEqual(requestDevice["applicationName"] as? String, expectedDevice["applicationName"] as? String, "applicationName mismatch")
XCTAssertEqual(requestDevice["platform"] as? String, expectedDevice["platform"] as? String, "platform mismatch")

for (key, value) in expectedDataFields {
XCTAssertEqual(requestDataFields[key] as? String, value as? String, "\(key) mismatch")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like body dict is never used and can be removed due to this refactor.

@@ -185,12 +185,14 @@ class TaskRunnerTests: XCTestCase {
func testResumeWhenNetworkIsBackOnline() throws {
let networkSession = MockNetworkSession(statusCode: 401, json: [:], error: IterableError.general(description: "Mock error"))
let checker = NetworkConnectivityChecker(networkSession: networkSession)
let monitor = NetworkMonitor()
let monitor = MockNetworkMonitor()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are only specific monitors the MockNetworkMonitor?


wait(for: [expectation1, expectation2, expectation3], timeout: testExpectationTimeout)

XCTAssertEqual(registerCallCount, 2, "Should have exactly 2 register calls")
XCTAssertTrue(disableCallMade, "Should have made exactly 1 disable call")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice updates! I like the testing the disable can only be called after register and only once.

value: "native",
inDictionary: body)
TestUtils.validateMatch(keyPath: KeyPath(string: "mobileFrameworkInfo.iterableSdkVersion"),
TestUtils.validateMatch(keyPath: KeyPath(keys: JsonKey.device, JsonKey.dataFields, JsonKey.mobileFrameworkInfo, JsonKey.iterableSdkVersion),
value: testSdkVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice addition.

@sumeruchat sumeruchat merged commit 31f8629 into master Mar 14, 2025
2 checks passed
@sumeruchat sumeruchat deleted the feature/MOB-10402-add-tests branch March 14, 2025 10:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants