Add unit tests - Githubissues

facebook / fishhook

A library that enables dynamically rebinding symbols in Mach-O binaries running on iOS.

BSD 3-Clause "New" or "Revised" License

5.18k stars 966 forks source link

Add unit tests #33

Closed kastiglione closed 5 years ago

kastiglione commented 8 years ago

This pull request adds unit tests, located in tests/. Testing is done via a small Xcode project, created solely for the purposes of these tests.

grp commented 8 years ago

Is Xcode and XCTest the best way to test this? A Makefile-built binary that exits 0 or 1 called from CI seems like it might be simpler than pulling in Objective-C, etc.

kastiglione commented 8 years ago

Is Xcode and XCTest the best way to test this?

I don't know how to define "best" here, but I was able to create it quickly and pretty much mindlessly, which was my criteria.

A Makefile-built binary that exits 0 or 1 called from CI seems like it might be simpler than pulling in Objective-C, etc.

I don't know how to do this targeting the simulator. Tests with the simulator would be useful in testing issues like #31. Also I get what you're saying at a high level, but I don't understand the argument about pulling in Objective-C (not in the context of basic tests for fishhook).

I don't have a strong interest in tests for this project, I just wanted something that would give me feedback that the two-levels namespace changes in #34 work to some degree. For example, I realized CI could be setup once tests were added, but I have no current intentions of doing that work.

modocache commented 8 years ago

For a practice C++ project I did the other day, I used googletest. It's fairly easy to set up using Git submodules, CMake, and a Makefile. In fact, I copied most of the setup from facebook/xcbuild -- only about 60-100 lines total, all very clean, no crufty Xcode XML. Thanks for the awesome example, @grp!

I think I'd recommend against using XCTest as well, but the simulator argument is enough to convince me! XCTest makes that very easy to do.

kastiglione commented 8 years ago

Thanks for the feedback @grp, @modocache.

To summarize:

I'm not arguing for Xcode or XCTest
I'm not arguing against simpler approaches (Makefile, etc)
Given my available time and knowledge, using Xcode is the approach I am willing to do
XCTest could be seen as providing some amount of integration testing

If people feel strongly against Xcode/XCTest, I'm fine to throw it away once #34 is done. I can always bring it back quickly in the future if I need/want it.

kastiglione commented 8 years ago

A Makefile-built binary that exits 0 or 1 called from CI seems like it might be simpler than pulling in Objective-C, etc.

I don't know how to do this targeting the simulator.

Thanks to @lawrencelomax, this can be done via xcrun simctl spawn.

grp commented 8 years ago

I was more talking philisophically than as a practical concern with adding any kind of tests :) Although the simctl approach sounds promising.

Sent from my iPhone

On Sep 9, 2016, at 02:33, Dave Lee notifications@github.com wrote:

A Makefile-built binary that exits 0 or 1 called from CI seems like it might be simpler than pulling in Objective-C, etc.

I don't know how to do this targeting the simulator.

Thanks to @lawrencelomax https://github.com/lawrencelomax, this can be done via xcrun simctl spawn.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/fishhook/pull/33#issuecomment-245694698, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ-PquXiPD859EJjrsjx2b4SOGnRDOMks5qoFTmgaJpZM4J3sf1 .

kastiglione commented 8 years ago

Maybe I'll create a with-tests branch and put this there, and not merge into master.

draveness commented 7 years ago

I've been watching this PR for some time. And really wanna know whether this PR will be merged or not. 😆

kastiglione commented 7 years ago

@Draveness I plan to create a branch, probably master-with-tests and keep this there.

facebook-github-bot commented 6 years ago

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

facebook-github-bot commented 6 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

kastiglione commented 6 years ago

@Megra this pull request was originally a complement to #34, I wanted to make sure I didn't break behavior. I probably won't be updating this, except if done to support #34.