Add TableView and CollectionView protocol

benasher44 commented 6 years ago

Changes in this pull request

This adds TableView and CollectionView protocols, so that the concrete UITableView and UICollectionView can be used less often in tests. This fixes the testing issues that surfaced in #136.

Checklist

[ ] All tests pass. Demo project builds and runs.
[ ] 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.
[ ] I have reviewed the contributing guide

In-Progress TODOs

[ ] Do this work for CollectionView

benasher44 commented 6 years ago

@jessesquires current state is that there's only TableView protocol, and all the TableView-side of ReactiveLists passes. General idea is that TableView captures all of the functionality relied upon in a UITableView by TableViewDriver. This means that we can tell "diff" or "no diff" a bit easier, since we can subvert DifferecenKit's UITableView extension in tests. Doing this forced me to confront a few issues in tests:

We seemed to be somehow using HeaderView and TestTableViewSectionHeaderFooter at the same time for the same IndexPath in tests. We didn't use the latter much, so I instead just copied the functionality out of there and into HeaderView and FooterView.
An XCTAssertNil is now an XCTAssertNotNil (will call this one out in line)
The registerViews(for:) method now belongs to a constrained extension of the TableView protocol. This actually ends up making our tests more thoroughly test this functionality (will call out where inline).

The down side is the testTableView methods in tests, which are needed to make cell/header/footer dequeuing work properly.

jessesquires commented 6 years ago

Hm... I don't think I fully understand the goals here and what we're gaining by doing this. What's wrong with subclassing UITableView in tests to achieve the mocking we want?

This means that we can tell "diff" or "no diff" a bit easier, since we can subvert DifferecenKit's UITableView extension in tests.

I'm not sure I'm following here. We don't need to test DifferenceKit, right? That lib has tests? We just want to test that if, for example, we add a SectionModel that [tableView numberOfSections] returns the correct value?

RE: this comment https://github.com/plangrid/ReactiveLists/pull/136#issuecomment-417031912

FWIW, IGListKit uses a real UICollectionView and UIWindow in all its tests: https://github.com/Instagram/IGListKit/blob/master/Tests/IGListTestCase.m#L12-L30

I generally don't like the introduction of the TableViewProtocol, but more importantly this will hinder us from moving toward a unified API for tables and collections as described in #46 — the eventual goal will be to just have CellViewModels and Sections and be able to render either a collection or a table with little to no code changes (essentially just changing the cell type).

We should probably discuss in person 😄

benasher44 commented 6 years ago

Hm... I don't think I fully understand the goals here

I tried dropping in a UIWindow and ran into crashes in UITableView. Admittedly, I didn't try to hard to work them out because I figured trying to make UIKit work well in a test environment would be difficult.

FWIW, IGListKit uses a real UICollectionView and UIWindow in all its tests:

That's certainly one approach. It does expose the tests to flaking due to bugs in UIKit though, as well as further extends the reach to already very integration-y unit tests.

more importantly this will hinder us from moving toward a unified API for tables and collections as described

Hm I actually thought a protocol might be the layer of indirection we needed to have the unified API. We could evolve this into a ListView protocol, which has the API contract for a view driven by ListViewDriver (combination of CollectionViewDriver and TableViewDriver). Right now, TableView is too thin of a layer to be helpful, but we could iterate on it.

jessesquires commented 6 years ago

Hm I actually thought a protocol might be the layer of indirection we needed to have the unified API.

Ah yes, that's correct. We've started laying that groundwork with CellContainerViewProtocol, which unifies the APIs for tables and collections for dequeueing and registering cells. This seems more like the appropriate place to add the insert/delete/move/reload methods, rather than introduce a new protocol

benasher44 commented 6 years ago

I can do that, but then I'd have to make TableViewDriver and CollectionViewDriver generic because that protocol has associatedtype requirements.

plangrid / ReactiveLists