New device agnostic option to avoid the suffix in the snapshots

[augustorsouza]

Adding an option to the agnostic snapshots to avoid the addition of the suffixes regarding dimension.

If accepted it will resolve #30

[CLAassistant]

All committers have signed the CLA.

[augustorsouza]

Is there anyone looking at pull requests for this project?

[alanzeino]

I didn't realise this when I commented on the issue, but this is too confusing because FBSnapshotTestCaseAgnosticOptionNone already exists.

And there's no nice way to do this with the option mask, because if we add a new FBSnapshotTestCaseAgnosticOptionScale then everyone will update to the new version and wonder why the scale isn't being added.

Perhaps it might work if we bump the major library version to follow semantic versioning, but that still doesn't solve the problem that people will wonder why all their tests are failing once they update.

[augustorsouza]

@alanzeino I don't know if I understood everything you commented. But, if people update right now with the fix I did they will have to add an option called FBSnapshotTestCaseAgnosticOptionNoSuffix to avoid the automatic append of the suffix.

If they still avoid using any option the default behavior will be used.

[alanzeino]

Sorry, should've been clearer. The confusing part is that there will be two options; FBSnapshotTestCaseAgnosticOptionNone and FBSnapshotTestCaseAgnosticOptionNoSuffix. The right way would be to call the new one FBSnapshotTestCaseAgnosticOptionScreenScale, so that users opt in to being 'agnostic' on screen scale, but then anyone who updates to the version that adds that will have to add this option, or their existing tests will fail.

I believe that in general the reason why the original library never made device scale something you can remove, is because the original authors would have considered that to be a fundamental part of the file name.

In short looking at this more now, I don't think we can make the change in a way that works for existing code.

[acecilia]

I would like to have this. Personally, I am manually adding the device name to the identifier, so the 2x/3x suffix is useless.
Seeing that sometimes using two different simulators with the same scaling throw different results, the scaling indicator is not as useful as it was when the library was conceived.

In addition, the actual implementation does not allow to modify the order of the agnostic suffixes/prefixes, or which separators to use (underscore, dots, brackets..). I would rewrite the implementation, so instead of having different flags for the different suffixes/prefixes, we can pass a formatter, which allows to modify the order and the separators (similar to the date formatters).

[augustorsouza]

sounds good, what do you think @alanzeino ? I can change my implementation to some kind of formatter block that optionally anyone can send to format the file name of the screenshot in any scenario the developer using the lib could chose.

[alanzeino]

It's a good idea, but again let's trade off that idea against supporting the current users. If we add that formatting block approach, we have to deprecate the deviceAgnostic BOOL and the agnosticOptions bitmask otherwise this API gets very confusing and bloated.

So we're at 3.0 now. If we follow semantic versioning, we deprecate deviceAgnostic and agnosticOptions in 4.0, and add the new formatter. In 5.0, we delete deviceAgnostic and agnosticOptions. This churn is a bit crazy, so I'm on the fence about it. I'm not even sure if I would deprecate deviceAgnostic and agnosticOptions in one version; I would probably want to deprecate deviceAgnostic first in one major version and agnosticOptions in a second.

I have some clarifying questions too, just so I understand where you're coming from:

Personally, I am manually adding the device name to the identifier, so the 2x/3x suffix is useless.

@acecilia So you are adding 'iPhone X' and 'iPhone SE' and 'iPhone 7' to your test file names?

Seeing that sometimes using two different simulators with the same scaling throw different results, the scaling indicator is not as useful as it was when the library was conceived.

@acecilia when you wrote this did you mean that when you take say, an iPhone SE and an iPhone 6 (which are both 2x scale), that they generate different snapshots so therefore the scale is not useful here?

If I understand correctly, this could also be resolved by making FBSnapshotTestCaseAgnosticOptionDevice a little smarter than it currently is, right? Today it uses UIDevice.current.model, which isn't very fine–grained; i.e., it always returns 'iPhone' for different kinds of iPhone models, like the 'iPhone X' or the 'iPhone 7 Plus'. So if I made that use better, more precise device names in the file paths (using a library like say, https://github.com/dennisweissmann/DeviceKit or https://github.com/InderKumarRathore/DeviceUtil), would that help solve this issue?

[alanzeino]

(Just investigating device names a bit more, seems like you can't get a more precise name when using the simulator. What I suggested above might not work. Will keep looking.)

[acecilia]

@alanzeino the main problem for me is that the framework does not allow for a flexible customization of the file names (I dont like the separators and order that the framework is using), so what I did is pass my own identifier, created using DeviceKit:

let dev = Device()
let imageSize = "\(Int(imageView.frame.size.width))x\(Int(imageView.frame.size.height))"
let identifier = "[\(dev.deviceName)]-[\(dev.systemVersion)]-[\(imageSize)]-\(snapshotIndex)-\(title)"

It looks like this:

<testIdentifierIntroducedByTheFramework>_[<device>]-[<os>]-[<width>x<height>]-<snapshotIndexForCurrentTest>-<Description of the screenshot>@<resolutionIntroducedByTheFramework>.png

And a real example:

testSignup_[iPhone 8]-[11.4]-[375x667]-1-phone prefix selector@2x.png

As you can see, the things I could not get rid off is the test name and the resolution suffix. My ideal snapshot name would be:

Signup-[iPhone 8]-[11.4]-[375x667]-1-phone prefix selector.png

IMHO what makes sense to me is one of this options (in order of preference):

• Keep the current naming convention and implementation, but adding the ability to override it 100% [allow removing the test name and the resolution indicator] in a easier way [right now getting rid of all naming modifications introduced by the framework requires to set configuration here and there, instead of one only "disableAllIdentifierCustomizations"].
• Do not provide any kind of naming convention, and require the user to pass a custom identifier, which will be used as is, and without any modifications (nor agnostic or other). Example: if the identifier is "image1", the image should be called "image1.png", and not "testName_image1@2x.png". In this case the user is on charge of properly naming the images.
• Change the implementation to some kind of formatter that allows flexibility and easy customization of the identifier. Even in this option, I think that people that want a very specific image identifier will probably require to pass their own. Also, implementing a formatter that provides with such flexibility + being type safe, may be too much effort for this tiny problem.

[alanzeino]

@acecilia One thing I've already mentioned on #14 is that in general, this library wasn't designed for the purpose of wanting to make a repository of nicely named screenshots. However in that issue we're going to allow for the folder name to be overridden, since that seems like a useful addition and solves the issue where sometimes the folder name includes redundant information which could be omitted.

I'm happy to consider changes that resolve issues with the library where the issue leads to a bug during testing but issues where a user has a custom workflow that is outside the goals and original intent of the project are a little less likely to make it into master. People are free to fork and make changes that make their workflow better and point their Podfile to that fork using the :git option.

If we continue down this path we have to 100% acknowledge that we need to make the change in a way that makes sense for users of the API and agree on how we deprecate the older API. I'm not interested in just adding stuff and leaving/supporting old ways of doing things in the API as well.

So again if the issue is that there's a bug where the FBSnapshotTestCaseAgnosticOptionDevice results in file names that aren't specific enough, I'm happy to fix that no problem. But more larger scale issues like supporting totally custom file names with formatters or other API is going to be more scrutinized. Probably any change made here will have large effects either on the API's consistency or on the consumers of the library (if the change is made in a way that requires a breaking version change keeping in conformance to semantic versioning).

Can we resolve this by improving the name used when passing the FBSnapshotTestCaseAgnosticOptionDevice option to the agnosticOptions bitmask?

[acecilia]

Implementing 100% custom name files by allowing to remove the test name and the resolution indicator will not break any API, it will be an incremental change, so you dont need to deprecate anything. It will not require to change main version number.

[alanzeino]

Unfortunately long term we will need to deprecate something because this API is already pretty bloated with both deviceAgnostic and agnosticOptions, and will become more so if we add a custom formatter option.

[augusto-souza-ifood]

Another option for us to think: optional env vars in the scheme, like the setting for the directories of the reference images.

Something like: USE_IMAGE_SUFFIX that accepts true or false The default is what people use now, so if it is not setted and we assume true, but we can turn it off setting this to false.

[augustorsouza]

@alanzeino and @acecilia if you guys agree I will can implement a environemnt var based solution. :)

[alanzeino]

An update here. In #64 we're going to add a new option mask to replace agnosticOptions called fileNameOptions. I'm going to add a new option for the screen scale, to take advantage of the opportunity presented by adding this new mask.