views:

338

answers:

7

Hello,

I didn't find much in tutorials on this specific question..

So I have a class called 'Job' which has public ctors and a single public Run() function. Everything in the class is private and encapsulated in the class. (You may remember an older post here on this http://stackoverflow.com/questions/1590953/testing-only-the-public-method-on-a-mid-sized-class, which replies helped me greatly)

This Run() method does a bunch of things - takes an excel file as input, extracts data out of it, sends a request to a third party data vendor, takes the result and puts it in the database and logs the begining / end of the job.

This Job class uses 3 seperate interfaces / classes inside it's run method, (IConnection will connect to the third party vendor and send the request, IParser will parse the results, and IDataAccess will save the results to the database). So now, the only real logic inside my Run() method is extracting out the excel input and sending it down the chain of the other classes. I created 3 mock classes and use DI on the Job class ctor and everything is fine and dandy...

Except - I'm still a little lost on how the heck to test my Run() method - because it's void and doesn't return anything...

In this case, should I add a return value to the Run() method that returns how many records were extracted from the Excel file? Since this is the only logic done in that function now.. this wouldn't be handled in real code, but would be in the unit tests... which seems a bit smelly to me - but i'm a newb as far as true TDD is concerned...

Second question - should I created a fourth class called IExcelExtractor, which does that logic for me? Or is this a bit of class explosion??

Even if I did the latter, how would I test my Run() function if it returns void and all of its work is being carried out by mocked objects which really do nothing? I could understand if my function had a meaningful return value... but in this case I'm a but confused.

Thanks so much for reading through all this if you made it this far.

+1  A: 

If the only thing that your run() method does is invoke other objects, then you test it but verifying that the mocks were invoked. Exactly how you do this depends on the mock package, but generally you will find an "expect" method of some sort.

Do not write code within your run() method that will track its execution. If you are unable to verify the method's operation based on its interactions with collaborators (the mocks), that indicates a need to rethink those interactions. Doing so also clutters the mainline code, increasing maintenance costs.

kdgregory
+2  A: 

When you inject a mock, you pass to the Run class's constructor a test class that you will ask if the test passed. For example, you could test that the IParser mock got the correct request given the excel file you passed in the constructor. You can do this via your own class, and collect the results in it and test what it collected, or you could do this via a mocking framework that gives you ways of expressing such testing without constructing a class.

I see that you tagged your question with tdd, but in true tdd you don't really get this question (you do, but asked differently) because you build the test first, which defines the interface, instead of building the class interface and then thinking how are you going to test this thing. The need to test drives the design. You still use the same techniques (and likely end up with the same design in this case), but the question would have come out a bit different.

Yishai
I added "unit-testing" tag to be more precise to what you say.
Robert Koritnik
+1  A: 

I've asked a similar question.

Though (sense over theory) I do think that some methods don't need unit tests as long as (and until) they:

  • don't return any values
  • don't change internal state of the class or system that can be checked
  • don't rely on anything else (as input or output) than your mock

If their functionality (ie. sequence of calls) is vital you will have to verify that internal functionality is met though. Meaning that you have to verify (using your mocks) that those methods have been called with correct parameters and correct sequence (if it matters).

Robert Koritnik
Most decent mock frameworks allow you to assert the flow of calls is correct through the mock objects passed in by providing mechanisms to fail the test if mock functions aren't called in the correct order.
workmad3
@workmad3: That's right. I changed my answer a bit to make this clear in case you understood I thought they don't...
Robert Koritnik
+3  A: 

You mention you have mock implementations of the 3 classes/interfaces being used in in your self-contained class...

Why not create some known values to return from your mock IConnection, just pass all of them through your mock IParser, and store them in your mock IDataAccess - then in the test check to see that the results in the mock IDataAccess match the expected results from the input from the mock IConnection after running through the run() method?

Edited to add an example -

Application interfaces / classes:

public interface IConnection {
    public List<Foo> findFoos();
}

public interface IParser {
    public List<Foo> parse(List<Foo> originalFoos);
}

public interface IDataAccess {
    public void save(List<Foo> toSave);
}

public class Job implements Runnable {
    private IConnection connection;
    private IParser parser;
    private IDataAccess dataAccess;

    public Job(IConnection connection, IParser parser, IDataAccess dataAccess) {
        this.connection = connection;
        this.parser = parser;
        this.dataAccess = dataAccess;
    }

    public void run() {
        List<Foo> allFoos = connection.findFoos();
        List<Foo> someFoos = parser.parse(allFoos);
        dataAccess.save(someFoos);
    }
}

Mocks / Test classes:

public class MockConnection implements IConnection {
    private List<Foo> foos;

    public List<Foo> findFoos() {
        return foos;
    }

    public void setFoos(List<Foo> foos) {
        this.foos = foos;
    }
}

public class MockParser implements IParser {

    private int[] keepIndexes = new int[0];

    public List<Foo> parse(List<Foo> originalFoos) {
        List<Foo> parsedFoos = new ArrayList<Foo>();
        for (int i = 0; i < originalFoos.size(); i++) {
            for (int j = 0; j < keepIndexes.length; j++) {
                if (i == keepIndexes[j]) {
                    parsedFoos.add(originalFoos.get(i));
                }
            }
        }
        return parsedFoos;
    }

    public void setKeepIndexes(int[] keepIndexes) {
        this.keepIndexes = keepIndexes;
    }
}

public class MockDataAccess implements IDataAccess {
    private List<Foo> saved;

    public void save(List<Foo> toSave) {
        saved = toSave;
    }

    public List<Foo> getSaved() {
        return saved;
    }
}

public class JobTestCase extends TestCase {

    public void testJob() {
        List<Foo> foos = new ArrayList<Foo>();
        foos.add(new Foo(0));
        foos.add(new Foo(1));
        foos.add(new Foo(2));
        MockConnection connection = new MockConnection();
        connection.setFoos(foos);
        int[] keepIndexes = new int[] {1, 2};
        MockParser parser = new MockParser();
        parser.setKeepIndexes(keepIndexes);
        MockDataAccess dataAccess = new MockDataAccess();
        Job job = new Job(connection, parser, dataAccess);
        job.run();
        List<Foo> savedFoos = dataAccess.getSaved();
        assertTrue(savedFoos.length == 2);
        assertTrue(savedFoos.contains(foos.get(1)));
        assertTrue(savedFoos.contains(foos.get(2)));
        assertFalse(savedFoos.contains(foos.get(0)));
    }
}
Nate
Thanks Nate and everyone else. This seems like the most simple and obvious answer - but again, my IDataAccess has just a void() function which saves the result to the database. Would I have to add extra methods to my IDataAccess class (like returnRecentInsertCount()), then make an accessor for the instance of my mocked IDataAccess object, to check that value? OR should I add functionality (have IDataAccess.Insert() return count, and have job.Run propogate this count to a return value?) Again - void return / private member problem..
dferraro
@dferraro - you could do that (add extra methods yourself), but you'll be far better off using a Mocking framework that does this for you, if there are any available for your development environment. (And there are many free ones for most popular languages.)
Jeff Sternal
I'm still struggling with having the mocks 'talk' to each other properly without doing actual logic.OK, so I have the IConnection Mock just return a 'fake' flat file, by returning a string like ",,,,,"..My mock parser then accepts this string... but.. now what? he accepts a string and returns a data table. If I actually put logic in place to turn this fake string into a datable, I am now re-writing my actual parsing function and it's no longer a mock =(...
dferraro
dferraro
I think an important thing to keep in mind is that - based on your description - Job.Run won't require many interesting unit tests. You would probably write tests (for example) that verify it does what you expect when your IConnection returns proper results (which is to say, you would test that it calls IParser with those results) and a test that verifies it does what you expect when it returns null (the correct behavior isn't obvious from what you've written). It's not very exciting, but the purpose of Job.Run is simply to coordinate the behavior of collaborators, not perform logic itself.
Jeff Sternal
I added an example to illustrate the process - though by this point there's been a lot more discussion and you probably already have similar code.
Nate
Thanks so much again Nate. This explained it to me very well - for some reason I was thinking that there wouldn't have to be logic in the mock objects - but in cases like this, there must be logic to 'connect' the mocks, which in my case is nearly as complicated as the actual objects themselves. Made me realize in my scenario, it's actually simpler and easier to teach the group how to use NMock then it is to actually create these mocks manually. This is an awesome thread. Thanks again all
dferraro
+2  A: 

The idea of TDD is basically that by adhering to it, you are going to write code that is easy to test, because you first write the tests against an interface that lacks the implementation, and then write the code to make the tests pass. It seems that you have written the Job class before the tests.

I figured out that you can change the Job.Run implementation, in which case if you want the code to be testable, you should do something to it to be able to read the values you need to test for.

Eemeli Kantola
+1  A: 

First of all, as your run() method is kind of a workflow launcher, and you have several steps that need to be executed inside the workflow, I think you need more than one unit test, maybe even to split the existing class into several smaller ones, each corresponding to a step in the workflow.

This way you'll be also testing in isolation each step of the workflow, if at any moment the workflow fails, these smaller unit tests will allow you to identify easier the faulty part ( the step that fails )

But maybe this is already the case, I don't know if you don't have already this kind of division.

Anyway, back to your run() method, the answer lies in your question :

This Run() method does a bunch of things - takes an excel file as input, extracts data out of it, sends a request to a third party data vendor, takes the result and puts it in the database and logs the beginning / end of the job

So you have:

  • some input data ( from the excel file )

  • some "output" data or rather the result of the wokflow.

In order for your run() to succeed, you need to check that:

a) the request has been sent to the third party and/or a result has been received. I don't know which of those will be easier to check, but at least you could probably log the request/response and check the logs ( in the unit test ) for the operation being executed. This will ensure that the whole workflow is executed ( we can imagine the scenario where the correct data are present in the db at the end of the workflow, but not because the run worked correctly, but because the data was already there or something along those lines - if a purge before the test does not delete some data for instance )

b) check the database for the correct values ( in respect toothe input values ) being inserted/updated in the proper places as a result of the workflow.

c) you could even check the logs you're mentioning ( beginning/end of the job ) for the validity of the delay between the two operations ( if you know that it can't work faster than say 10sec, if your log says job done in 1 sec you'll know something went wrong ... )


Edit: as a first test before a) above you might want to check also the input data, as you might imagine errors there as well ( missing excel file, or content has changed so you're having a wrong input, etc )

Billy
+4  A: 

What you're describing is often called behavior verification (as opposed to state verification). It's got its proponents and detractors, but for several categories of classes it's the only game in town if you want to unit test.

To unit test a class whose behavior is limited to interacting with collaborators, you typically pass mock collaborator objects that are instrumented in a way that allows you to verify their methods have been called in the way you expect.

If you were to do this by hand (yuck!) for the classes you mentioned in your question, you might create a MockParser class that implements IParser and adds properties that record if and how its methods were called.

It's better to use mocking framework that will create the mocks on the fly, specify expections on them, and verify those expectations.

I've been using NMock2 these days, and the tests look something like this:

// 'mockery' is the central framework object and Mock object factory
IParser mockParser   = mockery.NewMock<IParser>();

// Other dependencies omitted
Job     job          = new Job(mockParser);

// This just ensures this method is called so the return value doesn't matter
Expect.Once.On(mockParser).
    .Method("Parse").
    .WithAnyArguments().
    .Will(Return.Value(new object()));

job.Run();
mockery.VerifyAllExpectationsHaveBeenMet();
Jeff Sternal
A mocking framework is really the way to go for this.
ColinD
dferraro
BTW, I do have a question - what does mockery.VerifyallExpectationsHaveBeenMet() do??? I saw in my code, exceptions were thrown mid-unit test when the expected results or parameters were off. So then what's that final call at the end doing?
dferraro
Glad to hear you like it! What's more, I think some of the new mock frameworks (Rhino, Moq, etc.) are even better, but they're only usable for the 3.5 framework and up. As for configuring expectations on your mocks and verifying them, that's a pretty big topic. This answer to an earlier question of mine was very helpful to me when I was trying to figure it out: http://stackoverflow.com/questions/1107217/when-to-expect-and-when-to-stub/1107240#1107240. With NMock, when you use the Verify... method, you have to make sure you've told the Mockery about every call to every mock object it manages.
Jeff Sternal
Doh! I figured it out =). NMock will verify the expects/arguments at 'run time', and VeryifyAllExpectationsHaveBeenMet() will make sure that anything you DID specify to the mock framework really WAS called. Since I was doing 'reverse' TDD that Verify() call was never failing, until I commented out one of my calls and retained the Expect() method then watched it fail =)
dferraro