CRUD integration test, is it ok?

I want to test my CRUD operation to a MySql database (I use dapper as my ORM).

This is what I wrote with xUnit:

    private IPaymentRepository paymentRepository;
private Payment testPayment;
private string connectionString = "Database=..."; //Test DB

public PaymentRepositoryTest()
{
paymentRepository = new PaymentRepository(connectionString);
//My model has private setters so I have a builder to initialize the object.
testPayment = new Payment.Builder()
.Id(0)
.Description("Test")
.Type(1)
.WithPercentageDiscount(10)
.WithAdvancePercentageDiscount(0)
.WithInstallments(new int[] { 30 }).Build;
}


CREATE

[Fact]
public void Insert_Payment_ReturnId()
{
long id = 0;
using(var transaction = new TransactionScope())
{
id = paymentRepository.Insert(testPayment);
}

Assert.True(id > 0);
}


READ

[Fact]
public void Get_Payment_ReturnPaymentFromDb()
{
using (var transaction = new TransactionScope())
{
long id = paymentRepository.Insert(testPayment);
var payment = paymentRepository.GetById(id);

Assert.NotNull(payment);
}
}


UPDATE

I don't really like this code, having private setter here is unfortunate because I have to recreate the object. I'm also not really sure about checking only one field.

    public void Update_Payment()
{
long id = 0;

const string UPDATED_DESCRIPTION = "Test2";

using (var transaction = new TransactionScope())
{
id = paymentRepository.Insert(testPayment);

var updatedPayment = new Payment.Builder()
.Id(id)
.Description(UPDATED_DESCRIPTION)
.Type(1)
.WithPercentageDiscount(10)
.WithAdvancePercentageDiscount(0)
.WithInstallments(new int[] { 30 }).Build();

paymentRepository.Update(updatedPayment);

updatedPayment = paymentRepository.GetById(id);

Assert.Equal(UPDATED_DESCRIPTION, updatedPayment.Description);
}
}


DELETE

Just like the update, I don't like having to recreate the object just to have the proper ID.

[Fact]
public void Delete_Payment_ReturnNull()
{
using (var transaction = new TransactionScope())
{
long id = paymentRepository.Insert(testPayment);

var paymentToDelete = new Payment.Builder()
.Id((int)id)
.Description("Test")
.Type(1)
.WithPercentageDiscount(10)
.WithAdvancePercentageDiscount(0)
.WithInstallments(new int[] { 30 }).Build();

paymentRepository.Delete(paymentToDelete);

var payment = paymentRepository.GetById(id);

Assert.Null(payment);
}
}


The code of course works and I get the proper results, but I feel I'm repeating basically the same test over and over especially in Update and Delete.

Is this integration code good ? How can I make it better ?

• "Is this integration code good?" Did you test it? Are you running into any troubles?
– Mast
Aug 26 '20 at 16:26
• Yes it does work. There no problem, but since this is the first time I do integration test I'm not sure if it make sense at all. It seems to me that I'm basically testing the same thing over and over. 3 test out of 4 calls GetById, is that good ? Aug 26 '20 at 17:24
• In End2End tests it is a quite common practice to issue direct commands against the database server. Your application can rely on any ORM, but your tests can use a different channel to pre-populate, verify and cleanup. Sometimes E2E tests are rewritten in different language than the SUT to avoid re-usage of any component. If you wish you can use this concept on this level of testing as well. Aug 27 '20 at 10:15

1 Answer

Adding automated integration tests is a great way to take testing to the next level, and what you've written is really good for a first attempt. Here are some pointers/suggestions that might help you along your way.

It's almost inevitable that your tests will end up effectively testing the same thing multiple times. As you've identified, most of your tests use the GetById method, and in fact all of them Insert a payment. The only potential problem this causes is that if one of those methods stops working you suddenly have most/all of your tests failing and it may not be immediately obvious where the problem is. In the grand scheme of things, this isn't that much of an issue as you'll almost certainly be able to figure out where the problem is by looking at the test output and/or by stepping through the test with a debugger.

Although it seems like your tests are very similar, a lot of the similarities are down to them sharing common setup steps, e.g. inserting a payment so you have something to get/update/delete. It is absolutely fine to have this sort of duplication in unit/integration tests, and again is inevitable in most circumstances. As the number of tests grows, you might find that it makes sense to pull out some of the setup into a separate method that's used by multiple tests. This is also fine to do, as long as it doesn't affect the readability of the test, so avoid using generic method names like Setup() and instead prefer clear (if slightly verbose) names like CreateRepositoryWithSinglePayment(testPayment).

The fluent payment builder makes it hard to know which properties are required and which are optional (if any). I can't advise a great deal on this without seeing the Payment or PaymentBuilder classes, but if all of the properties are required, consider just creating a Payment constructor that assigns parameter values to readonly properties. Then there's no doubt about what values are required to create a valid Payment.

One thing to be aware of when integration testing is that it's often easy to end up with shared state between tests. Here, you are inserting a payment into your database as part of every test and this is not always deleted afterwards (or at least there's no tear-down code included in your post). Since you are actually persisting something in a database, it will therefore exist across tests unless explicitly deleted after each test is run. This could result in some very confusing test behaviour in the future as your tests might not be testing what you intended! You should add a tear-down step by implementing IDisposable and deleting every payment in your test database inside the Dispose() method. This ensures that every test will start from the same fresh state.

On a similar note, be aware that you can get strange behaviour when running integration tests in parallel if they share the same data store(s). You're fine at the moment because by default xUnit runs test cases within the same class in serial, but if you had another test class using the same database these would be run in parallel by default. There are ways to control this behaviour, but it's more something to be aware of as this shouldn't be an issue given your current test organisation.

You shouldn't need to provide the entire Payment object to delete it from the database. If you can read a payment by its Id using the GetById method then you should also be able to delete a payment by its Id. This should simplify the Delete test case somewhat.

There are plenty of other things you could test here. You've got tests for the 'happy' paths, i.e. the paths where everything goes as expected, but there are certainly lots of edge cases (or unexpected cases) that you could also test. Maybe you've already considered some/all of these, but off the top of my head:

• What happens when you insert a payment that already exists in the database?
• What happens when you try to read a payment that does not exist in the database?
• What happens when trying to update a payment that doesn't exist in the database?
• Can you update the payment with a null description? What happens if the description is really long or contains special characters? You'd probably want to use a [Theory] with [InlineData] to enumerate several different test cases if doing this.
• What happens when updating properties other than the description?
• What happens when trying to delete a payment that doesn't exist in the database?
• Oh man, thanks for the feedback. I really appreciate it. For the dispose part I use new TransactionScope() and never call .Complete() so the database should always be in a clear state. I tested it and it seems to work. Aug 27 '20 at 6:38