Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Adds ability to use device model and OS on image names #121

Merged
merged 1 commit into from
Oct 9, 2015

Conversation

esttorhe
Copy link
Contributor

@esttorhe esttorhe commented Oct 7, 2015

We like to actually record snapshots on multiple devices and multiple OSs (using bots for example is easier to automate testing on iPad/iPhone and a combination of different OSs).

Instead of having to manually pass the data to each test I added a new property:
recordModelAndOSInName which is used to check if those elements should be added to the recorded/retrieved image later on the comparison; resulting on an image name like the following:

context_controller_being_tested_iPhone9_0.png

Probably the names of the functions & properties could use some imagination but the functionality is there.

NSString *model = [device.model stringByReplacingOccurrencesOfString:@" " withString:@"_"];
NSString *os = [device.systemVersion stringByReplacingOccurrencesOfString:@"." withString:@"_"];

fileName = [NSString stringWithFormat:@"%@_%@%@", fileName, model, os];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We don't tend to align on the =; I'd prefer if we stuck to a single space before and after the = here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it; let me remove that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, were you to separate this logic out into a separate function or object, whose only responsibility was to format the strings returned by -[UIDevice model] and -[UIDevice syetemVersion], this code would be a little cleaner. It'd also be easier to test.

Speaking of which, how about some tests for this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I added a test case for this but it looks like I only added it to ashfurrow/Nimble-Snapshots#32

I'll separate the formatting logic elsewhere (more so because I wanted to actually have iPhone 5s as the model and that would required an extension on UIDevice) and will correctly show the desired model and will also add a test for it tomorrow morning

@modocache
Copy link

Awesome stuff, thanks! 👍

I had a few suggestions on the implementation. @nscoding or @adamjernst may have opinions on whether this feature should be added in the first place.

@nscoding
Copy link
Contributor

nscoding commented Oct 8, 2015

I don't have a problem merging this in, if everyone is onboard. It's a cool feature that we can include. You can achieve the same by providing your own specific suffixes function but this is clearly easier and you don't need another marco.

So:

  • @modocache suggestion's
  • Tests
  • Entry on the ReadMe file

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 8, 2015

Finally got some time and made some changes.

Slightly changed the approach because the simulator was not able to detect the «simulated device» and instead would store all images as iPhone Simulator which would defeat the purpose.

To this I was suggested to instead save them by screen bounds since different models won't necessarily affect the output of a test but rather the OS and size; so now the device agnostic images have a name like this:

reference_TestViewController_iPhone9_0_414x736@3x.png


TODO:

  • Add tests
    • Need to figure out how to test without mocking and making it pass no matter on which simulator the test runs
  • Entry on README

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 8, 2015

Ok @nscoding, @modocache I made the suggested changes, including a small test and added an entry to the README.

Feel free to review again whenever you guys have the time 🙇

Thanks

@@ -103,6 +103,11 @@
@property (readwrite, nonatomic, assign) BOOL recordMode;

/**
Specifies if the snapshot should have the name of the device model and OS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could write what is the default value.

@Grubas7
Copy link
Contributor

Grubas7 commented Oct 8, 2015

@esttorhe Nice!
But I have one question. You show example of saved file:

reference_TestViewController_iPhone9_0_414x736@3x.png

What if I run test on other iPhone, eg. iPhone 5s. Will test compare file above anyway, or after set deviceAgnostic test pass only for same device/simulator?

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 8, 2015

@Grubas7 if deviceAgnostic is set it will only pass the test if the same simulator and «screen size» device is used.

That's a desired behavior because I would like to know that on an iPhone 4 with iOS 7 it works as expected (according to the set constrains) but also be able to know that on iPhone 6s Plus on iOS 8.1 it fails because the constrains are bonkers due to the bigger screen/different simulator, etc

@Grubas7
Copy link
Contributor

Grubas7 commented Oct 8, 2015

@esttorhe Just asking ;) Default value is NO so IMHO everything is OK.

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 8, 2015

hehehe yeah; default value is NO; didn't want to mess with the existing implementation 🎋

if (self.isDeviceAgnostic) {
fileName = FBDeviceAgnosticNormalizedFileName(fileName);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@nscoding
Copy link
Contributor

nscoding commented Oct 9, 2015

Awesome stuff @esttorhe, can you squash the commits before I merge?

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 9, 2015

Sure; give me a sec

Add «device agnostic» support for the snapshots.

Renamed property.

Instead of `recordModelAndOSInName` now using `deviceAgnostic` and `isDeviceAgnostic` for better readability.

Suggested changes from the PR

- Extracted the «path normalizer» to FBSnapshotTestCasePlatform
- Removed alignment of `=`s
- Added property to the test case.

- Add tests

Added test for the device agnostic method.

Added `isDeviceAgnostic` entry to the README

Housekeeping 🏠

- Fixed internal documentation to state default values
- Changed `UIScreen` in favor of `UIWindow` as per 's recommendations.
@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 9, 2015

@nscoding squashed into 1 single commit now

nscoding pushed a commit that referenced this pull request Oct 9, 2015
Adds ability to use device model and OS on image names
@nscoding nscoding merged commit cf04b30 into facebookarchive:master Oct 9, 2015
@nscoding
Copy link
Contributor

nscoding commented Oct 9, 2015

🎉

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 9, 2015

@nscoding sorry to bother; any chance to cut a new podspec with this on? 🙈

@nscoding
Copy link
Contributor

nscoding commented Oct 9, 2015

I want to do it with #119 merged in but we are almost there

@esttorhe
Copy link
Contributor Author

esttorhe commented Oct 9, 2015

Sure; no rush 😄

Just wondering 👍

@diachini
Copy link

@esttorhe this is awesome! 👍

Out of curiosity, do you script the running of your tests over an array of devices so you don't manually run them in Xcode?

@esttorhe
Copy link
Contributor Author

@diachini I'm currently using ashfurrow/Nimble-Snapshots#32; there's a flag inside that framework that switches the check for images for a «record images» instead.

Using Buildasaur I set a bot to run tests on all available devices and OSs pulling Nimble-Snapshots from a fork that has the flag set to true and that way I can record automatically to all devices.

Granted is not the most beautiful/easiest way but I had to set it real quick; I'm thinking in revisiting this later to add an easier way to achieve it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants