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

Initial tests setup #422

Closed
wants to merge 4 commits into from
Closed

Initial tests setup #422

wants to merge 4 commits into from

Conversation

yas375
Copy link
Contributor

@yas375 yas375 commented Sep 26, 2015

Closes #414

Here is some initial work on adding unit tests to the project.

To run unit tests

  1. Open ChartsDemo/ChartsDemo.xcworkspace (not .xcodeproj!). Also please use the workspace if you are contributing to the library.
  2. Choose a simulator or a device with 2x scale screen. I.e. iPhone 5, 6, 6S,... but not 6S Plus because it has 3x screen scale. This is because snapshot view tests were recorded on such device and stored images are ..@2x.png.

Useful resources for contributors

About snapshot testing

Snapshot testing means generating images of the view and storing them as reference images. When tests are running another image is generated from the view and compared with the reference images.
This is achieved with these two libs:

When you need to regenerate a snapshot image replace next line

expect(chartView).to(haveValidSnapshot())

with

expect(chartView).to(recordSnapshot())

Note about warnings when running unit tests

When you run tests from Xcode 7 you will probably see 4 warnings like the next one:

ld: warning: directory not found for option '-F/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator9.0.sdk/Developer/Library/Frameworks'

2015-09-26_1541

This has been fixed in CocoaPods here: CocoaPods/CocoaPods#4219 and will be included in a next CocoaPods release.

Next steps (happy to discuss)

  • integration with Travis CI
  • integration with Coveralls
  • add Charts to the demo project as CocoaPod
  • add UI tests against existent Demo project to test user interactions
  • more unit tests. Would be nice to try to add a unit test for every issue being fixed. It will reduce chances of possible regressions in future.

I'm open for any feedback and happy to answer any questions regarding decisions being made while working on this.

@yas375
Copy link
Contributor Author

yas375 commented Sep 27, 2015

Force pushed the branch. I was using wrong version of cocoapods by a mistake.

@liuxuan30
Copy link
Member

bravo! First time knowing snapshot testing. It seems like using image recognition? How about the accuracy? And, it's fully automated?
Say I had a commit that introduced a bug that cause the bars to be moved 10 px to the left. Will it be able to find it?

@liuxuan30
Copy link
Member

BTW, what's the special for nimble? It's saying Nimble matchers for FBSnapshotTestCase. Forgive me first time know this, and quite useful

@yas375
Copy link
Contributor Author

yas375 commented Sep 28, 2015

It seems like using image recognition? How about the accuracy? And, it's fully automated?

It's just pixel by pixel comparison. Here is the source if you'd like to see how exactly it's implemented :)
It is automated, yes)

Say I had a commit that introduced a bug that cause the bars to be moved 10 px to the left. Will it be able to find it?

Exactly! :) You will get a failing test.

what's the special for nimble? It's saying Nimble matchers for FBSnapshotTestCase.

There is a library by FB called FBSnapshotTestCase. It has FBSnapshotTestController class which does all important things: renders a UIView or CALayer to an image, can store this image into a reference directory (when you run tests in record mode), read a reference image from the disk and compares it pixel by pixel to another image... Also there is FBSnapshotTestCase which subclasses XCTestCase and makes it easy to use FBSnapshotTestController by instantiating it for you, adding some useful macros for nicer expectations syntax. But in order to use it your specs need to be subclasses of FBSnapshotTestCase. So it works if you don't use Quick (or Kiwi/Specta/Cedar in obj-c). You just need to change superclass for your test cases. But we'd like to use Quick so we can group tests using context, describe and other nice things. As a result we can't subclass FBSnapshotTestCase. Here Nimble-Snapshots comes for the rescue! It provides a nice matcher and an easy way to run a test in record mode to capture an image. I hope I answered your question :)

Forgive me first time know this, and quite useful

No worries at all! Keep asking 👍

@danielgindi danielgindi reopened this Oct 1, 2015
@danielgindi
Copy link
Collaborator

Oops... This was closed by mistake, by mentioning the wrong issue number

@danielgindi
Copy link
Collaborator

@yas375 I'm having trouble with this, as currently if the user has not installed the required modules for unit tests - it does not compile the framework at all...

@yas375
Copy link
Contributor Author

yas375 commented Oct 1, 2015

@danielgindi libs required for unit tests are added using CocoaPods and they are stored in Pods directory. There is no requirement to install anything else. You even don't have to install CocoaPods unless you want to update some library used for testing. The only requirement added is that you need to open workspace file, not an xcodeproj file. Do you open ChartsDemo/ChartsDemo.xcworkspace?

@danielgindi
Copy link
Collaborator

Have you tried openingthe xcodeproj and compiling normally? It fails...
I'll look into it later

‏בתאריך יום חמישי, 1 באוקטובר 2015, Victor Ilyukevich <
notifications@github.com> כתב:

@danielgindi https://github.com/danielgindi libs required for unit
tests are added using CocoaPods and they are stored in Pods directory.
There is no requirement to install anything else. You even don't have to
install CocoaPods unless you want to update some library used for testing.
The only requirement added is that you need to open workspace file, not an
xcodeproj file. Do you open ChartsDemo/ChartsDemo.xcworkspace?


Reply to this email directly or view it on GitHub
#422 (comment)
.

@yas375
Copy link
Contributor Author

yas375 commented Oct 1, 2015

Have you tried openingthe xcodeproj and compiling normally? It fails...

If you are talking about ChartsDemo/ChartsDemo.xcodeproj then yes - it' is expected behavior. It's missing libraries added via CocoaPods. That's why you need to use ChartsDemo/ChartsDemo.xcworkspace which also includes Pods.xcodoproj which includes required libraries.

Charts/Charts.xcodeproj is untouched and I can open and compile it without any issues.


Here is a description of what CocoaPods make behind the scenes.


Please don't hesitate to ask in case you have any questions. I'm really interested in getting this merged in and improving test coverage of the library :)

@yas375
Copy link
Contributor Author

yas375 commented Oct 13, 2015

@danielgindi sorry for reminding, any thoughts on this work? Want me to rebase this on master?

@danielgindi
Copy link
Collaborator

I still have the compatibility issue with native Xcode. When compiling - it shows red errors about "No such module 'Quick'".
I want to try to move this to UI testing of Xcode 7 - what do you think?

@pmairoldi
Copy link
Collaborator

Ya that is because you need to run pod install and use the workspace instead of the project.

@pmairoldi
Copy link
Collaborator

UI testing would be good since it would be easier to test scrolling the chart and such. I haven't used it much at all so I don't know what is possible.

@yas375
Copy link
Contributor Author

yas375 commented Oct 15, 2015

I still have the compatibility issue with native Xcode. When compiling - it shows red errors about "No such module 'Quick'".

Are you sure you are opening xcworkspace and not the xcodeproj file? You shouldn't see this if you are opening workspace. The workspace has two projects in it.

2015-10-15_1504

2015-10-15_1454

If you'd like we can have a call with screen sharing. I'd be happy to help :)

Ya that is because you need to run pod install and use the workspace instead of the project.

You don't need to run pod install because Pods directory is commited as well. This is mainly to make the setup for collaborators as easy as possible. See more reasons here.


I want to try to move this to UI testing of Xcode 7 - what do you think?

It's a good thing to have. I don't think it's correct to say "I'd like to move from unit tests to UI tests". Both types of tests are important and both have pros and cons. I usually make more accent on unit tests because they give quicker feedback and help to design nice code. Especially if you are trying to to TDD. It's usually a lot harder to write a UI test first. You can also see that I had a note in the PR's description about adding UI tests in future ;)

@danielgindi
Copy link
Collaborator

Well, I WANT to open the xcodeproj file, as the users open the project
file- they do not use a workaround to open the project...
If adding tests mean that the users have to have pods and install Quick and
Nimble then we do not want that, we want to keep things simple

‏בתאריך יום שישי, 16 באוקטובר 2015, Victor Ilyukevich <
notifications@github.com> כתב:

I still have the compatibility issue with native Xcode. When compiling -
it shows red errors about "No such module 'Quick'".

Are you sure you are opening xcworkspace and not the xcodeproj file? You
shouldn't see this if you are opening workspace. The workspace has two
projects in it.

[image: 2015-10-15_1504]
https://cloud.githubusercontent.com/assets/217368/10528880/0a65c928-734e-11e5-828a-a866a76c07e1.png

[image: 2015-10-15_1454]
https://cloud.githubusercontent.com/assets/217368/10528696/9e9aa9bc-734c-11e5-9ddd-dbb2a856ae88.png

If you'd like we can have a call with screen sharing. I'd be happy to help
:)

Ya that is because you need to run pod install and use the workspace
instead of the project.

You don't need to run pod install because Pods directory is commited as
well. This is mainly to make the setup for collaborators as easy as
possible. See more reasons here
https://guides.cocoapods.org/using/using-cocoapods.html#should-i-check-the-pods-directory-into-source-control

.

I want to try to move this to UI testing of Xcode 7 - what do you think?

It's a good thing to have. I don't think it's correct to say "I'd like to
move from unit tests to UI tests". Both types of tests are important and
both have pros and cons. I usually make more accent on unit tests because
they give quicker feedback and help to design nice code. Especially if you
are trying to to TDD. It's usually a lot harder to write a UI test first.
You can also see that I had a note in the PR's description about adding UI
tests in future ;)


Reply to this email directly or view it on GitHub
#422 (comment)
.

@pmairoldi
Copy link
Collaborator

Moving to a workspace does have advantages. For example, right now there are 2 projects (charts and demo). Those could be in the same workspace. Otherwise if you really want to just keep the project then using Carthage to install quick would work to do that. Otherwise moving to xctest is doable.

@yas375
Copy link
Contributor Author

yas375 commented Oct 15, 2015

I don't think using workspace is a "workaround". It's feature Xcode has for ages. I can't remember when I was opening xcodeproj for the last time... I'm looking for a workspace file by default when I need to open a new project... Why is it considered a bad thing? I was aiming to make this all simple: "open workspace & run tests". You don't need to install anything else. All helpful libraries for testing are already there. Lots (or maybe most?) of developers are using CocoaPods and already familiar with it. It shouldn't be a problem.

@pmairoldi
Copy link
Collaborator

Based on the types of issues that are opened for this repository I believe that a lot of users that are using this library are somewhat beginners to iOS and it might be hard for them to understand workspaces and cocoapods if they have never seen it before. It is possible to use quick and such without cocoapods so it shouldn't be an issue to move to just a project.

@danielgindi
Copy link
Collaborator

Well I think that opening the workspace file is nog desired, otherwise
Apple would not have hidden it inside the project file.

And yes the beginners would never even try to Right-Click a project file
and look for a workspace, even if you write the instructions on their
forehead and put them in front of a mirror. And not only beginners... I've
found that many people just don't try anything out of their comfort zone,
they do what they are used to.

Anyway, for me if I download a project, double click the projectand
compile- and it does not work- it is considered a bad thing. Things SHOULD
"just work".

Also many people don't like CocoaPods. I for once don't use it. I feel like
it messes up to much with the project settings, and there are cases where
you have to create a new project file and drag everything to it to start
over, just becuase the CocoaPods was not very reliable and messed up
something. It's not a "standard", it's not part of the standard dev IDE, so
I wouldn't want to force it on people.
On

@pmairoldi
Copy link
Collaborator

You don't have to right click a project file to find a workspace. It's just like a project file. Just click it in the folder structure. But I understand your other points and they are very valid. So Carthage it is or move to xctest.

@yas375
Copy link
Contributor Author

yas375 commented Oct 16, 2015

Does anybody know if FBSnapshotTestCase and Nimble-Snapshots can be added using Carthage? I don't see anything about Carthage in theirs readmes...

@danielgindi I didn't use Carthage before, but can spend some time to redo this pull request using Carthage if you are interested in.

@pmairoldi
Copy link
Collaborator

As long as there is an Xcode project in the root of th github repo it should just work. Even if they don't mention it in the readme

@pmairoldi pmairoldi closed this Oct 26, 2015
@yas375 yas375 deleted the tests branch October 26, 2015 00:17
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.

Unit tests? ;)
4 participants