theextremeprogrammer / Succinct

UI tests at the speed of unit tests. Proper encapsulation. Architecture agnostic. Freedom to refactor.
MIT License
42 stars 7 forks source link

Change tapCell to not tap cell when isUserInteractionEnabled is false #71

Open rgravina opened 3 years ago

rgravina commented 3 years ago

Currently when a UITableViewCell has isUserInteractionEnabled set to false, tapCell taps the cell (I think internally it calls the UITableViewDelegates select cell method directly).

I think it would be more valuable if tapCell did nothing, and the tester would have their assertions about what happens after the tap fail.

theextremeprogrammer commented 3 years ago

Thanks for this @rgravina! This doesn't sound like a difficult one to implement - though I would think that ideally isUserInteractionEnabled is something that should likely be checked and handled for any/all UIView types. But in the case of tapping a UITableViewCell, this can be handled specifically in this issue.

It looks like currently Succinct can only tap a cell that contains a label with exact text when testing a UIViewController: public func tapCell(withExactText searchText: String). Does this criteria fit your needs or do you need to search for a cell using a different criteria?

Given the cell exists, it both calls the .selectRow(at:animated:scrollPosition:) method as well as the UITableView delegate?.tableView?(didSelectRowAt:) method:

if let cell = tableView.dataSource?.tableView(tableView, cellForRowAt: indexPath) {
    if let _ = cell.findLabel(withExactText: searchText) {
        tableView.selectRow(at: indexPath, animated: false, scrollPosition: .none)
        tableView.delegate?.tableView?(tableView, didSelectRowAt: indexPath)
    }
}

This code is some of the original code that I had written for Succinct, and after encountering some issues with testing UITableView objects and switching away from UITableView due to these difficulties, I haven't given any love to this part of the codebase recently so it definitely could use some improvements.

I think it would be more valuable if tapCell did nothing, and the tester would have their assertions about what happens after the tap fail.

I agree - I'd also like to add that I think Succinct should print a message indicating that it purposely didn't tap the cell because isUserInteractionEnabled is false. I can foresee a day when future me is trying to tap a cell in a test and wondering "Why isn't this working?!" and not realizing that it's because I set isUserInteractionEnabled to false lol.

Is this a high priority for you? Let me know so we can prioritize this along with the other issues you've helped to raise. 👍🏻

rgravina commented 3 years ago

It's not a high priority to change/fix. I noticed it as the tests were passing but the real cell wouldn't handle the tap because of isUserInteractionEnabled being false, and thought it would be nice to have a failing test for it. We ended up not needing to set isUserInteractionEnabled in this case so it's not a problem any more.

I had this thought too... isUserInteractionEnabled is a UIView property, so just like isHidden in #58 any parent view being hidden would prevent taps on the cell (like if the whole table view has user interaction off). So maybe it's worth thinking about this after that's done.