Add TableView and CollectionView protocol #139

benasher44 · 2018-09-05T23:44:25Z

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 · 2018-09-05T23:50:54Z

@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.

benasher44 · 2018-09-05T23:51:09Z

Sources/TableViewDriver.swift

@@ -35,7 +35,7 @@ open class TableViewDriver: NSObject {
    }

    /// The table view to which the `TableViewModel` is rendered.
-    public let tableView: UITableView
+    let tableView: TableView


This seemed okay to do?

I haven't tried it internally, but I think in most cases, you'll have another pointer to this view anyway.

benasher44 · 2018-09-05T23:51:58Z

Sources/TableViewDriver.swift

@@ -72,6 +72,8 @@ open class TableViewDriver: NSObject {

    private let _automaticDiffingEnabled: Bool

+    private let _registerViews: (TableViewModel) -> Void


This is needed to avoid associatedtype issues with CellContainerViewProtocol.

benasher44 · 2018-09-05T23:52:21Z

Sources/UITableView+Extensions.swift

@@ -16,7 +16,7 @@

 import UIKit

-extension UITableView {
+extension TableView where Self: CellContainerViewProtocol, Self.CellType: UITableViewCell {


protocol extension with generic constraints!

benasher44 · 2018-09-05T23:54:57Z

Tests/TableView/TableViewDriverTests.swift

            return
        }
        XCTAssertEqual(onScreenHeader.label, "title_header+A")
-        XCTAssertNil(self._tableView.headerView(forSection: indexKey.section))
+        XCTAssertNotNil(self._tableView.headerView(forSection: indexKey.section))


this flipped? seems like it's correct now. @jessesquires did I mess this up? or does this seem right?

Not sure. I didn't write any of these tests and they're all mostly really bad 😄 😅

benasher44 · 2018-09-05T23:55:44Z

Tests/TableView/TableViewDriverTests.swift

@@ -213,7 +217,8 @@ final class TableViewDriverTests: XCTestCase {
        let tableView = TestTableView()
        self.setupWithTableView(tableView)

-        XCTAssertEqual(tableView.callsToRegisterClass.count, 2)
+        // 1 header + 1 footer + 6 cells
+        XCTAssertEqual(tableView.callsToRegisterClass.count, 8)


improved count, now that we're exercising the registerViews(for:) functionality in the constrained protocol extension

jessesquires · 2018-09-06T23:02:15Z

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 #136 (comment)

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 · 2018-09-06T23:33:00Z

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 · 2018-09-07T17:29:55Z

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 · 2018-09-07T17:32:20Z

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

benasher44 added the in-progress label Sep 5, 2018

benasher44 added this to the 0.2.0 milestone Sep 5, 2018

benasher44 commented Sep 5, 2018

View reviewed changes

Add TableView protocol

20938c4

benasher44 force-pushed the basher/tableview-protocol branch from 5d15048 to 20938c4 Compare September 5, 2018 23:54

benasher44 commented Sep 5, 2018

View reviewed changes

benasher44 closed this Sep 10, 2018

benasher44 deleted the basher/tableview-protocol branch September 10, 2018 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TableView and CollectionView protocol #139

Add TableView and CollectionView protocol #139

benasher44 commented Sep 5, 2018 •

edited

Loading

benasher44 commented Sep 5, 2018

benasher44 Sep 5, 2018

benasher44 Sep 5, 2018

benasher44 Sep 5, 2018

benasher44 Sep 5, 2018

benasher44 Sep 5, 2018

jessesquires Sep 6, 2018

benasher44 Sep 5, 2018

jessesquires commented Sep 6, 2018

benasher44 commented Sep 6, 2018 •

edited

Loading

jessesquires commented Sep 7, 2018

benasher44 commented Sep 7, 2018

		@@ -72,6 +72,8 @@ open class TableViewDriver: NSObject {

		private let _automaticDiffingEnabled: Bool

		private let _registerViews: (TableViewModel) -> Void

Add TableView and CollectionView protocol #139

Add TableView and CollectionView protocol #139

Conversation

benasher44 commented Sep 5, 2018 • edited Loading

Changes in this pull request

Checklist

In-Progress TODOs

benasher44 commented Sep 5, 2018

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

jessesquires Sep 6, 2018

Choose a reason for hiding this comment

benasher44 Sep 5, 2018

Choose a reason for hiding this comment

jessesquires commented Sep 6, 2018

benasher44 commented Sep 6, 2018 • edited Loading

jessesquires commented Sep 7, 2018

benasher44 commented Sep 7, 2018

benasher44 commented Sep 5, 2018 •

edited

Loading

benasher44 commented Sep 6, 2018 •

edited

Loading