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

@dblock => allow specta extras, and a simpler image folder setting #5

Merged
merged 7 commits into from
Jan 18, 2014

Conversation

orta
Copy link
Collaborator

@orta orta commented Jan 16, 2014

In reference to #1 I've simplified the API for setting the images directory to a c function that feels a lot like the rest of spectra. So you do setReferenceImageDir("blah") like expected, and the docs now state that this only has to be done once.

In reference to #2 I've extended the API for expecta if we see that you've included Specta in CocoaPods, this is a weak dependency because expecta is built to work without Specta, and so should our library. If you don't have it installed it will always fail.

This means we offer:

haveValidSnapshot()
recordSnapshot()

haveValidSnapshotNamed()
recordSnapshotNamed()

NSString *referenceImagesDirectory = [NSString stringWithFormat:@"%s", FB_REFERENCE_IMAGE_DIR];
[[EXPExpectFBSnapshotTest instance] setReferenceImagesDirectory:referenceImagesDirectory];
});
setReferenceImageDir(FB_REFERENCE_IMAGE_DIR);
Copy link
Owner

Choose a reason for hiding this comment

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

Should I be worried that this is something global? Should this be called something like setSnapshotTestsReferenceImageDir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this makes sense to me.

@dblock
Copy link
Owner

dblock commented Jan 16, 2014

I made a bunch of semi-cosmetic comments, mostly about documentation. Update/fix the ones you want, I'll merge.

#import "SpectaUtility.h"
#import "SPTExample.h"

NSString *sanitizedTestPath();
Copy link
Owner

Choose a reason for hiding this comment

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

Also, does this have to be pre-declared like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

@orta
Copy link
Collaborator Author

orta commented Jan 18, 2014

I've pushed the changes.

@dblock
Copy link
Owner

dblock commented Jan 18, 2014

@orta, you sure you pushed any changes? Don't see any updates...

@orta
Copy link
Collaborator Author

orta commented Jan 18, 2014

oops, nope, have now.

~/s/i/ios-snapshot-test-case-expecta (master↑) git push orta master
Counting objects: 29, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (16/16), done.
Writing objects: 100% (19/19), 1.95 KiB | 0 bytes/s, done.
Total 19 (delta 12), reused 0 (delta 0)
To https://github.com/orta/ios-snapshot-test-case-expecta.git
   57c0085..e739cef  master -> master

### App setup

* Specify the location of reference images with `setGlobalReferenceImageDir(FB_REFERENCE_IMAGE_DIR);`
* This is a global and needs to only be set once, as the test suite is ran alphabetically, you should ensure it's in the the first test suite that loads.
Copy link
Owner

Choose a reason for hiding this comment

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

Still think that this method is just weird and flaky. I'll play with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree.

@dblock
Copy link
Owner

dblock commented Jan 18, 2014

Merging, thx.

dblock added a commit that referenced this pull request Jan 18, 2014
@dblock => allow specta extras, and a simpler image folder setting
@dblock dblock merged commit 4a4d6bd into dblock:master Jan 18, 2014
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.

None yet

2 participants