add device agnostic tests & add helpful assertion for device agnostic tests #137

bgerstle · 2015-12-09T04:44:30Z

Our app doesn't have a window when run as a test host, which results in keyWindow being nil. This could also happen if your test target is a "Logic Test", and not an "Application Test."

$ ls -1 WikipediaUnitTests/ReferenceImages_64/NSAttributedString_WMFModifyTests
[email protected]
testChangeGreenBackgroundColorsToBlue_iPhone_Simulator8_4_0x0@2x.png
[email protected]
[email protected]
[email protected]

I've added an assertion so others can get clearer feedback about what's going wrong and how to fix it. FBSnapshotTestCase could arguably create a keyWindow implicitly, but I figured it's better to let users set up their test host properly instead of implicitly relying on the library to do it for them.

facebook-github-bot · 2015-12-09T04:46:40Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

facebook-github-bot · 2015-12-09T05:02:29Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

- re-record all snapshots - point to FBSnapshotTestCase fork w/ a fix for screen size suffix (PR is already up: facebookarchive/ios-snapshot-test-case#137) - add "WMFVisualTestBatchRecordMode" to make it easier to re-record all snapshot tests

bgerstle · 2015-12-09T17:01:34Z

looks like I need to download iOS 9.0 sim and re-record snapshots. downloading now... ⏳

Grubas7 · 2015-12-09T17:09:56Z

FBSnapshotTestCase/FBSnapshotTestCasePlatform.m

@@ -34,7 +34,7 @@ BOOL FBSnapshotTestCaseIs64Bit(void)
 NSString *FBDeviceAgnosticNormalizedFileName(NSString *fileName)
 {
  UIDevice *device = [UIDevice currentDevice];
-  CGSize screenSize = [[UIApplication sharedApplication] keyWindow].bounds.size;


I'm not sure if this change is OK.
UIWindow != UIScreen -> UIWindow is unique for application (especially useful for split view), and UIScreen is for whole device.

That's true, but I figured using the screen would:

Be equivalent to the window in most cases

Work when the library is integrated as a framework (where keyWindow does not)

Maybe a more comprehensive way to solve this would be to generate the suffix based on the view being tested, which is probably what you really want anyway. This makes even more sense when you consider that snapshots are generated based on the bounds of the specified view, even creating and "keying" a window if necessary.[0]

0: https://github.com/facebook/ios-snapshot-test-case/blob/master/FBSnapshotTestCase/Categories/UIImage+Snapshot.m#L46

hmm, we could check if the keyWindow is nil and if it is, create a window with the [[UIScreen mainScreen] bounds]

How does this sound?

Actually, just had a realization: the problem doesn't have anything to do with frameworks. Our app short-circuits itself when being run as the test host, which means no AppDelegate, which means no keyWindow.

So, this is more about handling our specific edge case, which means that a window should be made the keyWindow before suffixes are generated. At which point, I'm wondering if we should be using a "dummy" app delegate (which just has a dummy window), or FBSnapshotTestCase should install one for me (e.g. if you were running a Logic Tests target which didn't even have a host).

I had similar problem:
I write some framework which is using UIApplication. Same framework doesn't have UIApplication, so I created TestApp, which hosts tests.
So check if this resolve your problem:

Add new Application target

Choose your framework test target

Go to general tab

Set your new Application target as Host Application

Make sure that your Application target contains framework tests in test schema

nice, if that works maybe we can assert and explain how to solve this

@Grubas7 the short-circuit is intentional, and our app is already the "Host" for our tests. This removes the unnecessary "link in both targets" step when adding new app code.

My question to you guys is, should FBSnapshotTestCase be defensive by setting the keyWindow itself (which it already tries to do), and if so, should it guarantee that keyWindow is available at file suffix generation time? For example, we could use a category on UIApplication that gets or creates a key window.

My question above determines whether the solution should be either:

Add an assertion and explain what users should do (set a host app or otherwise create a key window when visual tests are running)

Create a keyWindow manually if necessary

bgerstle · 2015-12-12T23:35:35Z

@Grubas7 @nscoding I reverted my change using UIScreen and added an assertion instead. This should prompt users (like me) who don't have a key window to go ahead and create one.

Grubas7 · 2015-12-13T11:39:00Z

@bgerstle Cool. I wonder if there should be some info about this in README. @nscoding ?

nscoding · 2015-12-13T22:30:25Z

FBSnapshotTestCaseDemo/FBSnapshotTestCaseDemoTests/FBSnapshotTestCaseDeviceAgnosticTests.m

+{
+    UIView *redView = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 40, 40)];
+    redView.backgroundColor = [UIColor redColor];
+    FBSnapshotVerifyView(redView, nil);


Travis is hitting the assertion that you added, why?

Good question, will have to investigate..
On Sun, Dec 13, 2015 at 17:30 Patrick Chamelo [email protected]
wrote:

In
FBSnapshotTestCaseDemo/FBSnapshotTestCaseDemoTests/FBSnapshotTestCaseDeviceAgnosticTests.m
#137 (comment)
:

+@EnD
+
+@implementation FBSnapshotTestCaseDeviceAgnosticTests
+
+- (void)setUp
+{

[super setUp];

self.deviceAgnostic = YES;

self.recordMode = NO;
+}

+- (void)testViewSnapshot
+{

UIView *redView = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 40, 40)];

redView.backgroundColor = [UIColor redColor];

FBSnapshotVerifyView(redView, nil);

Travis is hitting the assertion that you added, why?

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/ios-snapshot-test-case/pull/137/files#r47451523
.

Because the keyWindow is nil... 😖 Even though the app delegate is certainly making it key and visible. Any ideas?

Also @Grubas7 this is why the tests were added

Should I:

Somehow wait for the application to have a key window, maybe using expectationWithPredicate?

Fall back to the application's delegate's window?

what do you suggest? poll keyWIndow in FBSnapshotTestCase.setUp? e.g.:

- (void)setUp { // other stuff [self expectationWithPredicate:[NSPredicate predicateWithBlock:^ (...) { return evaluatedObject.keyWindow != nil; } ...]; }

Sorry, I had to look at it a bit to figure out what is going on and there are a couple of issues:

The build script that I added on travis is building and testing both iPhone 5 and iPhone 6, so we have to record the image for both of these devices.

The build script runs the FBSnapshotControllerTests and in particular testFailedImageWithDeviceAgnosticShouldHaveModelOnName. These tests don't have an application associated with, so we hit the assertion. Maybe we can make the window a parameter and simply write tests to make sure that FBDeviceAgnosticNormalizedFileName is working as expected.

@bgerstle thoughts on my latest comment?

oh I didn't see the updated commits

@nscoding yeah, sorry I didn't respond, but better late than never:

The build script that I added on travis is building and testing both iPhone 5 and iPhone 6, so we have to record the image for both of these devices.

I already figured that out when Travis failed and downloaded/recorded the proper screenshots.

testFailedImageWithDeviceAgnosticShouldHaveModelOnName. These tests don't have an application associated with, so we hit the assertion.

Those tests are in the Demo project, which does have a host application (see above screenshot). I think the problem was that UIImage+Snapshot was causing issues when installing a "dummy" window when given a view without a window. This somehow caused the keyWindow to return nil when device-agnostic tests were run—which I determined after seeing each test pass in isolation. As a result, I've modified UIImage+Snapshot to use the application's keyWindow if the given view doesn't have a window (or if it isn't a window itself). Let me know if you think this is acceptable.

Maybe we can make the window a parameter and simply write tests to make sure that FBDeviceAgnosticNormalizedFileName is working as expected.

That could be useful for verifying the formatting of test names (to prevent unintended regressions and thus break tests for users), but doesn't address the problems targeted by this PR.

- re-record all snapshots - point to FBSnapshotTestCase fork w/ a fix for screen size suffix (PR is already up: facebookarchive/ios-snapshot-test-case#137) - add "WMFVisualTestBatchRecordMode" to make it easier to re-record all snapshot tests

bgerstle · 2015-12-28T21:31:08Z

@Grubas7 @nscoding this should be ready to go, see my latest comment for a brief explanation on the approach and c9a9530 for the implementation

Grubas7 · 2015-12-28T22:01:14Z

FBSnapshotTestCase/Categories/UIApplication+StrictKeyWindow.m

+
+@implementation UIApplication (StrictKeyWindow)
+
+- (UIWindow*)fb_strictKeyWindow {


Missing space before asterisk.

Have you guys considered using uncrustify or clang-format? 😅 While I'm at it, should I remove the comment headers (Created by Brian Gerstle etc.)?

went ahead and did it anyway

Grubas7 · 2015-12-28T22:50:01Z

Looks good 👍

bgerstle · 2015-12-28T22:59:40Z

@Grubas7 thanks for the feedback! all your comments should be addressed 👌

Grubas7 · 2015-12-29T12:25:03Z

@bgerstle could you squash your commits?

bgerstle · 2015-12-29T16:04:50Z

@Grubas7 squashed & amended commit message

nscoding · 2016-01-03T02:12:08Z

FBSnapshotTestCase/Categories/UIApplication+StrictKeyWindow.m

+
+@implementation UIApplication (StrictKeyWindow)
+
+- (UIWindow *)fb_strictKeyWindow {


new line before {

nscoding · 2016-01-03T02:16:16Z

Thank you for working on this, it looks great, I left a couple of comments (nit picks really).

bgerstle · 2016-01-03T17:44:27Z

@nscoding addressed all of your comments, including the iPhone 6 screen size issue (and re-recorded reference image), squashed, and re-pushed

nscoding · 2016-01-03T23:20:08Z

testSuccessfulComparisonOfDeviceAgnosticReferenceImages tests are failing, it's looking for 320x568 (iPhone 5 right? but we didn't provide this one)

bgerstle · 2016-01-03T23:22:41Z

Ahh that's different now too, as it used to be 320x480, forgot to re-record
those
On Sun, Jan 3, 2016 at 18:20 Patrick Chamelo [email protected]
wrote:

testSuccessfulComparisonOfDeviceAgnosticReferenceImages tests are failing,
it's looking for 320x568 (iPhone 5 right? but we didn't provide this one)

—
Reply to this email directly or view it on GitHub
#137 (comment)
.

Use UIApplication.keyWindow instead of a manually-created & installed window when snapshotting a view w/o a window. This prevents keyWindow from being `nil` when generating device-agnostic reference image filenames. Accessing keyWindow is done through a category, so as to always throw an assertion if it is nil, instructing developers to make sure their tests have a host application w/ a key window.

bgerstle · 2016-01-04T17:01:08Z

@nscoding should be good now 🏁

nscoding · 2016-01-08T20:50:46Z

FBSnapshotTestCaseDemo/FBSnapshotTestCaseDemo/Launch Screen.storyboard

@@ -0,0 +1,50 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<document type="com.apple.InterfaceBuilder3.CocoaTouch.Storyboard.XIB" version="3.0" toolsVersion="9531" systemVersion="15C50" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" launchScreen="YES" useTraitCollections="YES" initialViewController="01J-lp-oVM">


Do we need this?

Yes, the launch screen fixes the issue where the app doesn't fill the
screen.

On Fri, Jan 8, 2016 at 12:50 Patrick Chamelo [email protected]
wrote:

In FBSnapshotTestCaseDemo/FBSnapshotTestCaseDemo/Launch Screen.storyboard
#137 (comment)
:

@@ -0,0 +1,50 @@
+
+

Do we need this?

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/ios-snapshot-test-case/pull/137/files#r49236579
.

add device agnostic tests & add helpful assertion for device agnostic tests

This happens when snapshotting the key window. This was introduced by facebookarchive#137.

facebook-github-bot added the CLA Signed label Dec 9, 2015

Grubas7 reviewed Dec 9, 2015
View reviewed changes

bgerstle changed the title ~~add device agnostic tests & use screen for filename size suffix~~ add device agnostic tests & add helpful assertion for device agnostic tests Dec 12, 2015

nscoding reviewed Dec 13, 2015
View reviewed changes

Grubas7 reviewed Dec 28, 2015
View reviewed changes

bgerstle force-pushed the master branch 2 times, most recently from 027784b to c2b8911 Compare December 29, 2015 16:04

nscoding reviewed Jan 3, 2016
View reviewed changes

bgerstle force-pushed the master branch from c2b8911 to e437a75 Compare January 3, 2016 17:43

bgerstle force-pushed the master branch from e437a75 to d6617c5 Compare January 4, 2016 01:25

nscoding reviewed Jan 8, 2016
View reviewed changes

nscoding added a commit that referenced this pull request Jan 8, 2016

Merge pull request #137 from bgerstle/master

e42af8b

add device agnostic tests & add helpful assertion for device agnostic tests

nscoding merged commit e42af8b into facebookarchive:master Jan 8, 2016

ksuther added a commit to ksuther/ios-snapshot-test-case that referenced this pull request Mar 24, 2016

Don't throw an exception if view == window

3cd462b

This happens when snapshotting the key window. This was introduced by facebookarchive#137.

ksuther mentioned this pull request Mar 24, 2016

Don't throw an exception if view == window #153

Merged

alanzeino mentioned this pull request Aug 11, 2017

deviceAgnostic strictly enforces key window requirement unnecessarily #235

Open


		@implementation UIApplication (StrictKeyWindow)

		- (UIWindow*)fb_strictKeyWindow {


		@implementation UIApplication (StrictKeyWindow)

		- (UIWindow *)fb_strictKeyWindow {

		@@ -0,0 +1,50 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>
		<document type="com.apple.InterfaceBuilder3.CocoaTouch.Storyboard.XIB" version="3.0" toolsVersion="9531" systemVersion="15C50" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" launchScreen="YES" useTraitCollections="YES" initialViewController="01J-lp-oVM">

add device agnostic tests & add helpful assertion for device agnostic tests #137

add device agnostic tests & add helpful assertion for device agnostic tests #137

Conversation

bgerstle commented Dec 9, 2015

facebook-github-bot commented Dec 9, 2015

facebook-github-bot commented Dec 9, 2015

bgerstle commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgerstle commented Dec 12, 2015

Grubas7 commented Dec 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgerstle commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grubas7 commented Dec 28, 2015

bgerstle commented Dec 28, 2015

Grubas7 commented Dec 29, 2015

bgerstle commented Dec 29, 2015

Choose a reason for hiding this comment

nscoding commented Jan 3, 2016

bgerstle commented Jan 3, 2016

nscoding commented Jan 3, 2016

bgerstle commented Jan 3, 2016

bgerstle commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment