-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix for #348 - IGListStackedSectionController's children need to know numberOrItems before didUpdate is called #390
Conversation
…’s backgroundColor property
… to each test method.
… numberOrItems before didUpdate is called
Nice. Thanks @jeffbailey ! 💯 |
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.
Woot, thanks @jeffbailey!
If we change all the tests to use batch updates we'll need to add an expectation so the asserts actually trigger.
|
||
XCTAssertEqual([stack numberOfItems], 6); | ||
[stack performBatchAnimated:false updates:^{} completion:^(BOOL finished) { | ||
XCTAssertEqual([stack numberOfItems], 6); |
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.
These tests use IGListAdapterUpdater which asynchronously collect updates during a runloop turn, so this assert wont execute w/out an expectation+wait setup. Here's an example. Same goes for all the asserts in this change that got moved into the completion block.
A really easy way to check that the block executes is:
__block BOOL executed = NO;
[adapter performUpdates:^{
executed = YES;
}];
XCTAssertTrue(executed);
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.
Yep, it should have dawned on me I made the changed tests asynchronous :). The issue with these particular tests is that they don't follow the normal pattern of the other tests because they don't use the adapter. I don't know without digging further why they don't use the adapter or if they can be changed easily. Also, my proposed solution wouldn't work anyway since the completion block never gets called because there's no CollectionContext set on the section controller. One easy way to use to fix the broken tests would be to use the following pattern:
- (void)test_whenInitializingStack_thatNumberOfItemsMatches {
IGListTestSection *section1 = [[IGListTestSection alloc] init];
section1.items = 2;
IGListTestSection *section2 = [[IGListTestSection alloc] init];
section2.items = 3;
IGListTestSection *section3 = [[IGListTestSection alloc] init];
section3.items = 0;
IGListTestSection *section4 = [[IGListTestSection alloc] init];
section4.items = 1;
IGListStackedSectionController *stack = [[IGListStackedSectionController alloc] initWithSectionControllers:@[section1, section2, section3, section4]];
[stack performSelectorOnMainThread:@selector(reloadData) withObject:nil waitUntilDone:TRUE];
XCTAssertEqual([stack numberOfItems], 6);
}
Basically just call the private reloadData method using performSelector. Let me know what you think the best way to proceed would be.
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.
@rnystrom - let me know what you think about just using:
[stack performSelectorOnMainThread:@selector(reloadData) withObject:nil waitUntilDone:TRUE];
to get the unit tests working.
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.
@jeffbailey oh! I missed that. Good call.
Instead of doing the performSelector
hack, how about we expose reloadData
in the internal header and call it directly?
The header is already imported to the test.
@jeffbailey updated the pull request - view changes |
1 similar comment
@jeffbailey updated the pull request - view changes |
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.
Also we need to add a CHANGELOG entry!
|
||
XCTAssertEqual([stack numberOfItems], 6); | ||
[stack performBatchAnimated:false updates:^{} completion:^(BOOL finished) { | ||
XCTAssertEqual([stack numberOfItems], 6); |
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.
@jeffbailey oh! I missed that. Good call.
Instead of doing the performSelector
hack, how about we expose reloadData
in the internal header and call it directly?
The header is already imported to the test.
@jeffbailey updated the pull request - view changes |
1 similar comment
@jeffbailey updated the pull request - view changes |
Friendly ping ✨ any updates here? @rnystrom if this is good, we can follow up with a changelog entry separately. |
The internal header is a great option. I'll get this done today. Thanks @rnystrom |
@jeffbailey updated the pull request - view changes |
@jeffbailey updated the pull request - view changes |
@jeffbailey looks great! Very last thing is a quick note in the |
@jeffbailey updated the pull request - view changes |
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
… numberOrItems before didUpdate is called Summary: The reloadData method in IGListStackedSectionController was being called too soon. It was moved from the constructor to the `didUpdateToObject` method. Tests were updated to reflect the change. - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Closes #390 Differential Revision: D4419751 Pulled By: rnystrom fbshipit-source-id: 7c812d306b23dd251c160425873930eb8022b1a5
…d to know numberOrItems before didUpdate is called Summary: The reloadData method in IGListStackedSectionController was being called too soon. It was moved from the constructor to the `didUpdateToObject` method. Tests were updated to reflect the change. - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Closes Instagram#390 Differential Revision: D4419751 Pulled By: rnystrom fbshipit-source-id: 7c812d306b23dd251c160425873930eb8022b1a5 # Conflicts: # CHANGELOG.md
Changes in this pull request
The reloadData method in IGListStackedSectionController was being called too soon. It was moved from the constructor to the
didUpdateToObject
method. Tests were updated to reflect the change.Pull request checklist
CHANGELOG.md
for any breaking changes, enhancements, or bug fixes.