-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Start a thrash test suite for the collection node #1246
Conversation
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? |
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:
|
🚫 CI failed with log |
There was a problem hiding this 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?
Tests/ASCollectionViewThrashTests.mm
Outdated
@@ -0,0 +1,184 @@ | |||
// | |||
// ASTableViewThrashTests.mm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ASTableViewThrashTests.mm | |
// ASCollectionViewThrashTests.mm |
Tests/ASCollectionViewThrashTests.mm
Outdated
|
||
#pragma mark Test Methods | ||
|
||
// Disabled temporarily due to issue where cell nodes are not marked invisible before deallocation. |
There was a problem hiding this comment.
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.
Tests/ASCollectionViewThrashTests.mm
Outdated
} | ||
|
||
ASThrashDataSource *ds = [[ASThrashDataSource alloc] initCollectionViewDataSourceWithData:_update.oldData]; | ||
// why is ds.collectionView.test_enableSuperUpdateCallLogging available on table view but now collection view>? |
There was a problem hiding this comment.
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/ASCollectionViewThrashTests.mm
Outdated
|
||
@end | ||
|
||
@implementation ASCollectionViewThrashTests { |
There was a problem hiding this comment.
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?
Co-Authored-By: mikezucc <mikezuccarino@gmail.com>
Generated by 🚫 Danger |
Im creating a thrash test suite for the collection node to try and reproduce some very fickle, hard to reproduce issues.
tl;dr:
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.