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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic tests. Fixes #30. #31

Closed
wants to merge 16 commits into from
Closed

Add basic tests. Fixes #30. #31

wants to merge 16 commits into from

Conversation

squarefrog
Copy link
Contributor

I've added a small suite of basic view tests, using FBSnapshotTestCase. We should now be confident that any changes made will not affect existing installs.
screen shot 2014-05-18 at 01 18 33

馃嵒 馃憤

@squarefrog
Copy link
Contributor Author

Bahh I can't figure out why Travis is using an old version of Cocoapods. I'm going to sleep now but I'll ask the guys why this is failing tomorrow. Don't merge this in yet.... 馃挙

@squarefrog
Copy link
Contributor Author

\o/

thank you and good night!

@squarefrog
Copy link
Contributor Author

As you might be able to tell, something is not quite right with this. I'm going to use this pull request as a scratch pad until I figure out what's wrong.

Then I'll close it, scrap it and set it up again so I don't pollute the commit history of this project!

@tandavas
Copy link
Member

Wow, I never knew that there's such thing as view testing exists before! This FBSnapshotTestCase seems to be very useful for project since we are directly working with view stuffs. So, I think that adding this is definitely a great choice!

However, one thing that bothers me and I want to ensure that I get this right. If we decided to include this in our project, it will create pod dependencies in our project. Due to the dependencies caused, developers who wish to use our project by cloning (not using CocoaPods 馃槩 ) need to first install pod dependencies by pod install before they can actually use/see our buttons, which I think it won't be a good idea for those who wish to use our buttons only, not contributing.

So, in order to solve this problem, I think we should adopt the git workflow described in Pro Git, where we always have two branches -- master and develop branch.

  • The master branch stays as it is ( No FBSnapshotTestCase added ). So, the developers can clone the project and use it right away without needing to install the pod dependencies.
  • The develop branch has FBSnapshotTestCase. So, if anyone wants to contribute to us needs to checkout to this branch and make pull request to this branch only.

@squarefrog Does this make sense to you ? Will something like this work ?

And, thank you for introducing this to us! FBSnapshotTestCase is really cool, and your PR works!

cheers ! 馃槃 馃嵒 馃槂

@squarefrog
Copy link
Contributor Author

However, one thing that bothers me and I want to ensure that I get this right. If we decided to include this in our project, it will create pod dependencies in our project. Due to the dependencies caused, developers who wish to use our project by cloning (not using CocoaPods ) need to first install pod dependencies by pod install before they can actually use/see our buttons, which I think it won't be a good idea for those who wish to use our buttons only, not contributing.

I understand where you are coming from, but the project file actually stays as is: without any CocoaPod dependencies. In the podfile is the following

target "HTPressableButtonTests" do
  pod 'FBSnapshotTestCase'
end

This states that that dependency is linked to the HTPressableButtonTests target only. Therefore, if you just open the project and build and run it should still work fine. If you open up HTPressableButton.xcodeproj and run, it knows nothing of the CocoaPods dependencies so it just runs fine. If you attempt to run the tests from this project you do get a linker warning: ld: library not found for -lPods-HTPressableButtonTests. But this is solved by running pod install and opening the workspace file instead, as outlined in my README changes.

So, in order to solve this problem, I think we should adopt the git workflow described in Pro Git, where we always have two branches -- master and develop branch.

This workflow gets a 馃憤 from me, but I don't know that it'll work how you expect. If you merge changes in from develop, it will bring in the tests too. I do think that either way the develop branch is a smart idea. Master should always be the latest, most stable release.


Don't worry that this is on master, as I mentioned above, when I figure out how to get the test output in the Travis build, I'll essentially scrap my fork and re do the changes in one nice single commit. I created a quick sample project which seems to output test status just fine.

@herinkc
Copy link
Member

herinkc commented May 18, 2014

@squarefrog It seems that @tandavas opened the wrong file. About having two branches (master and develop), maybe we can keep it for later when our project becomes really big and hard to handle/follow up. I'm cool with having these tests. So how shall we proceed? Do we merge this first or wait for your cleaned up one? Please do take your time :)

@squarefrog
Copy link
Contributor Author

Well it's both your decision so I'll just go with whatever workflow you decide.

Don't merge this yet, I really want to clean it up first.

@squarefrog
Copy link
Contributor Author

I'm sorry for spamming this pull request with updates. I'm trying to ascertain why Travis is building fine, but refusing to run the tests.

@squarefrog
Copy link
Contributor Author

Woo hoo! Never have I been so happy to see a test fail :)

Ok looks like I'm done here. I'm going to close and clean this up. Expect a fresh tidy pull request soon.

@tandavas
Copy link
Member

@squarefrog Awesome work! Thank you @herinkc for pointing out that I opened the wrong file :x

I have been quite busy in the past few days ( and in the next few days !), but i'm sure @herinkc will sort things out here :D Nice PR btw 馃憤

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

3 participants