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

Remove necessity of declaring PIN_APP_EXTENSIONS macro #72

Merged
merged 4 commits into from
Mar 5, 2016
Merged

Remove necessity of declaring PIN_APP_EXTENSIONS macro #72

merged 4 commits into from
Mar 5, 2016

Conversation

maicki
Copy link
Collaborator

@maicki maicki commented Mar 3, 2016

No description provided.


+ (BOOL)isAppExtension
{
return [[[NSBundle mainBundle] executablePath] containsString:@".appex/"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be dispatch_once'd?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you write a unit test for this? I think this is doable if you separated this into 2 methods, once of which just took a string and return a BOOL, and isAppExtension could call that one with [[NSBundle mainBundle] executablePath]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep this will be dispatched_once. I will look into how we could integrate a unit test for it.

@benasher44
Copy link
Contributor

Here's the smoke test I thought of to help ensure this can't break again:

  • Add a new extension target to PINCache.xcodeproj
  • Add a new test target that tests the extension target
  • Add a test that just initializes the cache objects and tries to call the startBackgroundTask method, which in a non-extension context would create a background task. If this breaks, the test should crash/fail.

@benasher44
Copy link
Contributor

Does this sound realistic/doable?

@benasher44
Copy link
Contributor

Also, @martin-gearzero's final comment in #68 seems preferable to inspecting any of the main bundle's paths.

@maicki
Copy link
Collaborator Author

maicki commented Mar 4, 2016

@benasher44 @garrettmoon Added the dispatch_once for the is app extension check and added a basic share extension target that just initializes PINCache and calls a method that would create a PINBackgroundTask.

I couldn't really figure out how we can test PINCache usage within the share extension via a test target. Happy for any help!

@@ -1120,11 +1134,17 @@ - (instancetype)init
+ (instancetype)start
{
PINBackgroundTask *task = [[self alloc] init];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@garrettmoon You think we should just return nil here and not allocate any PINBackgroundTask if isAppExtension == YES?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a good call to me.

@garrettmoon
Copy link
Collaborator

@maicki It might be possible to do with a UI automation test? Force the extension to run in Safari via a share extension?

I think this is a good start though and worth merging.

garrettmoon added a commit that referenced this pull request Mar 5, 2016
Remove necessity of declaring PIN_APP_EXTENSIONS macro
@garrettmoon garrettmoon merged commit 46d56ac into pinterest:master Mar 5, 2016
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.

3 participants