I just want to know what can be done to make this code smaller in a nice and easy way.
Tests
using System;
using Vegan.Test.TestClass;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Data.Entity.Validation;
using System.Linq;
namespace Vegan.Test
{
[TestClass]
public class IngredientsTest
{
[AssemblyInitialize]
public static void AssemblyInitialize(TestContext context)
{
System.Data.Entity.Database.SetInitializer(new DatabaseInitializer());
}
[TestMethod]
public void DeleteIngredient()
{
using (var ctx = new TestDbContext())
{
//Arrange
Ingredient ingredient = new TestClass.Ingredient();
ingredient.Id = 1;
ingredient.Name = "Watermelon";
ingredient.VegeOrVegetarian = VeganType.Both;
//Act
ctx.ingredients.Add(ingredient);
ctx.SaveChanges();
IngredientService service = new IngredientService(ctx);
service.removeIngredient(ingredient.Id);
service.saveChanges();
//Assert
Assert.AreEqual(0, ctx.ingredients.Count());
}
}
[TestMethod]
public void AddIngredient()
{
using (var ctx = new TestDbContext())
{
//Arrange
Ingredient ingredient = new TestClass.Ingredient();
ingredient.Id = 1;
ingredient.Name = "Watermelon";
ingredient.VegeOrVegetarian = VeganType.Both;
//Act
ctx.ingredients.Add(ingredient);
ctx.SaveChanges();
IngredientService service = new IngredientService(ctx);
var TestIngredientList = service.getIngredients();
//Assert
Assert.AreEqual(1, TestIngredientList.Count);
}
}
[TestMethod]
public void AddIngredientWithTooShortName()
{
using (var ctx = new TestDbContext())
{
//Arange
Ingredient ingredient = new Ingredient();
ingredient.Id = 1;
ingredient.Name = "";
ingredient.VegeOrVegetarian = VeganType.Both;
//Act
ctx.ingredients.Add(ingredient);
try
{
ctx.SaveChanges();
}
catch (DbEntityValidationException db)
{
foreach (var validationErrors in db.EntityValidationErrors)
{
foreach (var validationError in validationErrors.ValidationErrors)
{
//Assert
Assert.Fail("Property: {0} Error: {1}", validationError.PropertyName, validationError.ErrorMessage);
}
}
}
}
}
[TestMethod]
public void AddIngredientWithNullName()
{
using (var ctx = new TestDbContext())
{
//Arrange
Ingredient ingredient = new Ingredient();
ingredient.Id = 1;
ingredient.Name = null;
ingredient.VegeOrVegetarian = VeganType.Both;
ctx.ingredients.Add(ingredient);
//act
try
{
ctx.SaveChanges();
}
catch (DbEntityValidationException db)
{
foreach (var validationErrors in db.EntityValidationErrors)
{
foreach (var validationError in validationErrors.ValidationErrors)
{
//Assert
Assert.Fail("Property: {0} Error: {1}", validationError.PropertyName, validationError.ErrorMessage);
}
}
}
}
}
}
Other classes
using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Data.Entity;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace Vegan.Test.TestClass
{
public enum VeganType
{
Vegan,
Vegetarian,
Both
};
public class TestDbContext : DbContext,IDisposable
{
public virtual IDbSet<Ingredient> ingredients { get; set; }
}
public class DatabaseInitializer : System.Data.Entity.DropCreateDatabaseAlways<TestDbContext>
{
protected override void Seed(TestDbContext context)
{
context.SaveChanges();
}
}
public class Ingredient
{
public int Id { get; set; }
[Required(ErrorMessage ="Name is required")]
[StringLength(60,ErrorMessage ="Value must be between 3 and 60",MinimumLength =3)]
public String Name { get; set; }
public VeganType VegeOrVegetarian { get; set; }
}
public class IngredientService
{
private readonly TestDbContext _TestDbContext;
public IngredientService(TestDbContext TestDbContext)
{
_TestDbContext = TestDbContext;
}
public List<Ingredient> getIngredients()
{
return _TestDbContext.ingredients.ToList();
}
public void removeIngredient(int id)
{
var toRemove = _TestDbContext.ingredients.Find(id);
_TestDbContext.ingredients.Remove(toRemove);
}
public void saveChanges()
{
_TestDbContext.SaveChanges();
}
}
}
2 Answers 2
The best way to make a code smaller is searching for duplicate code. In your test methods, you have quite a lot of duplicate code that can be extracted to other methods.
For instance, you can move the context creation and destruction to the test initialize and cleanup methods:
[TestClass]
public class IngredientsTest
{
TestDbContext ctx;
[AssemblyInitialize]
public static void AssemblyInitialize(TestContext context)
{
System.Data.Entity.Database.SetInitializer(new DatabaseInitializer());
}
[TestInitialize]
public void Initialize()
{
ctx = new TestDbContext();
}
[TestCleanup]
public void Cleanup()
{
ctx.Dispose();
}
...
}
Another thing you can do is create a private method to create ingredients:
Ingredient CreateTestIngredient(int id, string name, VeganType type)
{
Ingredient ingredient = new Ingredient();
ingredient.Id = id;
ingredient.Name = name;
ingredient.VegeOrVegetarian = type;
return ingredient;
}
So now your first test would be something like this:
[TestMethod]
public void DeleteIngredient()
{
var testIngredient = CreateTestIngredient(1, "Watermelon", VeganType.Both);
ctx.ingredients.Add(testIngredient);
ctx.SaveChanges();
var serviceUT = new IngredientService(ctx);
serviceUT.removeIngredient(testIngredient.Id);
serviceUT.saveChanges();
Assert.AreEqual(0, ctx.ingredients.Count());
}
Note the variable serviceUT for the subject under test. Just repeat until all the duplicate code is gone.
@Carlos Alejo has already covered how to remove some of your duplication. I would add that with unit testing, there's often a balance to be found between removing all the duplication and still making it obvious what your tests are actually testing. That said, I'm going to focus on your tests in general.
AAA
I don't tend to put comments in my tests that section out the different steps (Act, Arrange, Assert) although I know that some people like to do so. If you are going to add the comments, then I'd be careful to make sure you split it to reflect what you're testing. Looking at one of your tests:
[TestMethod]
public void DeleteIngredient()
{
using (var ctx = new TestDbContext())
{
//Arrange
Ingredient ingredient = new TestClass.Ingredient();
ingredient.Id = 1;
ingredient.Name = "Watermelon";
ingredient.VegeOrVegetarian = VeganType.Both;
//Act
ctx.ingredients.Add(ingredient);
ctx.SaveChanges();
IngredientService service = new IngredientService(ctx);
service.removeIngredient(ingredient.Id);
service.saveChanges();
//Assert
Assert.AreEqual(0, ctx.ingredients.Count());
}
}
The test is called 'DeleteIngredient', which suggests that the target of the test is the DeleteIngredient call. Setting up the database, including adding the ingredient to be deleted and saving it belongs in the Arrange step, not in the Act step.
Test Naming
Your test names aren't particularly descriptive. For example AddIngredientWithNullName. This tells me nothing about the expectation of the test. Should adding an ingredient with a null name work or fail? It's unclear from the name, so I have to look at the test.
Bug?
Since the test name isn't explicit, it's unclear if this is the correct behaviour of your test, or not, however I would expect that a test called AddIngredientWithTooShortName is testing that it fails when supplied with a name that is too short. Looking at the implementation, you're actually testing:
When Adding an ingredient that has a short name
Ensure that no exceptions are thrown, OR,
IF an exception is thrown that it doesn't contain a Validation Error
Be clear about the level you're testing
I would have a particular test class focus on a level of the application that I want to test. It's unclear currently what level your class is focused on. You appear to be testing DbContext based calls (AddIngredientWithTooShortName) at the same time as testing Service level calls (AddIngredient). I would separate the tests into separate classes so that it is clearer what you're testing and why.
AddIngredient
In AddIngredient, you're saving an ingredient using the context, then retrieving it using a service, as I've said, it's unclear which element is being tested (service/context), however is checking that the number of records returned is 1, or should you really be checking that the record returned is the one that you've added?