Add ability to exclude OS version from snapshot file name #158
Conversation
When deviceAgnostic is set to true, this variable will control whether the OS version is included in the snapshot file name.
|
Yes, this makes sense! |
|
|
||
| @see deviceAgnostic | ||
| */ | ||
| @property (readwrite, nonatomic, assign) BOOL includeOSVersionInFilename; |
There was a problem hiding this comment.
Can we change this from a BOOL to an enum where we could select what information to append? The default value will accommodate all of them. I want to avoid a specific property just for the name OS version.
typedef NS_OPTIONS(NSInteger, FBSnapshotTestDeviceAgnosticOption) {
FBSnapshotTestDeviceAgnosticOptionModel = 1 << 0,
FBSnapshotTestDeviceAgnosticOptionSystemVersion = 1 << 1,
FBSnapshotTestDeviceAgnosticOptionScreenSize = 1 << 2,
};
@property (readwrite, nonatomic, assign) FBSnapshotTestDeviceAgnosticOption deviceAgnosticOptions;
There was a problem hiding this comment.
Yeah, that is a nicer solution. I was planning on a follow up PR to this one that used more descriptive device names in the snapshot filenames, rather than just using model names, so maybe you can give me some feedback on that idea now, and I might be able to incorporate it into this?
Basically my rationale is that it isn't easy to determine if a snapshot is for an iPhone 6, or a 6 Plus from the filename. While the use of the device model (iPhone or iPad) and size dimensions give you some idea of what it is, having iPhone6 or iPhone6Plus in the file name is more readable.
Would it be preferable to replace device model with more descriptive device names, or make descriptive device names a further option? I'd prefer to replace, but it would be a breaking change for everybody using deviceAgnostic = true.
|
Hi @nathanshox and @nscoding I've found that my PR #161 actually extends that feature. What do you think? |
|
@Antondomashnev yes, it's the same, I will need to go over both PR this week and see which one we should go ahead with. I was blocked because I wanted to cut a spec for 2.1.1 which I finally did. |
ghost
added
the
|
This is similar to what I've proposed in #235. But this still couples And the funny thing is, my problem is the inverse of #161. It is precisely that an OS update requires regenerating all the snapshots that I want to be able to have two versions of all the snapshots checked into master. If you're changing the Base SDK and conversely the default OS version of the simulator you use for snapshots (if you only use one), it's pretty beneficial to land all the new snapshots well in advance of adopting the new SDK. |
|
Hey there! We've moved the repo and we're PR'ing a change that should help you here: |
We run tests on multiple devices and use
deviceAgnosticto get images specific for each device. We only ever test on the latest OS, and the current behaviour is that the OS version is included in the filename.This presents a number of problems when we are updating tests to the latest OS. Namely, we end up with completely different images so we can't compare changes easily, and we also end up with orphaned images that we must remember to manually delete.
This PR adds a Boolean,
includeOSVersionInFilenameto FBSnapshotTestCase to control whether the version gets included in the filename when usingdeviceAgnostic = true. By default it is set toYESso that it doesn't break current users. If it is set toNO, you will get a snapshot file name that includes the device model, but not the OS version.