Remove "context saved" expectation from CoreDataStack mocks #18485

crazytonyli · 2022-04-30T03:27:03Z

This PR is part of paaHJt-3ky-p2.

Changes

Remove testExpectation from TestContextManager and ContextManagerMock. This expectation is used to assert ContextManager.saveContext method are called. This logic is split into two areas: created in unit test method, but fulfilled outside of test method. This PR moves all this logic into unit test method, for better readability, it also simplifies CoreDataStack mock implementations.

Test Instructions

Make sure all unit tests pass.

Regression Notes

N/A. All changes in this PR are in unit test, they don't have any impact on the app.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

They are moved to test methods.

crazytonyli · 2022-04-30T03:28:21Z

WordPress/WordPressTest/ContextManagerMock.m

+    // FIXME: Remove this method to use superclass one instead
+    // This log magically resolves a deadlock in
+    // `ZDashboardCardTests.testShouldNotShowQuickStartIfDefaultSectionIsSiteMenu`
+    NSLog(@"Context save completed");


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

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.

mokagio

This is shaping up well, thank you @crazytonili!

The deadlock behavior, and it's resolution with a simple log, is quite baffling... 😳

I'd say there's something deeper down in the CoreData stack and/or how the Dashboard part of the app uses it. I'm saying this because the tests were explicitly prefixed with Z to let them run at the end.

See:

Also, BlogDashboardServiceTests explicitly mentions CoreData as a possible cause of issues:

WordPress-iOS/WordPress/WordPressTest/Dashboard/BlogDashboardServiceTests.swift

Lines 6 to 13 in 112b352

    
           /// This test stuite is clashing with other tests 
        
           /// Specifically: 
        
           /// - [BlogJetpackTest testJetpackSetupDoesntReplaceDotcomAccount] 
        
           /// - CommentServiceTests.testFailingFetchCommentLikesShouldCallFailureBlock() 
        
           /// 
        
           /// We weren't able to figure out why but it seems a race condition + Core data 
        
           /// For now, renaming the suite to change the execution order solves the issue. 
        
           class ZBlogDashboardServiceTests: XCTestCase {

You shared a possible fix and I love your initiative. But, I'd say let's focus on the test suite for the context of this trial because we want to keep it time constrained.

IMHO, having a FIXME comment plus NSLog is no worse than renaming tests to start with Z so they run last as a workaround.

mokagio · 2022-05-02T06:07:29Z

WordPress/WordPressTest/BlogJetpackTest.m


    AccountService *accountService = [[AccountService alloc] initWithManagedObjectContext:[ContextManager sharedInstance].mainContext];
    WPAccount *wpComAccount = [accountService createOrUpdateAccountWithUsername:@"user" authToken:@"token"];
-    [self waitForExpectationsWithTimeout:2.0 handler:nil];
+    [self waitForExpectations:@[saveExpectation] timeout:2.0];


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

What's your rationale for this change?

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.

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. 👍

mokagio · 2022-05-02T06:23:01Z

WordPress/WordPressTest/BlogJetpackTest.m

+    XCTestExpectation *saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];

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


🤔 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;

}

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.

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. 👍

mokagio · 2022-05-02T06:24:04Z

WordPress/WordPressTest/ContextManagerMock.m

+    // FIXME: Remove this method to use superclass one instead
+    // This log magically resolves a deadlock in
+    // `ZDashboardCardTests.testShouldNotShowQuickStartIfDefaultSectionIsSiteMenu`
+    NSLog(@"Context save completed");


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.

They are the same object in runtime, since `TestContextManager` overrides `ContextManager.sharedInstance` with a mock instance.

mokagio

I think this is now good to merge. Thank you @crazytonyli, and congrats on your first WordPress iOS contribution 😄

I updated #18488 so it will run the tests in CI for your latest changes. If that's green, we should be good to merge the PR 🎉

mokagio · 2022-05-03T04:35:07Z

@crazytonyli

If that's green, we should be good to merge the PR 🎉

Actually, as an external contributor, you wouldn't be able to merge the PR. I will enable auto-merge, so if/when the tests pass, this will land on trunk.

Also notice I added labels and milestones. That's a bit of our process that's not required during the trial but would still be appropriate.

Rule of thumb to choose the correct milestone value: if at the start of a sprint, use the current milestone (i.e. the one that will close at the start of the next sprint); if towards the end of a sprint (say only 2-3 days left) use the next milestone.

In our case, today is Tuesday, and it's the first week of the sprint. 19.8 is the milestone in code freeze (see the ❄️ in the name), 19.9 is the current milestone (the one that we'll close at the start of next sprint), and 20.0 is the next milestone (the WordPress apps adhere to the WordPress versioning which is not strictly SemVer: once we reach x.9 we roll to (x+1).0

crazytonyli · 2022-05-03T06:07:54Z

🥳 Thanks for merging this PR!

I did notice the labels and milestone in other PRs. But I don't think I have the permission to add labels nor milestone.

mokagio · 2022-05-03T19:36:16Z

But I don't think I have the permission to add labels nor milestone.

Oh, I wasn't expecting that... No worries, then. Someone will add them

Remove "context saved" expectation from CoreDataStack mocks

98dc3b7

They are moved to test methods.

crazytonyli commented Apr 30, 2022

View reviewed changes

mokagio reviewed May 2, 2022

View reviewed changes

mokagio mentioned this pull request May 2, 2022

Maintainer PR to run tests on external contributor PR #18485 #18488

Merged

crazytonyli marked this pull request as ready for review May 2, 2022 08:39

Use testContextManager instead of ContextManager.sharedInstance

c6873f2

They are the same object in runtime, since `TestContextManager` overrides `ContextManager.sharedInstance` with a mock instance.

crazytonyli requested a review from mokagio May 2, 2022 22:52

mokagio approved these changes May 3, 2022

View reviewed changes

mokagio added Testing Unit and UI Tests and Tooling Core Data Issues related to Core Data labels May 3, 2022

mokagio added this to the 19.9 milestone May 3, 2022

mokagio enabled auto-merge May 3, 2022 04:36

mokagio merged commit 90cc17e into wordpress-mobile:trunk May 3, 2022

crazytonyli deleted the test-expectation branch May 4, 2022 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "context saved" expectation from CoreDataStack mocks #18485

Remove "context saved" expectation from CoreDataStack mocks #18485

crazytonyli commented Apr 30, 2022

crazytonyli Apr 30, 2022

mokagio May 2, 2022

mokagio left a comment

mokagio May 2, 2022

crazytonyli May 2, 2022

mokagio May 3, 2022

mokagio May 2, 2022

crazytonyli May 2, 2022

mokagio May 3, 2022

mokagio May 2, 2022

mokagio left a comment

mokagio commented May 3, 2022

crazytonyli commented May 3, 2022

mokagio commented May 3, 2022

	/// This test stuite is clashing with other tests
	/// Specifically:
	/// - [BlogJetpackTest testJetpackSetupDoesntReplaceDotcomAccount]
	/// - CommentServiceTests.testFailingFetchCommentLikesShouldCallFailureBlock()
	///
	/// We weren't able to figure out why but it seems a race condition + Core data
	/// For now, renaming the suite to change the execution order solves the issue.
	class ZBlogDashboardServiceTests: XCTestCase {

		XCTestExpectation *saveExpectation = [self expectationForNotification:NSManagedObjectContextDidSaveNotification object:self.testContextManager.mainContext handler:nil];

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

	- (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;
	}

	- (NSManagedObjectContext *)mainContext
	{
	if (_mainContext) {
	return _mainContext;
	}

	_mainContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSMainQueueConcurrencyType];
	_mainContext.persistentStoreCoordinator = self.persistentStoreCoordinator;

	return _mainContext;
	}

Remove "context saved" expectation from CoreDataStack mocks #18485

Remove "context saved" expectation from CoreDataStack mocks #18485

Conversation

crazytonyli commented Apr 30, 2022

Changes

Test Instructions

Regression Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

mokagio commented May 3, 2022

crazytonyli commented May 3, 2022

mokagio commented May 3, 2022