Skip to content

Start a thrash test suite for the collection node#1246

Merged
nguyenhuy merged 12 commits into
TextureGroup:masterfrom
mikezucc:collectionview-thrash-tests
Jan 3, 2019
Merged

Start a thrash test suite for the collection node#1246
nguyenhuy merged 12 commits into
TextureGroup:masterfrom
mikezucc:collectionview-thrash-tests

Conversation

@mikezucc

Copy link
Copy Markdown
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
Copy Markdown
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

mikezucc commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

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

ghost commented Nov 20, 2018

Copy link
Copy Markdown

🚫 CI failed with log

@nguyenhuy nguyenhuy left a comment

Copy link
Copy Markdown
Member

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?

Comment thread Tests/ASCollectionViewThrashTests.mm Outdated
@@ -0,0 +1,184 @@
//
// ASTableViewThrashTests.mm

Copy link
Copy Markdown
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

Comment thread Tests/ASCollectionViewThrashTests.mm Outdated

#pragma mark Test Methods

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

Copy link
Copy Markdown
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.

Comment thread 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>?

Copy link
Copy Markdown
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.

Comment thread Tests/ASThrashUtility.h Outdated
Comment thread Tests/ASCollectionViewThrashTests.mm Outdated

@end

@implementation ASCollectionViewThrashTests {

Copy link
Copy Markdown
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

ghost commented Jan 2, 2019

Copy link
Copy Markdown
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.

3 participants