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.
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.
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.
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.
@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.
@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 |
|
|
||
| XCTAssertEqual([stack numberOfItems], 6); | ||
| [stack performBatchAnimated:false updates:^{} completion:^(BOOL finished) { | ||
| XCTAssertEqual([stack numberOfItems], 6); |
There was a problem hiding this comment.
@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
didUpdateToObjectmethod. Tests were updated to reflect the change.Pull request checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.