KaneCheshire / PixelTest

Fast, modern, simple iOS snapshot testing written purely in Swift.
MIT License
58 stars 8 forks source link

Add a way of enforcing people use the contentView of cells #39

Closed KaneCheshire closed 4 years ago

KaneCheshire commented 6 years ago

Snapshot testing cells requires the contentView be snapshot tested rather than the whole cell, so would be good to enforce this somehow.

theblixguy commented 6 years ago

Maybe the verify method could have a check:

if view is UITableViewCell {
    XCTFail("You must pass the `.contentView` of the UITableViewCell, not the cell itself!")
}

or you can continue with the snapshotting by extracting the contentView from the cell:

if view is UITableViewCell {
    print("Recieved a UITableViewCell, using its `.contentView` for snapshotting!")
    let contentView = view.contentView
    layoutCoordinator.layOut(contentView, with: layoutStyle)
    ...
} else {
    layoutCoordinator.layOut(view, with: layoutStyle)
    ...
}
KaneCheshire commented 6 years ago

Nice, I like both options. Second one feels more magical, first one makes it super clear what's happening

Jon889 commented 5 years ago

Could add this. Unfortunately using unavailable to give an error won't work as it will just use the original method, but with this at least you get a compile time warning.

    @available(*, deprecated, message: "You can't use UITableViewCell, pass the contentView instead")
    open func verify(_ view: UITableViewCell, layoutStyle: LayoutStyle,
                     scale: Scale = .native, file: StaticString = #file, function: StaticString = #function, line: UInt = #line) {
        fatalError("You can't use UITableViewCell, pass the contentView instead")
    }
theblixguy commented 5 years ago

Oh yeah, that would be nice! I would replace deprecated with obsoleted if we're going to throw a fatal error. Otherwise, we can add a deprecation and automatically extract the contentView from a view if its a table view cell.

Jon889 commented 5 years ago

I tried changing it to obsoleted and it didn't give a warning or error, I think like with unavailable it the switches to the UIView method. fatalError could be replaced with XCTFail

theblixguy commented 5 years ago

Oh! Hmm, we could also add a separate method for table view cells, like:

open func verify(_ view: UITableViewCell, layoutStyle: LayoutStyle,
                     scale: Scale = .native, file: StaticString = #file, function: StaticString = #function, line: UInt = #line) {
  let contentView = view.contentView
  layoutCoordinator.layOut(contentView, with: layoutStyle)
...
}
KaneCheshire commented 5 years ago

I reckon the fatalError is enough on its own no? They’ll soon get an error with a nice description of why there’s a fatalError so seems okay. No need to over complicate it?

KaneCheshire commented 5 years ago

Or we do as @theblixguy suggests and try to help them by extracting the content view for them, with maybe a message printed to the console or something. Whatever makes people’s lives easier really!

Jon889 commented 5 years ago

I was thinking that a compile time warning is better than at runtime, but as it's tests the fatalError would be enough. That is doesn't work on UITableViewCell's seems like an implementation detail, so getting the contentView out for the user would be nice.

theblixguy commented 5 years ago

Yeah, a compile time warning is always more useful than runtime warning, might have to wait for Swift 5 compile time assertions!

In the meantime, I suggest:

open func verify(_ view: UIView, layoutStyle: LayoutStyle, scale: Scale = .native, file: StaticString = #file, function: StaticString = #function, line: UInt = #line) {
    let viewToVerify = extractTableViewContentViewIfNeeded(from: view)
    layoutCoordinator.layOut(viewToVerify, with: layoutStyle)

    XCTAssertTrue(view.bounds.width > 0, "View has no width after layout", file: file, line: line)
    XCTAssertTrue(view.bounds.height > 0, "View has no height after layout", file: file, line: line)

    switch mode {
    case .record:
        record(viewToVerify, scale: scale, file: file, function: function, line: line, layoutStyle: layoutStyle)
    case .test:
        test(viewToVerify, scale: scale, file: file, function: function, line: line, layoutStyle: layoutStyle)
    }
}

private func extractTableViewContentViewIfNeeded(from view: UIView) -> UIView {
    guard view = view as? UITableViewCell else { return view }
    print("WARNING: Received a UITableViewCell, using its `.contentView` for snapshotting.")
    return view.contentView
}
KaneCheshire commented 5 years ago

Hmm I think we get warning directives in Swift 4.2: https://www.hackingwithswift.com/articles/77/whats-new-in-swift-4-2

theblixguy commented 5 years ago

Yeah, but that's different from compile time assertions. In Swift 5 (or later), you will be able to do something like #assert(!functionThatReturnsFalse(), "Function didn't return false!") and it will be evaluated at compile time rather than run time 👍

KaneCheshire commented 4 years ago

This has been implemented now