6

I'm dealing with a lot of functions like the one below and I'm unsure of what kind of unit tests should be written for them.

public void UpdateEmployeeClockIn(int employeeId)
{
var sql = string.format("Update Employee set ClockIn = GETUTCDATE() where employeeId={0}", employeeId);

db.ExecuteNonQuery(sql);
}

I'm thinking I could refactor this method into one that maybe returns a sql statement and I could run a few tests on the sql statement to make sure there is a correct where clause etc. Any ideas? I keep running into code like this where I simply do not know how or what to test for.

8

It depends on the layer of abstraction between this code and the database.

If an abstraction there is, and you can actually put a mock instead of the object which actually accesses a real database, then your unit tests may check that the correct SQL query is sent to the mock when the method is called with an arbitrary argument, say 123. Given the actual code and the fact that unit testing is white box testing, there is nothing else here to test. Things could be different if you were sanitizing your inputs, for instance by checking that employeeId cannot be negative or zero.

If there are no abstractions, you can't write unit tests for this method without refactoring it first. You can write automated tests, but those would be integration, system, functional or acceptance tests, not unit tests. In this case, you'll run:

  • A similar test which passes an ID of an existing employee, and checks that the database entry was changed.

  • A test which uses an ID which doesn't exist.

  • A test which runs on a database set in read-only mode, or an unreachable database (both cases are especially important if you have a cluster).


Note that:

  • While in the actual piece of code, SQL Injection is impossible, this is still a wrong way to use the database. When you use parametrized queries, the database engine can cache the execution plan. In your case, you are virtually preventing it from doing this.

  • You're not sanitizing your inputs. This could be intentional, if you want to let your database return an error on invalid values, but usually, it indicates that the developer simply forgot to do it. This could make it easier to perform DOS attacks against your database.

  • You are not catching any exception. Is it intentional? It may be: for instance, you may prefer passing the exception up the stack in order for the business layer or presentation layer to deal with it. However, there are cases, where you have to deal with some exceptions right now. For instance, if accessing a cluster, you may have to deal with read-only instances or plain connection loss; instead of simply showing the error to the user, the method could handle the exception gracefully by retrying the query or doing what makes sense to do in a specific situation.

  • Sql inhection teoretically is possible in OP's case too. Negative integer have a sign, which can be overwritten. – Fabio Nov 26 '16 at 16:00
  • 1
    You are not caching any exception. Why catching exception is required? Exceptions is good - very good things - they are thrown and show you that something goes wrong. In my understanding you need to catch exceptions only in top level of your application, which usually is UI and only for two reasons: 1. Show more friendly error message for the user. 2. Loggiing – Fabio Nov 26 '16 at 16:44
  • @Fabio: about the negative sign, do you mean setting a custom culture with custom formatting rules? I believe if a hacker has that sort of permissions at server level, there is not much which could be done in terms of protection against SQL Injection. – Arseni Mourzenko Nov 26 '16 at 19:59
  • @Fabio: about catching exceptions, I edited my answer. You don't have to catch exceptions necessarily in the UI: you catch them where it makes sense. If you can handle them in the data access layer, do it there. If DAL has no idea what to do with an exception, then it will indeed propagate until the global try/catch block which will handle logging and graceful termination of the application. – Arseni Mourzenko Nov 26 '16 at 20:01
  • 1
    @Fabio one thing you may want to consider is creating custom exceptions to help abstract away the specifics of the DAL implementation. Often I'll have a PersistanceException class. That way, I don't leak SqlException up into my business domain. I'll catch the SqlException and wrap it in a PersistanceException before re throwing it. Makes life easier if you're evolving a system from simple in memory repositories to files to an actual database. The client code never cares that you've swapped out the DAL. – RubberDuck Nov 27 '16 at 17:04
2

In addition to @Arseni's answer.

Your method depends on the database. Even db variable is an interface you still passing query which depends on the database type or database version etc.

Unit tests are tests which are separated from any IO operations (File system, Web service or database). Because unit tests are tests which developer need to run every 1 minute and execution time of tests need to be very short (seconds). While tests with involved IO operation takes longer time.

Tests which depend on IO operations have place to be, but they called "Integration tests".

So if your method is part of Data layer then you can write integration test where you can run another SELECT query and assert that ClockIn column isn't NULL after updating.

But if your method is part of Business logic then for proper unit testing you need abstract this method to the interface for example. Then while testing a method which will use UpdateEmployeeClockIn you can assert that UpdateEmployeeClockIn method was called with correct parameter.

Your method have another case which can make your testing almost impossible. You using system's datetime function ClockIn = GETUTCDATE(). Function GETUTCDATE() always return different value. You cannot create expected value against which your result can be asserted.
Because you using time from database then I suggest move time method to the dedicated function in your interface. Which can be mocked up and return defined expected value for the testing

public interface IDataService
{
    void UpdateEmployeeClockIn(int employeeId, DateTime timeIn);
    DateTime GetCurrentTime();       
}

Then in your Business logic layer

public class EmployerManager
{
    private readonly IDataService _dataService;

    public Employer(IDataService dataService)
    {
        _dataService = dataService;
    }

    public void UpdateClockinWithCurrentTime(int employerId)
    {
        DateTime currentTime = _dataService.GetCurrentTime();
        _dataService.UpdateEmployeeClockIn(employerId, currentTime);
    }
}

In this case you can write unit tests for the method UpdateClockinWithCurrentTime where you can tests through mock of IDataService that UpdateEmployeeClockIn was executed with correct parameters.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.