Port Xcode 9 XCTAttachment API integration from FB repo by delebedev · Pull Request #19 · uber/ios-snapshot-test-case

delebedev · 2018-02-26T21:08:54Z

facebookarchive/ios-snapshot-test-case#230 (comment)

Author of original PR (@SiarheiFedartsou was Ok with delegating it to me 😄 )

===
I've signed CLA but it's taking a while to update it seems.

CLAassistant · 2018-02-26T21:09:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ SiarheiFedartsou
❌ delebedev
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

alanzeino · 2018-03-09T18:49:57Z

+- (void) addAttachmentsWithErrors:(NSArray<NSError *> *)errors identifier:(NSString *)identifier {
+#if defined(__IPHONE_11_0) || defined(__TVOS_11_0)
+    if (self.recordMode) {
+        UIImage* image = [_snapshotController referenceImageForSelector:self.invocation.selector identifier:identifier error:nil];


* aligned with the variable

alanzeino · 2018-03-09T18:51:40Z

  return nil;
 }

+- (void) addAttachmentsWithErrors:(NSArray<NSError *> *)errors identifier:(NSString *)identifier {


An internal method should be prefixed with _, there should be no space between the return type and the start of the method name, and the first opening brace should be on a new line

alanzeino · 2018-03-09T18:56:11Z

+    if (self.recordMode) {
+        UIImage* image = [_snapshotController referenceImageForSelector:self.invocation.selector identifier:identifier error:nil];
+        if (image) {
+            XCTAttachment *attachement = [XCTAttachment attachmentWithImage:image];


s/attachement/attachment

alanzeino · 2018-03-09T18:56:43Z


+- (void) addAttachmentsWithErrors:(NSArray<NSError *> *)errors identifier:(NSString *)identifier {
+#if defined(__IPHONE_11_0) || defined(__TVOS_11_0)
+    if (self.recordMode) {


I would pass this in to the method, say as ranInRecordMode

alanzeino · 2018-03-09T18:58:36Z

+    } else if (errors.firstObject != nil) {
+        NSError *error = errors.firstObject;
+        if (error.userInfo[FBReferenceImageKey] != nil) {
+            XCTAttachment *attachement = [XCTAttachment attachmentWithImage:error.userInfo[FBReferenceImageKey]];


s/attachement/attachment

alanzeino · 2018-03-09T18:58:40Z

+            [self addAttachment:attachement];
+        }
+        if (error.userInfo[FBCapturedImageKey] != nil) {
+            XCTAttachment *attachement = [XCTAttachment attachmentWithImage:error.userInfo[FBCapturedImageKey]];


s/attachement/attachment

alanzeino · 2018-03-09T18:58:44Z

+            [self addAttachment:attachement];
+        }
+        if (error.userInfo[FBDiffedImageKey] != nil) {
+            XCTAttachment *attachement = [XCTAttachment attachmentWithImage:error.userInfo[FBDiffedImageKey]];


s/attachement/attachment

alanzeino · 2018-03-09T19:10:17Z

  if (self.recordMode) {
-    return @"Test ran in record mode. Reference image is now saved. Disable record mode to perform an actual snapshot comparison!";
+    if (errors.count > 0) {
+      return [NSString stringWithFormat:@"Snapshot comparison failed: %@", errors.firstObject];


Is this possible? Looking at L102-L105, it is doing this:

BOOL referenceImageSaved = [self _compareSnapshotOfViewOrLayer:viewOrLayer referenceImagesDirectory:referenceImagesDirectory identifier:(identifier) tolerance:tolerance error:&error]; if (!referenceImageSaved) { [errors addObject:error]; }

When self.recordMode is on, compareSnapshotOfViewOrLayer:... the BOOL from the function relates to whether or not the reference image was saved.

alanzeino · 2018-03-09T19:11:33Z

-      FBSnapshotVerifyViewOrLayer(layer, identifier: identifier, suffixes: suffixes, tolerance: tolerance, file: file, line: line)
-    }
-
-    private func FBSnapshotVerifyViewOrLayer(_ viewOrLayer: AnyObject, identifier: String = "", suffixes: NSOrderedSet = FBSnapshotTestCaseDefaultSuffixes(), tolerance: CGFloat = 0, file: StaticString = #file, line: UInt = #line) {


I believe there's a good reason why SwiftSupport.swift uses its own FBSnapshotVerifyViewOrLayer function instead of leveraging the Objective-C one. This should probably stay as is, with modifications to support this new attachment feature though.

alanzeino · 2018-03-09T19:12:42Z

Not sure how much of these questions you can answer, since you weren't the original author. If you want, I can take this PR and make the change instead.

delebedev · 2018-03-10T10:23:16Z

@alanzeino I tried to understand what was the reasoning behind deleting part of swift code and how I can apply new attachement logic to FBSnapshotVerifyViewOrLayer in swift but due to quite a few ifs there I am just not certain how to do it right.

I've fixed coding style and some other issues you've flagged and would appreciate your help with reverting changes to SwiftSupport.swift.

delebedev · 2018-04-16T21:38:56Z

@alanzeino any chance you can have a look at this PR? I appreciate you maintaining the lib and I think this PR is very useful addition to the upstream

alanzeino · 2018-06-21T23:44:52Z

@delebedev I was waiting on you to revert the changes to SwiftSupport.swift and to reimplement this as needed there (you can revert a change in a file in git using git checkout HEAD^ path/to/the/file.swift — assuming you only have one commit on the branch, otherwise replace HEAD^ with the base commit).

Would you like me to implement this feature? I think since the original author isn't around it would be better if I just take a look at this.

delebedev · 2018-06-22T08:32:14Z

@alanzeino please do it, as I've written above in the comments I don't really know which parts of SwiftSupport should be reverted and which kept, better if you do it right yourself 👍

reidmain · 2019-06-25T23:33:22Z

This feature was added by #86

SiarheiFedartsou added 2 commits February 26, 2018 21:07

Implement Xcode 9 integration using XCTAttachement API

4ea21c4

Add tvOS support for Xcode 9 integration & some cleanup

25feb78

delebedev changed the title ~~Port Xcode 9 integration using XCTAttachment API from FB repo~~ Port Xcode 9 XCTAttachment API integration from FB repo Feb 26, 2018

alanzeino reviewed Mar 9, 2018

View reviewed changes

delebedev added 3 commits March 10, 2018 10:24

s/attachement/attachment

951bb0d

Conform to coding style

d45a6ff

Pass recordMode as parameter

b85e09e

This was referenced Jun 20, 2019

Save failed images as attachments with XCTest #83

Closed

Added support for Xcode 9 attachments. #86

Merged

reidmain closed this Jun 25, 2019

Uh oh!

Conversation

delebedev commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanzeino Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanzeino commented Mar 9, 2018

Uh oh!

delebedev commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delebedev commented Apr 16, 2018

Uh oh!

alanzeino commented Jun 21, 2018

Uh oh!

delebedev commented Jun 22, 2018

Uh oh!

reidmain commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

delebedev commented Feb 26, 2018 •

edited

Loading

CLAassistant commented Feb 26, 2018 •

edited

Loading

alanzeino Mar 9, 2018 •

edited

Loading

delebedev commented Mar 10, 2018 •

edited

Loading