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

Unit tests take too long to run #133

Closed
rhgills opened this issue Dec 13, 2013 · 17 comments
Closed

Unit tests take too long to run #133

rhgills opened this issue Dec 13, 2013 · 17 comments

Comments

@rhgills
Copy link
Contributor

rhgills commented Dec 13, 2013

~5 seconds on my machine.

We have some acceptance tests coexisting with the unit tests, perhaps we should move some of those into a different target.

@jasperblues
Copy link
Member

The biggest culprit by far would by TyphoonTestUtilsTests - it's testing asynch. You could just the values in there to be much, much smaller and should be good after that.

@rhgills
Copy link
Contributor Author

rhgills commented Dec 14, 2013

Ahh, interesting, thanks. How about deleting TyphoonTestUtils and their corresponding tests? We don't use them anywhere.

We should really try to avoid asynchronous unit tests at all costs. If it has to be asynchronous, it's really an integration or acceptance test and can stand to be run less often (by being on a different target).

@rhgills
Copy link
Contributor Author

rhgills commented Dec 14, 2013

Removed in df1457a. Tests run in .2 seconds now. We can always revert.

@rhgills rhgills closed this as completed Dec 14, 2013
@jasperblues
Copy link
Member

Its not for us, it’s for anyone who wants to write an integration test involving threads - eg a network request. . Similar to Kiwi, Cedar & Expecta asynch matchers.

Docs are here: https://github.com/typhoon-framework/Typhoon/wiki/Configuration-Management-%26-Testing

I use it all the time in my projects.

@jasperblues jasperblues reopened this Dec 14, 2013
@jasperblues
Copy link
Member

This is not an integration test.

This is a UNIT TEST for a utility that provides features to help with Integration Testing

@rhgills
Copy link
Contributor Author

rhgills commented Dec 15, 2013

Sorry about that Jasper, I wasn't aware that was part of the public API. My apologies. I got a little gung-ho, I should have waited to consult with you before removing that.

Having that utility in Typhoon seems a bit out of scope to me. I didn't at all expect that we would be providing features to users to help out with asynchronous integration tests. It doesn't seem like it fits with the rest of the library, although I appreciate the usefulness. I've spent lots of time trying to write asynchronous tests and have written some similar code myself. The same goes for the String and URL utilities which don't look like they are used in Typhoon itself. There aren't that many Utils at the moment, but I think the focus of Typhoon would benefit from being forked off into a seperate project. A JasperKit, perhaps? :)

Considering how to classify the TestUtilTests tests themselves— I think the interaction with NSRunLoop, and more generally with the concept of passing time, hints at another abstraction that should be introduced. It certainly makes testing difficult.

I consider asynchronous unit tests to be a code smell. Off the top of my head, to make the tests for the TestUtils synchronous I would consider introducing a class (let's call it Widget for now). The RealWidget would spin the NSRunLoop, where the FakeWidget used in the unit tests would do nothing, so that it immediately returns. Call the Widget a Waiter (confusing) or Sleeper (better), with the fake implementation being called FakeSleeper (LightSleeper? ;) ) and the real one RunLoopSleeper.

The FakeSleeper would have a method that you could use to verify it was called (numberOfSleeps). You could store a count of how many times its sleep method was called, and verify that in the unit tests (4 times every second you were supposed to wait). The RunLoopSleeper could take the interval to sleep as a constructor parameter, and spin a run loop for that long when sleep was called. The RunLoopSleeper would then be so simple that you didn't have to test it; except in an acceptance test, which would be similar to the tests you have now. But the unit tests using the FakeSleeper should give you enough confidence that nothing has broken when you change the TyphoonTestUtils. Then, the slower acceptance test could be run only on the build server at checkin.

I saw you lowered the timeout on the TyphoonTestUtils. The problem I've had with doing that before is that test might start failing intermittently, on slower machines than yours or even on the very same machine under heavy load. We can raise the timeout to a sufficiently large value like it was before to reduce but not eliminate this concern, but then it really slows down the tests. And if we start adding any more asynchronous unit tests that will exacerbate the issue. Obviously this is just one test and this is just 5 seconds (now less), but those seconds add up.

@jasperblues
Copy link
Member

Thanks for your feedback . . and will definitely keep Typhoon focused.

Wrt to moving TyphoonTestUtils though, the answer is no. This has been in Typhoon since day one, essentially. I value 100% Unit Test coverage. I also value Integration Tests, to see the units working together, and that's what this utility is for.

The test case is a unit test, because it is testing the contract of this utility - so providing asynch is a special case here.

When I opened-up Typhoon for contributions I knew that we'd get a lot of velocity and new ideas. When working in a team scenario like this there's always going to be one or two things that you don't like. . . (or if you really have a lot of different ideas, you can always fork and strike out in a new direction).

There's lots of other ways we can test this class if it becomes a problem - and simply moving it out of the project is not the direction I want to take.

@jasperblues
Copy link
Member

Relates to: #74

@rhgills
Copy link
Contributor Author

rhgills commented Dec 20, 2013

You're welcome!

Don't get me wrong, I agree that TyphoonTestUtils (and integration testing) has value (and for the record am okay with leaving it in the scope of the project). Let's set aside the asynchronicity of the tests for this class, which, you're right, is a technical problem with lots of possible solutions. I'm trying to understand where TyphoonTestUtils fits in with what we're trying to do.

This is the more philosophical question of what services we want Typhoon to provide to users. We bill ourselves as a dependency injection container. What I'm wrestling with is: where does making writing integration tests easier fit in with that mission? Or are the utils for our own future use, when testing parts of Typhoon?

@jasperblues
Copy link
Member

Typhoon core is indeed just a powerful dependency injection container. . . (with one or two extra utils in there). . Its main strength is providing a powerful core - the DI metadata is de-coupled, allowing for different types of expression - block-style, XML, macros, and future innovations.

I don't have plans to be like Spring and do everything but the kitchen sink (hang on! . . . a kitchen-sink module would be kinda cool right?), however I am planning on the following:

  • Some Spring-style integration test utils.
  • An AOP framework
  • Deeper IDE integration

And most importantly:

_your fantastic ideas and innovations_

. . . now that we have the typhoon-framework module in GitHub we can keep code organized into modules there. . This way we can publish a few different configs in CocoaPods: core, light, test-utils . . . maybe even AOP ? (AOP would be really simple to implement in Obj-C)

@rhgills
Copy link
Contributor Author

rhgills commented Dec 20, 2013

Aha, great idea! I like the modules idea, particularly as we grow more.

typhoon-core
typhoon-utils
typhoon-aop

etc.

https://github.com/rspec/rspec does this too.

This makes more sense to me, and it feels like the Utils we have fit better in this kind of a structure.

Regarding AOP, I think that would be fantastic. I'm not terribly familiar with the idea myself but am keen to learn. It's certainly a very interesting idea, and seems underserved. The biggest Obj-C implementation I could find: https://github.com/moszi/AOP-in-Objective-C

@jasperblues
Copy link
Member

A 2 minute summary of AOP

Sometimes we have requirements that break OOP. They can be infrastructure requirements (security, transaction management, sophisticated logging) or business requirements (every time a customer interacts with the store, track their behavior and make a genius-bar suggestion). . . . These are the kinds of requirements that cut-across many modules. . . In AOP-speak we call them cross-cutting concerns.

Why do these requirements break OOP. The three principles are encapsulation (aka modularization), polymorphism and inheritance. Using encapsulation we might have a class that's has a single-responsibility related to making purchases from a store.

With these cross-cutting concerns, the class has to do a whole bunch of stuff before/after its main responsibility.

  • Check authorization
  • Start a transaction
  • Log
  • Make a purchase
  • Send stats to genius bar
  • Commit transaction

. . and worse we have to repeat these everywhere. DI won't help us here - we could pass around an AuditService, TransactionManager, GeniusTracker, etc in the API of the service, but that would make for a very ugly interface.

So what AOP does is allow you to :

  • Write a single module that performs the cross-cutting conern
  • Identify all of the places in your code where this concern applies. . Do do this we use something called "A pointcut expression" and it can match on things like method name / signature, classname, "annotations" , etc. . . we then use the intercept pattern to weave those requirements in. . . Either:
  • Before method invocation
  • After method invocation
  • If method throws exception
  • around method invocation (the most flexible kind).

A great way to learn about AOP is to try it in Spring - they make it easy to get started.

@rhgills
Copy link
Contributor Author

rhgills commented Dec 22, 2013

Right, cross-cutting concerns! Thanks for the quick refresher. I'll definitely play around with Spring AOP a bit.

@jasperblues
Copy link
Member

Thanks for pointing that framework out - hadn't seen that one before. Only justin Spahr-summers' experimental one in libextobjc.. .

The simple proxy approach taken in the one you pointed out would suit us we'll if we're only going to intercept container-built instances - eg you can use AOP for any object built by Typhoon, (as opposed to any object full-stop). . What's missing in that lib seems to be the point-cut expression language.

Getting off topic here though ;)

@eriksundin
Copy link
Contributor

Feeding onto the OT-discussion. Have been thinking for a while of writing a proxy-based AOP lib for objective-c, inspired by Spring AOP which I've enjoyed working with quite a lot in the past.

Had some time in between christmas meals and produced a foundation at least:
https://github.com/eriksundin/apsara-aop

Still missing some stuff:

  • non-invasive aspect declaration (need to implement a protocol now for advice handling)
  • pointcut expressions, got the protocol there but only a simple "BlockMatchingPointcut" implementation yet. A simplified AspectJ-like expression impl should be added)

... and it needs some proper testing.

If we are interested in going with a proxy approach in Typhoon. I think it's probably a good idea to introduce a ComponentPostProcessor, and have an implementation that can automatically proxy components based on the declared aspects in the container.

Move the AOP discussion to a separate issue perhaps? :-)

@rhgills
Copy link
Contributor Author

rhgills commented Dec 31, 2013

Sure, sounds good to me. Or even to a separate typhoon-aop repo.

@jasperblues
Copy link
Member

Closing - trying to get the issues list down to a set of immediately actionable tickets.

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

No branches or pull requests

3 participants