tags:

views:

174

answers:

8

I'm trying to unit test in a personal PHP project like a good little programmer, and I'd like to do it correctly. From what I hear what you're supposed to test is just the public interface of a method, but I was wondering if that would still apply to below.

I have a method that generates a password reset token in the event that a user forgets his or her password. The method returns one of two things: nothing (null) if everything worked fine, or an error code signifying that the user with the specified username doesn't exist.

If I'm only testing the public interface, how can I be sure that the password reset token IS going in the database if the username is valid, and ISN'T going in the database if the username is NOT valid? Should I do queries in my tests to validate this? Or should I just kind of assume that my logic is sound?

Now this method is very simple and this isn't that big of a deal - the problem is that this same situation applies to many other methods. What do you do in database centric unit tests?

Code, for reference if needed:

public function generatePasswordReset($username)
{
    $this->sql='SELECT  id
                FROM    users
                WHERE   username = :username';

    $this->addParam(':username', $username);
    $user=$this->query()->fetch();

    if (!$user)
        return self::$E_USER_DOESNT_EXIST;
    else
    {
        $code=md5(uniqid());
        $this->addParams(array(':uid'        => $user['id'],
                               ':code'       => $code,
                               ':duration'   => 24 //in hours, how long reset is valid
                              ));

        //generate new code, delete old one if present
        $this->sql ='DELETE FROM password_resets WHERE user_id=:uid;';
        $this->sql.="INSERT INTO password_resets (user_id, code, expires)
                     VALUES      (:uid, :code, now() + interval ':duration hours')";

        $this->execute();
    }
}
A: 

In general one might "mock" the object you are invoking, verifying that it receives the expected requests.

In this case I'm not sure how helpful that is, you amost end up writing the same logic twice ... we thought we sent "DELETE from password" etc. Oh look we did!

Hmmm, what did we actually check. If the string was badly formed, we woudln't know!

It may be against the letter of Unit testing law, but I would instead test these side-effects by doing separate queries against the database.

djna
+1  A: 

You can break it down some more, that function is doing alot which makes testing it a bit tricky, not impossible but tricky. If on the other hand you pulled out some smaller extra functions (getUserByUsername, deletePasswordByUserID, addPasswordByUserId, etc. Then you can test those easily enough once and know they work so you don't have to test them again. This way you test the lower down calls making sure they are sound so you don't have to worry about them further up the chain. Then for this function all you need to do is throw it a user that does not exist and make sure it comes back with a USER_DOESNT_EXIST error then one where a user does exist (this is where you test DB comes in). The inner works have already been exercised else where (hopefully).

Pete Duncanson
A: 

Testing the public interface is necessary, but not sufficient. There are many philosophies on how much testing is required, and I can only give my own opinion. Test everything. Literally. You should have a test that verifies that each line of code has been exercised by the test suite. (I only say 'each line' because I'm thinking of C and gcov, and gcov provides line-level granularity. If you have a tool that has finer resolution, use it.) If you can add a chunk of code to your code base without adding a test, the test suite should fail.

William Pursell
+3  A: 

The reason this function is more difficult to unit test is because the database update is a side-effect of the function (i.e. there is no explicit return for you to test).

One way of dealing with state updates on remote objects like this is to create a mock object that provides the same interface as the DB (i.e. it looks identical from the perspective of your code). Then in your test you can check the state changes within this mock object and confirm you have received what you should.

ire_and_curses
How much of the "database" do you need to implement in order to be able to check those state changes? Parsing SQL? Maintaining a some data mimicing tables? My view is that using a real database ends up being less work.
djna
I do agree there are difficulties with unit testing in this particular case. You are essentially testing meta-programming (creating SQL to dynamically manipulate a complex state model). As others have said, the function could be simplified.In this particular example, without refactoring, I would test in two stages. 1->Write an SQL query to do the update, and check (by hand) that it works on a real DB. 2->Write the unit test for this code, hard coding the SQL query you tested in part 1, and check that the SQL you send to the DB matches the form of your tested query.
ire_and_curses
+6  A: 

The great thing about unit testing, for me at least, is that it shows you where you need to refactor. Using your sample code above, you've basically got four things happening in one method:

//1. get the user from the DB
//2. in a big else, check if user is null
//3. create a array containing the userID, a code, and expiry
//4. delete any existing password resets
//5. create a new password reset

Unit testing is also great because it helps highlight dependencies. This method, as shown above, is dependent on a DB, rather than an object that implements an interface. This method interacts with systems outside its scope, and really could only be tested with an integration test, rather than a unit test. Unit tests are for ensuring the working/correctness of a unit of work.

Consider the Single Responsibility Principle: "Do one thing". It applies to methods as well as classes.

I'd suggest that your generatePasswordReset method should be refactored to:

  • be given a pre-defined existing user object/id. Do all those sanity checks outside of this method. Do one thing.
  • put the password reset code into its own method. That would be a single unit of work that could be tested independently of the SELECT, DELETE and INSERT.
  • Make a new method that could be called OverwriteExistingPwdChangeRequests() which would take care of the DELETE + INSERT.
p.campbell
I get what you're saying, but even then I'm still going to be in the same boat. If I can test all 2/3 methods individually, how is that different from testing one method that does the same thing?This is in my User model class. Assuming I put my sanity checks in the controller, I would just have to move my testing to the controller side.
ryeguy
A: 

Databases are global variables. Global variables are public interfaces for every unit that uses them. Your test cases must therefore vary inputs not only on the function parameter, but also the database inputs.

JeffP
A: 

If your unit tests have side-effects (like changing a database) then they have become integration tests. There is nothing wrong in itself with integration tests; any automated testing is good for the quality of your product. But integration tests have a higher maintenance cost because they are more complex and are easier to break.

The trick is therefore to minimize the code that can only be tested with side effects. Isolate and hide the SQL queries in a separate MyDatabase class which does not contain any business logic. Pass an instance of this object to your business logic code.

Then when you unit test your business logic, you can substitute the MyDatabase object with a mock instance which is not connected to a real database, and which can be used to verify that your business logic code uses the database correctly.

See the documentation of SimpleTest (a php mocking framework) for an example.

Wim Coenen
A: 

Unit tests serve the purpose of verifying that a unit works. If you care to know whether a unit works or not, write a test. It's that simple. Choosing to write a unit test or not shouldn't be based on some chart or rule of thumb. As a professional it's your responsibility to deliver working code, and you can't know if it's working or not unless you test it.

Now, that doesn't mean you write a test for each and every line of code. Nor does it necessarily mean you write a unit test for every single function. Deciding to test or not test a particular unit of work boils down to risk. How willing are you to risk that your piece of untested code gets deployed?

If you're asking yourself "how do I know if this functionality works", the answer is "you don't, until you have repeatable tests that prove it works".

Bryan Oakley