Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Swift Package Manager #126

Merged
merged 7 commits into from
Aug 23, 2021
Merged

Conversation

bfernandesbfs
Copy link
Contributor

Support Swift Package Manager

Add support to SPM

Changed

  • Add Package.swift
  • Project folders organization for compatibility to structure Swift Package Manager
  • Fix Headers Objc
  • Change load resource into test target

@bfernandesbfs
Copy link
Contributor Author

Hi @alanzeino!

Can you analyse this PR?
I'm fixed quantity of changes to better code review and reduce impact of move files

.gitignore Outdated
Pods
Podfile.lock
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed

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 revert this

@@ -28,7 +28,11 @@
// OTHER DEALINGS IN THE SOFTWARE.
//

#if SWIFT_PACKAGE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't a SwiftPM project use angle bracket imports? Does it not support Header Search Paths?

Copy link
Contributor Author

@bfernandesbfs bfernandesbfs May 13, 2020

Choose a reason for hiding this comment

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

Objective-C has this in common with C/C++; the quoted form is for "local" includes of files (you need to specify the relative path from the current file, e.g. #include "headers/my_header.h"), while the angle-bracket form is for "global" includes -- those found somewhere on the include path passed to the compiler (e.g. #include <math.h>) and SPM use local path

@@ -7,8 +7,8 @@
*
*/

#import <FBSnapshotTestCase/FBSnapshotTestCase.h>
#import <FBSnapshotTestCase/FBSnapshotTestController.h>
#import "Public/FBSnapshotTestCase.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious about this change; can we not specify which files are Public and Internal via the Package.swift?

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 try specify, but in Package.swift the property publicHeadersPath: have an unique path for file .h and conflict with file .swift

import XCTest

#if SWIFT_PACKAGE
@_exported import FBSnapshotTestCaseCore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a private Swift feature and not one we should use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need exported the public header of submodules, this case we can use only this in test target

Copy link
Collaborator

Choose a reason for hiding this comment

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

why can't people just import FBSnapshotTestCaseCore if they need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case FBSnapshotTestCaseCore is a private module

Comment on lines +196 to +197
NSURL *url = [[NSURL fileURLWithPath: @__FILE__] URLByDeletingLastPathComponent];
NSString *path = [[url URLByAppendingPathComponent: [NSString stringWithFormat:@"%@.%@", name, type]] path];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this is because the image isn't visible in the bundle? Is there a better way to do this because this seems like a hack that finds the temp directory storing the code file and using that path to find the image. Was bundle nil before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, today it's not possible load bundle resource in the SPM, but has open issue in swift .org about this. I' created this hack as temp solution, if bundle fail we can see in the unit test

Choose a reason for hiding this comment

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

Might be worth adding a comment in the code explaining that?

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 read file with using reserved key __FILE__, this create relative path from file test

Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment in the code linking to the open issue would be suffice

Copy link

Choose a reason for hiding this comment

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

@@ -1,20 +0,0 @@
PODS:

Choose a reason for hiding this comment

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

I don't think the Podfile.lock should be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah it's used in the demo because the demo is a real app depending on FBSnapshotTestCase, therefore it needs a Podfile and a Podfile.lock

Comment on lines +196 to +197
NSURL *url = [[NSURL fileURLWithPath: @__FILE__] URLByDeletingLastPathComponent];
NSString *path = [[url URLByAppendingPathComponent: [NSString stringWithFormat:@"%@.%@", name, type]] path];

Choose a reason for hiding this comment

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

Might be worth adding a comment in the code explaining that?

@jsorge
Copy link

jsorge commented Sep 8, 2020

It's been 3+ months since the last activity here. Is this still being considered for merging?

@ralewis85
Copy link

Heading into November with no update @alanzeino

@alanzeino
Copy link
Collaborator

Heading into November with no update @alanzeino

I can't merge anything in this repo anymore, I asked the team at Uber to move the repo to my ownership but didn't hear back (other than telling me to fork it).

When I worked to get ownership of FBSnapshotTestCase from Facebook to Uber back in the day, I had to fork and churn every user of the repo with a new podspec, cocoapod name, etc. If I forked this today, everyone would have to do that again, and it's just not worth it. My wish is that Uber transfer me this repo since they don't maintain it anymore, but they have might their reasons to keep it (cc @briankhsieh), and I don't work there anymore so I don't have much influence on that part. Funny thing is now I work at Facebook I could resurrect the original repo, but it would be madness to make everyone churn their podspec back to the original...

My advice? Use the pointfree snapshot library. If you have Objective-C needs then you don't need SwiftPM support anyway, since I don't think SwiftPM supports Objective-C anyway?

@liuxuan30
Copy link

liuxuan30 commented Dec 8, 2020

My advice? Use the pointfree snapshot library

Could you share more steps to do so? Not understand how to get the latest update.

Besides is there anything we can do to help you get back the repo? It's ridiculous for Uber to take this repo as a hostage...

@alanzeino
Copy link
Collaborator

My advice? Use the pointfree snapshot library

Could you share more steps to do so? Not understand how to get the latest update.

https://github.com/pointfreeco/swift-snapshot-testing

@liuxuan30
Copy link

liuxuan30 commented Dec 14, 2020

My advice? Use the pointfree snapshot library

Could you share more steps to do so? Not understand how to get the latest update.

https://github.com/pointfreeco/swift-snapshot-testing

Thanks, it is a new library. I'm still want to keep the current one though. Anything we could do to help you to get back the repo?

@liuxuan30
Copy link

I'm hoping @briankhsieh could see this. If you could either help to transfer the ownship or you update it. Thank you.

@liuxuan30
Copy link

liuxuan30 commented Dec 14, 2020

@alanzeino

Funny thing is now I work at Facebook I could resurrect the original repo, but it would be madness to make everyone churn their podspec back to the original...

It might be easier just to change a podspec. The point is this is a great library, I think people would like to do the change again once they understand the situation here. It will be a pity that we loose this library only because you cannot get the repo and people don't want to churn.

@alanzeino
Copy link
Collaborator

alanzeino commented Dec 17, 2020

Why don't you download the source of the PR (https://github.com/bfernandesbfs/ios-snapshot-test-case/tree/feature/spm) to your repo and then reference the package in your Package.swift with a relative path .package(url: "Vendor/iOSSnapshotTestCase")

I haven't tried this but it should work and unblock you from having iOSSnapshotTestCase in SwiftPM

@alanzeino
Copy link
Collaborator

Update: I'm working again on getting ownership of the repo again. In the meantime, I'm developing on the project again on my fork:

https://github.com/alanzeino/ios-snapshot-test-case

I've merged in @bfernandesbfs' changes and I'm testing them now. Hopefully within the month I'll have the ability to maintain this repo again and I'll merge my fork back into here.

@bfernandesbfs
Copy link
Contributor Author

Great news \o/

…an be manually set to the root folder where reference images are stored, which is needed for Swift Package Manager. After this change there are now 4 ways of setting reference image directories instead of 3:

 1. Set the preprocessor macro FB_REFERENCE_IMAGE_DIR to a double quoted c-string with the path. This only works for Objective-C tests.
 2. Set an environment variable named FB_REFERENCE_IMAGE_DIR with the path. This takes precedence over the preprocessor macro to allow for run-time override.
 3. (New way) Set `FBSnapshotTestCase.bundleResourcePath` as the root folder where reference images are stored.
 4. Keep everything unset, which will cause the reference images to be looked up inside the bundle holding the current test, in the Resources/ReferenceImages_* directories.
@ianrhamilton
Copy link

@alanzeino how's it going with the ownership transfer?

…undleresourcepath

Added bundleResourcePath variable to FBSnapshotTestController which c…
@dogo
Copy link

dogo commented Jun 9, 2021

@alanzeino how's it going with the ownership transfer?

@alanzeino
Copy link
Collaborator

Should know more in a few weeks

alanzeino added a commit that referenced this pull request Aug 24, 2021
alanzeino added a commit that referenced this pull request Aug 24, 2021
@dogo
Copy link

dogo commented Aug 25, 2021

🥳

@alanzeino
Copy link
Collaborator

alanzeino commented Aug 25, 2021

For anyone needing this you can now use this via your Package.swift as so:

.package(url: "https://github.com/uber/ios-snapshot-test-case.git", from: "7.0.0")

or via Xcode:

https://github.com/uber/ios-snapshot-test-case/blob/main/README.md#swift-package-manager

@bfernandesbfs
Copy link
Contributor Author

Hey @alanzeino thanks by you help with this PR 🙌🏻🎉

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

Successfully merging this pull request may close these issues.

10 participants