Conversation
|
|
||
| /// A set of properties that provide information about device. | ||
| @objc(AGDeviceMockable) | ||
| public protocol DeviceMockable { |
There was a problem hiding this comment.
DeviceMockable > MockableDevice?
| import Foundation | ||
| import UIKit | ||
|
|
||
| public typealias DeviceViewMockable = ViewMockable & DeviceMockable |
There was a problem hiding this comment.
DeviceViewMockable > MockableDeviceView?
| @@ -0,0 +1,102 @@ | |||
| // | |||
| // MockingDeviceView.swift | |||
There was a problem hiding this comment.
MockingDeviceView or MockedDeviceView? 🧐
There was a problem hiding this comment.
Probably you are right.
Mocking and Mockable is good for protocol (https://swift.org/documentation/api-design-guidelines/#protocols-describing-what-is-should-read-as-nouns) and for class probably MockedDeviceView would be better.
Please check also remark: #19 (comment)
aleksandergrzyb
left a comment
There was a problem hiding this comment.
Couple of questions. 👌 Looks really nice.
| var name: String { get set } | ||
|
|
||
| /// Interface orientation to stub. | ||
| var orientation: UIInterfaceOrientation { get set } |
There was a problem hiding this comment.
Why UIInterfaceOrientation and not UIUserInterfaceSizeClass? I don't know if combination of UIUserInterfaceIdiom and UIInterfaceOrientation makes sense. UIUserInterfaceIdiom can be UIUserInterfaceIdiomCarPlay, what orientation has this class then?
If we want to support only iOS (iPhone and iPad) we should be more specific than UIUserInterfaceIdiom, maybe create own type.
There was a problem hiding this comment.
Why UIInterfaceOrientation and not UIUserInterfaceSizeClass? I don't know if combination of UIUserInterfaceIdiom and UIInterfaceOrientation makes sense.
UIUserInterfaceSizeClasses are defined in ViewMockable protocol.
UIUserInterfaceIdiom can be UIUserInterfaceIdiomCarPlay, what orientation has this class then?
I have never checked interface orientation on CarPlay, but UIInterfaceOrientation has value .unknown so in worst case we could use that.
If we want to support only iOS (iPhone and iPad) we should be more specific than UIUserInterfaceIdiom, maybe create own type.
I don't think we have to limit only to iOS. Car play and tvOS probably can be supported out of box, watchKit requires some work.
|
|
||
| public typealias DeviceViewMockable = ViewMockable & DeviceMockable | ||
|
|
||
| /// View stubing layout defining properties and information about device. |
There was a problem hiding this comment.
stubing ➡ stubbing (same for other files)
| /// Additionally `mockParameters` contains information about mocked device, | ||
| /// which can be use to distinguish interface orientation or device model. | ||
| @objc(AGMockingDeviceView) | ||
| public class MockingDeviceView: UIView { |
There was a problem hiding this comment.
Maybe DeviceMockView? I find those classes names and protocols pretty confusing.
There was a problem hiding this comment.
I'm wondering if Mock/Mocking/Mockable word is correctly used.
mock objects are simulated objects that mimic the behavior of real objects in controlled ways*
And this is a UIView subclass, so it's not a abstract object that try to fulfil our expectation, but it's normal class. The behaviour that I'm trying to achieve is basically inject some read-only data to UIView instance. Maybe more precise name would be StubbedView/StubbedDeviceView?
| /// Some of layout properties (e.g. `safeAreaInsets`, or `traitCollection`) are read-only, | ||
| /// so this view can be used as container which helps to force custom layout. | ||
| @objc(AGMockingView) | ||
| public class MockingView: UIView { |
There was a problem hiding this comment.
When one could use MockingView instead of DeviceMockingView? Did you consider making one a subclass of another? It looks like lots of code duplication.
There was a problem hiding this comment.
When one could use MockingView instead of DeviceMockingView?
MockingViewmight be useful for simple testing cases, eg: Developer needs to test some view for larger content size category or custom trait collection.
DeviceMockingView should be used for testing view on specific device eg. iPad (so mockParameters would contains whole set of iPad UI specific values like: size and safe area insets) and additionally values which are useful for describing reference images (eg. orientation [on iPad height > width not always means portrait orientation], device name)
Did you consider making one a subclass of another? It looks like lots of code duplication.
Subclass is impossible because of mockParameters property has different type in MockingView and DeviceMockingView. I reduced trait collection logic.
There was a problem hiding this comment.
Subclass is impossible because of mockParameters property has different type in MockingView and DeviceMockingView.
What about making just one subclass of UIView that has two initializers (ViewMockable and DeviceViewMockable)? This kind of generic class would just be one purpose class that would be passed to FBSnapshotVerify(...) method. I think it would make usage a little simpler while preserving all the functionality. What do you think?
WHAT:
WHY:
DeviceViewMockablegives us possibility to create objects for specific test case, e.g.MockingViewgives easy to use View mock containerTO CONSIDER 🤔
MockingViewshould be calledStubingView? Same for protocols.