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

Remove "context saved" expectation from CoreDataStack mocks #18485

Merged
merged 2 commits into from
May 3, 2022
Merged

Remove "context saved" expectation from CoreDataStack mocks #18485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98dc3b7
Remove "context saved" expectation from CoreDataStack mocks
crazytonyli Apr 30, 2022
c6873f2
Use `testContextManager` instead of `ContextManager.sharedInstance`
crazytonyli May 2, 2022
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
1 change: 0 additions & 1 deletion WordPress/WordPressTest/AccountServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ class AccountServiceTests: XCTestCase {
super.setUp()

contextManager = TestContextManager()
contextManager.requiresTestExpectation = false
accountService = AccountService(managedObjectContext: contextManager.mainContext)
}

Expand Down
1 change: 0 additions & 1 deletion WordPress/WordPressTest/AtomicAuthenticationServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ class AtomicAuthenticationServiceTests: XCTestCase {
super.setUp()

contextManager = TestContextManager()
contextManager.requiresTestExpectation = false

let api = WordPressComRestApi(oAuthToken: "")
let remote = AtomicAuthenticationServiceRemote(wordPressComRestApi: api)
Expand Down
20 changes: 8 additions & 12 deletions WordPress/WordPressTest/BlogJetpackTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,17 @@ - (void)testJetpackUsername {
}

- (void)testJetpackSetupDoesntReplaceDotcomAccount {
XCTestExpectation *saveExpectation = [self expectationWithDescription:@"Context save expectation"];
self.testContextManager.testExpectation = saveExpectation;
XCTestExpectation *saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];

AccountService *accountService = [[AccountService alloc] initWithManagedObjectContext:[ContextManager sharedInstance].mainContext];
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 it's surprising to see the expectation using self.textContextManager.mainContext and this using [ContextManager sharedInstance].mainContext.

They look like different contexts. Or am I missing something?

For reference, here's where each is instantiated (I think)

For ContextManager:

WordPress-iOS/WordPress/Classes/Utility/ContextManager.m

Lines 72 to 83 in 112b352

- (NSManagedObjectContext *const)mainContext
{
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
NSManagedObjectContext *context = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSMainQueueConcurrencyType];
context.parentContext = self.writerContext;
self.mainContext = context;
[[[NullBlogPropertySanitizer alloc] initWithContext:context] sanitize];
});
return _mainContext;
}

For TestContextManager, which internally uses ContextManager:

WordPress-iOS/WordPress/WordPressTest/ContextManagerMock.m

Lines 70 to 80 in 112b352

- (NSManagedObjectContext *)mainContext
{
if (_mainContext) {
return _mainContext;
}
_mainContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSMainQueueConcurrencyType];
_mainContext.persistentStoreCoordinator = self.persistentStoreCoordinator;
return _mainContext;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! The code does look like they are referencing different context objects. But at runtime, they are actually the same one, the one in ContextManagerMock, since TestContextManager overrides the shared instance(the overriding actually happens within ContextManagerMock). Which is why self.testContextManager.mainContext is same as [[ContextManager sharedInstance] mainContext].

I will update the code to use the same reference for clarity.

BTW, the above overriding behaviour will be updated in my establishing usage pattern PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! I knew I must have been missing something, else I would have expected the tests to fail 😄

Thank you for making it explicit in c6873f2. 👍

WPAccount *wpComAccount = [accountService createOrUpdateAccountWithUsername:@"user" authToken:@"token"];
[self waitForExpectationsWithTimeout:2.0 handler:nil];
[self waitForExpectations:@[saveExpectation] timeout:2.0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this PR replaces all the waitForExpectations(timeout:, handler:) with wait(for:, timeout:).

What's your rationale for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this expectation was the only expectation created within this test method. This change is just making that explicit - test method is waiting for main context to be saved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. One could argue that using waitForExpectations(timeout:, handler:) would make for smaller diffs, because if we add new expectations that method will automatically wait for them.

But, I'm with you on the value of explicitly stating which expectation we're waiting for. 👍

WPAccount * defaultAccount = [accountService defaultWordPressComAccount];
XCTAssertEqualObjects(wpComAccount, defaultAccount);

saveExpectation = [self expectationWithDescription:@"Context save expectation"];
self.testContextManager.testExpectation = saveExpectation;
saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];
[accountService createOrUpdateAccountWithUsername:@"test1" authToken:@"token1"];
[self waitForExpectationsWithTimeout:2.0 handler:nil];
[self waitForExpectations:@[saveExpectation] timeout:2.0];
defaultAccount = [accountService defaultWordPressComAccount];
XCTAssertEqualObjects(wpComAccount, defaultAccount);
}
Expand All @@ -101,8 +99,7 @@ - (void)testWPCCShouldntDuplicateBlogs {
statusCode:200 headers:@{@"Content-Type":@"application/json"}];
}];

XCTestExpectation *saveExpectation = [self expectationWithDescription:@"Context save expectation"];
self.testContextManager.testExpectation = saveExpectation;
XCTestExpectation *saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];

AccountService *accountService = [[AccountService alloc] initWithManagedObjectContext:self.testContextManager.mainContext];
BlogService *blogService = [[BlogService alloc] initWithManagedObjectContext:self.testContextManager.mainContext];
Expand Down Expand Up @@ -130,7 +127,7 @@ - (void)testWPCCShouldntDuplicateBlogs {
};

// Wait on the merge to be completed
[self waitForExpectationsWithTimeout:2.0 handler:nil];
[self waitForExpectations:@[saveExpectation] timeout:2.0];

// test.blog + wp.com + jetpack
XCTAssertEqual(1, [accountService numberOfAccounts]);
Expand Down Expand Up @@ -174,8 +171,7 @@ - (void)testSyncBlogsMigratesJetpackSSL
statusCode:200 headers:@{@"Content-Type":@"application/json"}];
}];

XCTestExpectation *saveExpectation = [self expectationWithDescription:@"Context save expectation"];
self.testContextManager.testExpectation = saveExpectation;
XCTestExpectation *saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];

AccountService *accountService = [[AccountService alloc] initWithManagedObjectContext:self.testContextManager.mainContext];
BlogService *blogService = [[BlogService alloc] initWithManagedObjectContext:self.testContextManager.mainContext];
Expand All @@ -192,7 +188,7 @@ - (void)testSyncBlogsMigratesJetpackSSL
jetpackBlog.url = @"https://jetpack.example.com/";

// Wait on the merge to be completed
[self waitForExpectationsWithTimeout:2.0 handler:nil];
[self waitForExpectations:@[saveExpectation] timeout:2.0];

XCTAssertEqual(1, [accountService numberOfAccounts]);
// test.blog + wp.com + jetpack (legacy)
Expand Down
2 changes: 0 additions & 2 deletions WordPress/WordPressTest/ContextManagerMock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ NS_ASSUME_NONNULL_BEGIN
@property (nonatomic, readwrite, strong) NSManagedObjectModel *managedObjectModel;
@property (nonatomic, readwrite, strong) NSPersistentStoreCoordinator *persistentStoreCoordinator;
@property (nonatomic, readonly, strong) NSPersistentStoreCoordinator *standardPSC;
@property (nonatomic, readwrite, assign) BOOL requiresTestExpectation;
@property (nonatomic, readonly, strong) NSURL *storeURL;
@property (nonatomic, nullable, readwrite, strong) XCTestExpectation *testExpectation;
@end

/**
Expand Down
38 changes: 4 additions & 34 deletions WordPress/WordPressTest/ContextManagerMock.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ @implementation ContextManagerMock
@synthesize persistentStoreCoordinator = _persistentStoreCoordinator;
@synthesize mainContext = _mainContext;
@synthesize managedObjectModel = _managedObjectModel;
@synthesize requiresTestExpectation = _requiresTestExpectation;
@synthesize testExpectation = _testExpectation;

- (instancetype)init
{
Expand All @@ -23,7 +21,6 @@ - (instancetype)init
// Override the shared ContextManager
[ContextManager internalSharedInstance];
[ContextManager overrideSharedInstance:self];
_requiresTestExpectation = YES;
}

return self;
Expand Down Expand Up @@ -79,40 +76,13 @@ - (NSManagedObjectContext *)mainContext
return _mainContext;
}

- (void)saveContext:(NSManagedObjectContext *)context
{
[self saveContext:context withCompletionBlock:^{
if (self.testExpectation) {
[self.testExpectation fulfill];
self.testExpectation = nil;
} else if (self.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
}];
}

- (void)saveContextAndWait:(NSManagedObjectContext *)context
{
[super saveContextAndWait:context];
if (self.testExpectation) {
[self.testExpectation fulfill];
self.testExpectation = nil;
} else if (self.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
}

- (void)saveContext:(NSManagedObjectContext *)context withCompletionBlock:(void (^)(void))completionBlock
{
[super saveContext:context withCompletionBlock:^{
if (self.testExpectation) {
[self.testExpectation fulfill];
self.testExpectation = nil;
} else if (self.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
completionBlock();
}];
// FIXME: Remove this method to use superclass one instead
// This log magically resolves a deadlock in
// `ZDashboardCardTests.testShouldNotShowQuickStartIfDefaultSectionIsSiteMenu`
NSLog(@"Context save completed");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still trying to figure out cause of this deadlock 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

For what is worth, I can see the deadlock without this line on my end, too. The test you mention just sits there with a spinner next to it in the test navigator.

}

- (NSURL *)storeURL
Expand Down
21 changes: 10 additions & 11 deletions WordPress/WordPressTest/NotificationSyncMediatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ class NotificationSyncMediatorTests: XCTestCase {
XCTAssert(manager.mainContext.countObjects(ofType: Notification.self) == 0)

// CoreData Expectations
manager.testExpectation = expectation(description: "Context save expectation")

let contextSaved = expectation(forNotification: .NSManagedObjectContextDidSave, object: manager.mainContext)

// Mediator Expectations
let expect = expectation(description: "Sync")
Expand All @@ -74,7 +73,7 @@ class NotificationSyncMediatorTests: XCTestCase {
expect.fulfill()
}

waitForExpectations(timeout: timeout, handler: nil)
wait(for: [contextSaved, expect], timeout: timeout)
}


Expand Down Expand Up @@ -129,7 +128,7 @@ class NotificationSyncMediatorTests: XCTestCase {
XCTAssert(manager.mainContext.countObjects(ofType: Notification.self) == 0)

// CoreData Expectations
manager.testExpectation = expectation(description: "Context save expectation")
let contextSaved = expectation(forNotification: .NSManagedObjectContextDidSave, object: manager.mainContext)

// Mediator Expectations
let expect = expectation(description: "Sync")
Expand All @@ -141,7 +140,7 @@ class NotificationSyncMediatorTests: XCTestCase {
expect.fulfill()
}

waitForExpectations(timeout: timeout, handler: nil)
wait(for: [contextSaved, expect], timeout: timeout)
}


Expand All @@ -161,7 +160,7 @@ class NotificationSyncMediatorTests: XCTestCase {
XCTAssertFalse(note.read)

// CoreData Expectations
manager.testExpectation = expectation(description: "Context save expectation")
let contextSaved = expectation(forNotification: .NSManagedObjectContextDidSave, object: manager.mainContext)

// Mediator Expectations
let expect = expectation(description: "Mark as Read")
Expand All @@ -172,7 +171,7 @@ class NotificationSyncMediatorTests: XCTestCase {
expect.fulfill()
}

waitForExpectations(timeout: timeout, handler: nil)
wait(for: [contextSaved, expect], timeout: timeout)
}

/// Verifies that Mark Notifications as Read effectively toggles a Notifications' read flag
Expand All @@ -197,7 +196,7 @@ class NotificationSyncMediatorTests: XCTestCase {
XCTAssertTrue(note2.read)

// CoreData Expectations
manager.testExpectation = expectation(description: "Context save expectation")
let contextSaved = expectation(forNotification: .NSManagedObjectContextDidSave, object: manager.mainContext)

// Mediator Expectations
let expect = expectation(description: "Mark as Read")
Expand All @@ -210,7 +209,7 @@ class NotificationSyncMediatorTests: XCTestCase {
expect.fulfill()
}

waitForExpectations(timeout: timeout, handler: nil)
wait(for: [contextSaved, expect], timeout: timeout)
}

/// Verifies that Mark Notifications as Read modifies only the specified notifications' read status
Expand All @@ -235,7 +234,7 @@ class NotificationSyncMediatorTests: XCTestCase {
XCTAssertTrue(note2.read)

// CoreData Expectations
manager.testExpectation = expectation(description: "Context save expectation")
let contextSaved = expectation(forNotification: .NSManagedObjectContextDidSave, object: manager.mainContext)

// Mediator Expectations
let expect = expectation(description: "Mark as Read")
Expand All @@ -247,7 +246,7 @@ class NotificationSyncMediatorTests: XCTestCase {
expect.fulfill()
}

waitForExpectations(timeout: timeout, handler: nil)
wait(for: [contextSaved, expect], timeout: timeout)
}


Expand Down
2 changes: 0 additions & 2 deletions WordPress/WordPressTest/TestContextManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ NS_ASSUME_NONNULL_BEGIN
@property (nonatomic, readwrite, strong) NSManagedObjectModel *managedObjectModel;
@property (nonatomic, readwrite, strong) NSPersistentStoreCoordinator *persistentStoreCoordinator;
@property (nonatomic, readonly, strong) NSPersistentStoreCoordinator *standardPSC;
@property (nonatomic, readwrite, assign) BOOL requiresTestExpectation;
@property (nonatomic, readonly, strong) NSURL *storeURL;
@property (nonatomic, nullable, readwrite, strong) XCTestExpectation *testExpectation;
@property (nonatomic, strong, nullable) id<ManagerMock, CoreDataStack> stack;


Expand Down
31 changes: 2 additions & 29 deletions WordPress/WordPressTest/TestContextManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ - (instancetype)init
if (self) {
// Override the shared ContextManager
_stack = [[ContextManagerMock alloc] init];
_requiresTestExpectation = YES;
}

return self;
Expand Down Expand Up @@ -55,45 +54,19 @@ - (void)setMainContext:(NSManagedObjectContext *)mainContext
[_stack setMainContext:mainContext];
}

-(void)setTestExpectation:(XCTestExpectation *)testExpectation
{
[_stack setTestExpectation:testExpectation];
}

- (void)saveContext:(NSManagedObjectContext *)context
{
[self saveContext:context withCompletionBlock:^{
if (self.stack.testExpectation) {
[self.stack.testExpectation fulfill];
self.stack.testExpectation = nil;
} else if (self.stack.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
}];
[_stack saveContext:context];
}

- (void)saveContextAndWait:(NSManagedObjectContext *)context
{
[_stack saveContextAndWait:context];
if (self.stack.testExpectation) {
[self.stack.testExpectation fulfill];
self.stack.testExpectation = nil;
} else if (self.stack.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
}

- (void)saveContext:(NSManagedObjectContext *)context withCompletionBlock:(void (^)(void))completionBlock
{
[_stack saveContext:context withCompletionBlock:^{
if (self.stack.testExpectation) {
[self.stack.testExpectation fulfill];
self.stack.testExpectation = nil;
} else if (self.stack.requiresTestExpectation) {
NSLog(@"No test expectation present for context save");
}
completionBlock();
}];
[_stack saveContext:context withCompletionBlock:completionBlock];
}

- (nonnull NSManagedObjectContext *const)newDerivedContext {
Expand Down