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

Start a thrash test suite for the collection node #1246

Merged
merged 12 commits into from
Jan 3, 2019

Conversation

mikezucc
Copy link
Contributor

Im creating a thrash test suite for the collection node to try and reproduce some very fickle, hard to reproduce issues.

tl;dr:

  • Split up existing table view thrash tests to have a common utility
  • Created a collection view/node (UIKit reference flag) thrash test

What do yall think? I want to add a test that has multiple collection views in memory to test the way UIKit manages index paths globally.

@mikezucc
Copy link
Contributor Author

The issue I was investigating: global resource of NSIndexPaths falling out of sync with UICollectionViewDataSource mid collection view update causing a crash inside UIKit

I think this multithreading issue in UIKit is exacerbated when the main run loop gets busied, with a user typing on the keyboard and network interrupts triggering further collection view reloads. What are some strategies to simulate this through the tests?

@mikezucc
Copy link
Contributor Author

mikezucc commented Nov 20, 2018

I think the XCTest runner also may not give the most accurate, true replication of the aforementioned problems since the test runner manipulates the run loop on its own.

I put in a flag to prevent usage of XCTest.waitWithExpectation on some test runs, but its not entirely avoidable for the dispatch main thrash test.

from the docs:

 * -waitForExpectationsWithTimeout:handler: runs the run loop while handling events until all expectations
 * are fulfilled or the timeout is reached. Clients should not manipulate the run
 * loop while using this API.

@ghost
Copy link

ghost commented Nov 20, 2018

🚫 CI failed with log

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

Changes in this PR LGTM.

@mikezucc, do you want to share the stack trace of the crash you're investigating?

@@ -0,0 +1,184 @@
//
// ASTableViewThrashTests.mm
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ASTableViewThrashTests.mm
// ASCollectionViewThrashTests.mm


#pragma mark Test Methods

// Disabled temporarily due to issue where cell nodes are not marked invisible before deallocation.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove the comment since the test is enabled now.

}

ASThrashDataSource *ds = [[ASThrashDataSource alloc] initCollectionViewDataSourceWithData:_update.oldData];
// why is ds.collectionView.test_enableSuperUpdateCallLogging available on table view but now collection view>?
Copy link
Member

Choose a reason for hiding this comment

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

We just didn't need it before. May worth adding it if you think it'd be helpful.

Tests/ASThrashUtility.h Outdated Show resolved Hide resolved

@end

@implementation ASCollectionViewThrashTests {
Copy link
Member

Choose a reason for hiding this comment

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

Per our coding convention, brackets should be on the following line (and ASTableViewThrashTests.mm doesn't adhere to it). Can you update the rest of this PR to follow this rule?

@ghost
Copy link

ghost commented Jan 2, 2019

1 Warning
⚠️ This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

@nguyenhuy nguyenhuy merged commit 22acb98 into TextureGroup:master Jan 3, 2019
@TextureGroup TextureGroup deleted a comment Jan 3, 2019
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.

None yet

3 participants