----- "Adam Young" ayoung@redhat.com wrote:
On 07/12/2010 03:47 PM, Jesus Rodriguez wrote:
While working in ConsumerResourceTest recently, I noticed that it requires the DB. I started thinking (yeah imagine that) :) that maybe we shouldn't require the DB for anything except the curators.
Another thing I noticed is we have 3 ways of creating a test consumer:
- direct ctor i.e. new Consumer(...)
- TestUtil.createConsumer
- consumerCurator.create(...)
I prefer the first one or the second if it is used by many places aside from this particular Test class. The only thing that should call the curators are the curator tests the rest should use mockito to mock them out.
The ConsumerResourceTest requires a lot of infrastructure, like the database, curators, services etc. While guice makes some of this easier, it makes for a slow and brittle test.
I replaced the testCreateConsumer (and 2 others) with mocks. http://pastie.org/1041394
The original test 'looked' simpler: http://pastie.org/1041401 but it relied on a lot of stuff from the @Before method. And the problem I ran into was that this test uses a StubIdentityService which does not behave like our DefaultIdentity Service adapter nor any other.
My proposal is that we make the resource unit tests use mocks as much as possible to test the interaction. And the only thing that should use the DB should be the Curator tests.
Thoughts? comments?
I'm not on the project anymore, but I heartily endorse this approach.
Testability is one of the best side effects of using GUice.
Yes absolutely - I think this is a big point that Dmitri made a few weeks back, as well. We also get the benefit of faster tests, and hopefully more loosely coupled classes.
Along these lines, there are a few other things I would suggest (warning: personal opinion)
* Don't just autopilot a test<method_name> for single method - this is unlikely to get all branching logic and doesn't take multiple inputs into account. Especially if there are null input cases or any arithmetic that has to happen, write a bunch of different test cases with crazy inputs so that we know that the method behaves appropriately.
* Along those lines - we don't have to keep a 1-to-1 mapping between test class and class. It could make more sense to split tests between multiple test classes to make the setup/teardown more appropriate to similar situations.
* As much as possible, make a single assertion per test case. I know this sounds crazy at first, but it really does make test cases more useful when a test fails - there is seldom a need to go through and debug the test case to find which assertion is failing and why.
- Justin
candlepin mailing list candlepin@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/candlepin