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 all commits
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
22 changes: 9 additions & 13 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];
AccountService *accountService = [[AccountService alloc] initWithManagedObjectContext:self.testContextManager.mainContext];
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