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

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

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

Crebs
Copy link
Contributor

@Crebs Crebs commented Feb 4, 2025

Resolve failing unit tests caused by handling of strongSelf in the accessTokenForRefresh:completion: method. Ensure the completion handler executes safely when self is deallocated, improving test stability and robustness of asynchronous operations.

@Crebs Crebs requested a review from bbirman February 4, 2025 18:50
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.22%. Comparing base (892e836) to head (294c98f).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
...KCore/SalesforceSDKCore/Classes/Util/SFSDKOAuth2.m 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3827      +/-   ##
==========================================
+ Coverage   60.70%   61.22%   +0.51%     
==========================================
  Files         233      233              
  Lines       21703    21710       +7     
==========================================
+ Hits        13175    13291     +116     
+ Misses       8528     8419     -109     
Components Coverage Δ
Analytics 68.39% <ø> (ø)
Common 68.87% <ø> (ø)
Core 49.93% <83.33%> (+0.84%) ⬆️
SmartStore 73.70% <ø> (ø)
MobileSync 87.58% <ø> (ø)
Files with missing lines Coverage Δ
...KCore/SalesforceSDKCore/Classes/Util/SFSDKOAuth2.m 44.17% <83.33%> (+1.22%) ⬆️

... and 9 files with indirect coverage changes

}

[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

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

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

@Crebs Crebs requested a review from bbirman February 4, 2025 21:37
@Crebs Crebs merged commit 6b729d7 into forcedotcom:dev Feb 4, 2025
9 checks passed
@Crebs Crebs deleted the @W-17749645 branch February 4, 2025 23:26
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.

4 participants