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.

Sign up for GitHub

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

@W-17749645 Resolve failing unit tests caused by handling of strongSelf #3827

Merged
merged 5 commits into from
Feb 4, 2025
Merged

@W-17749645 Resolve failing unit tests caused by handling of strongSelf #3827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f651ed3
Resolve failing unit tests caused by handling of strongSelf
Crebs Feb 4, 2025
41c4356
adding back tests that were flappy
Crebs Feb 4, 2025
5d5b473
Remove flappy test
Crebs Feb 4, 2025
9740394
Remove flappy test
Crebs Feb 4, 2025
294c98f
Remove flappy test
Crebs Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions libs/SalesforceSDKCore/SalesforceSDKCore/Classes/Util/SFSDKOAuth2.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ - (void)accessTokenForRefresh:(SFSDKOAuthTokenEndpointRequest *)endpointReq comp
NSURLSession *session = [self createURLSessionWithIdentifier:instanceIdentifier];

__weak typeof(self) weakSelf = self;
NSString *className = NSStringFromClass([self class]);
[[session dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *urlResponse, NSError *error) {
__strong typeof(weakSelf) strongSelf = weakSelf;
SFSDKOAuthTokenEndpointResponse *endpointResponse = nil;
Expand All @@ -293,21 +294,29 @@ - (void)accessTokenForRefresh:(SFSDKOAuthTokenEndpointRequest *)endpointReq comp
endpointResponse = [[SFSDKOAuthTokenEndpointResponse alloc] initWithError:[NSError errorWithDomain:kSFOAuthErrorDomain code:code userInfo:nil]];

if (error.code == NSURLErrorTimedOut) {
[SFSDKCoreLogger d:[strongSelf class] format:@"Refresh attempt timed out after %f seconds.", endpointReq.timeout];
[SFSDKCoreLogger d:[SFSDKOAuth2 class] format:@"Refresh attempt timed out after %f seconds.", endpointReq.timeout];
}

[SFSDKCoreLogger d:[strongSelf class] format:@"SFOAuth2 session failed with error: error code: %ld, description: %@, URL: %@", (long)error.code, [error localizedDescription], errorUrlString];
[SFSDKCoreLogger d:[SFSDKOAuth2 class] format:@"SFOAuth2 session failed with error: error code: %ld, description: %@, URL: %@", (long)error.code, [error localizedDescription], errorUrlString];
dispatch_async(dispatch_get_main_queue(), ^{
if (completionBlock) {
completionBlock(endpointResponse);
}
});
return;
} else {
[SFSDKEventBuilderHelper createAndStoreEvent:@"tokenRefresh" userAccount:[SFUserAccountManager sharedInstance].currentUser className:NSStringFromClass([strongSelf class]) attributes:nil];
}

[strongSelf handleTokenEndpointResponse:completionBlock request:endpointReq data:data urlResponse:urlResponse];
[SFSDKEventBuilderHelper createAndStoreEvent:@"tokenRefresh" userAccount:[SFUserAccountManager sharedInstance].currentUser className:className attributes:nil];
if (strongSelf) {
Copy link
Member

Choose a reason for hiding this comment

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

What was the test failure this was causing?

Copy link
Contributor Author

@Crebs Crebs Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Everytime I'd run SalesforceSDKCore UnitTests, I was getting an Error that "a method should be called on main" . But after doing some debugging and tracking it down, it was because this we were setting a nil value into a dictionary during a test. Inside of SFSDKEventBuilderHelper line 57. The reason it was crashing every time was because a strongSelf was nil every time. The way the life cycle was set up for the test would cause self to be released and nil to be inserted in the dictionary. It was happening because of SalesforceSDKIdentityTests and TestSetupUtils synchronousAuthRefresh lifecycle. Let me know if you want sync up and chat about this too.

Copy link
Member

@bbirman bbirman Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a urlResponse when strongSelf is nil? Wondering if something is being deallocated too early
edit: oops missed the merge part

[strongSelf handleTokenEndpointResponse:completionBlock request:endpointReq data:data urlResponse:urlResponse];
} else {
[SFSDKCoreLogger d:[SFSDKOAuth2 class] format:@"Token endpoint response handler skipped because self was deallocated."];
dispatch_async(dispatch_get_main_queue(), ^{
if (completionBlock) {
completionBlock(nil);
}
});
}
}] resume];
}

Expand Down
3 changes: 1 addition & 2 deletions libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
let request = RestClient.shared.request(forQuery: "select name from CONTACT", apiVersion: nil)

var erroredResult: RestClientError?
RestClient.shared.fetchRecords(ofModelType: TestContact.self, forRequest: request) { result in

Check warning on line 56 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'fetchRecords(ofModelType:forRequest:withDecoder:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `fetchRecords(ofModelType:forRequest:withDecoder:)` instead.
switch (result) {
case .failure(let error):
erroredResult = error
Expand Down Expand Up @@ -263,8 +263,7 @@
}
}

// TODO: Add back after fixing Flappiness
func _testAsyncBatchRequestStopOnFailure() async throws {
func testAsyncBatchRequestStopOnFailure() async throws {
do {
// Create account
let accountName = self.generateRecordName()
Expand Down Expand Up @@ -325,7 +324,7 @@
var response: CompositeResponse?
var restClientError: Error?

RestClient.shared.send(compositeRequest: compositeRequest) { result in

Check warning on line 327 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(compositeRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(compositeRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down Expand Up @@ -384,7 +383,7 @@
var response: CompositeResponse?
var restClientError: Error?

RestClient.shared.send(compositeRequest: compositeRequest) { result in

Check warning on line 386 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(compositeRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(compositeRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down Expand Up @@ -421,7 +420,7 @@
var response: CompositeResponse?
var restClientError: Error?

RestClient.shared.send(compositeRequest: compositeRequest) { result in

Check warning on line 423 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(compositeRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(compositeRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down Expand Up @@ -462,7 +461,7 @@
var response: BatchResponse?
var restClientError: Error?

RestClient.shared.send(batchRequest: batchRequest) { result in

Check warning on line 464 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(batchRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(batchRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down Expand Up @@ -537,7 +536,7 @@
var response: BatchResponse?
var restClientError: Error?

RestClient.shared.send(batchRequest: batchRequest) { result in

Check warning on line 539 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(batchRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(batchRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down Expand Up @@ -593,7 +592,7 @@
var response: BatchResponse?
var restClientError: Error?

RestClient.shared.send(batchRequest: batchRequest) { result in

Check warning on line 595 in libs/SalesforceSDKCore/SalesforceSDKCoreTests/RestClientTest.swift

View workflow job for this annotation

GitHub Actions / ios-pr (SalesforceSDKCore) / test-ios 

'send(batchRequest:_:)' is deprecated: Deprecated in Salesforce Mobile SDK 13.0 and will be removed in Salesforce Mobile SDK 14.0. Use the async/await version of `send(batchRequest:)` instead.
defer { expectation.fulfill() }
switch (result) {
case .success(let resp):
Expand Down
3 changes: 1 addition & 2 deletions libs/SalesforceSDKCore/SalesforceSDKCoreTests/SalesforceOAuthUnitTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,7 @@ - (void)testCredentialsCoding {
credsIn = nil;
}

// TODO: Add back after fixing Flappiness
- (void)_testCredentialsCopying {
- (void)testCredentialsCopying {
NSString *domainToCheck = @"login.salesforce.com";
NSString *redirectUriToCheck = @"redirectUri://done";
NSString *jwtToCheck = @"jwtToken";
Expand Down
8 changes: 3 additions & 5 deletions libs/SalesforceSDKCore/SalesforceSDKCoreTests/SalesforceRestAPITests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ - (void)testCreateBogusContact {
// - delete object (requestForDeleteWithObjectType)
// - query new object (requestForQuery) and make sure we don't get anything
// - search new object (requestForSearch) and make sure we don't get anything
- (void)_testCreateQuerySearchDelete {
- (void)testCreateQuerySearchDelete {
// create object
//use a SOSL-safe format here to avoid problems with escaping characters for SOSL
NSString *lastName = [self generateRecordName];
Expand Down Expand Up @@ -1447,8 +1447,7 @@ - (void) testCollectionUpsertExistingRecords {
XCTAssertEqualObjects(accountsRetrieved[1][@"Name"], secondAccountNameUpdated);
}

// TODO: Add back after fixing Flappiness
- (void) _testCollectionUpdate {
- (void) testCollectionUpdate {
NSString* firstAccountName = [NSString stringWithFormat:@"%@_account_1_%lf", ENTITY_PREFIX_NAME, CFAbsoluteTimeGetCurrent()];
NSString* secondAccountName = [NSString stringWithFormat:@"%@_account_2_%lf", ENTITY_PREFIX_NAME, CFAbsoluteTimeGetCurrent()];
NSString* contactName = [NSString stringWithFormat:@"%@_contact_%lf", ENTITY_PREFIX_NAME, CFAbsoluteTimeGetCurrent()];
Expand Down Expand Up @@ -1937,8 +1936,7 @@ - (void)testUploadBatchDetailsDeleteFilesCommunity {
}

// Upload files / get owned files / delete files / get owned files again
// TODO: Add back after fixing Flappiness
- (void)_testUploadOwnedFilesDelete {
- (void)testUploadOwnedFilesDelete {

// upload first file
NSDictionary *fileAttrs = [self uploadFile];
Expand Down
Loading