Testing classes

Question

I'm just getting into testing and wanted to see if someone can tell me if I'm writing tests correctly. I'm using C#, NUnit, & Should.

This is the class I'm testing:

 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
using System.Linq;
using System.Reflection.Emit;
Using VM.Models.Common;
namespace Vm.Models.CustomerNS
{
 public class Customer 
 {
// ReSharper disable once InconsistentNaming
 public int ID { get; set; }
 public List<Address> Locations { get; set; }
 public List<User> Users { get; set; }
 public List<BuyerNum> BuyerNumbers { get; set; }
 public Dictionary<string, string> VehicleList { get; set; }
 public List<Contact> Contacts { get; set; }
 public string Name { get; set; }
 public Customer()
 {
 Locations = new List<Address>();
 Users = new List<User>();
 BuyerNumbers = new List<BuyerNum>();
 Contacts = new List<Contact>();
 VehicleList = new Dictionary<string,string>();
 }
 public void AddLocation(AddressType addtype, string addressName, string streetAddress, string city, string state, string zip, string country)
 {
 var addressId = 0;
 var lastOrDefault = Locations.LastOrDefault();
 if (lastOrDefault != null) addressId = lastOrDefault.AddressId + 1;
 Address newAddress = new Address
 {
 AddressId = addressId,
 AddressName = addressName,
 AddressType = addtype,
 City = city,
 State = state
 };
 if (streetAddress != null) newAddress.StreetAddress = streetAddress;
 if (zip != null) newAddress.Zip = zip;
 Locations.Add(newAddress);
 }
 public void RemoveLocation(int addressId)
 {
 Debug.Assert(Locations != null, "Locations != null");
 var addressid = Locations.Where(x => x.AddressId == addressId);
 var enumerable = addressid as IList<Address> ?? addressid.ToList();
 if (enumerable.Count() != 1) { throw new InvalidOperationException("Cannot Delete"); }
 Locations.Remove(enumerable.First());
 }
 public void ModifyLocation(int addressId, AddressType addtype, string addressName, string streetAddress, string city, string state, string zip, string country)
 {
 var addressid = Locations.Where(x => x.AddressId == addressId);
 var enumerable = addressid as Address[] ?? addressid.ToArray();
 if (enumerable.Count() != 1) { throw new InvalidOperationException("Cannot Delete"); }
 Locations.Remove(enumerable.First());
 }
 public Address GetLocation(int addressId)
 {
 return Locations.First(x => x.AddressId == addressId);
 }
 public List<Address> GetAllLocations()
 {
 return Locations;
 }
 }
}

Testing Class

 using System.Linq;
using Vm.Models.Common;
using VM.Models.CustomerNS;
using NUnit.Framework;
using Should;
// ReSharper disable once CheckNamespace
namespace vm.Models.CustomerNS.Tests
{
 [TestFixture()]
 public class CustomerTests
 {
 [Test()]
 public void CustomerTest()
 {
 var customerObject = new Customer() { Name = "Hello World!!" };
 customerObject.ShouldNotBeNull();
 Assert.AreEqual(customerObject.Name, "Hello World!!");
 }
 [Test()]
 public void AddLocationTest()
 {
 var customerObject = new Customer() { Name = "Hello World!!" };
 customerObject.AddLocation(AddressType.Business, "Hello Location", "123 Street Name", "Hello City", "HS", "10001", "US");
 customerObject.Locations.First().ShouldNotBeNull();
 Assert.AreEqual(customerObject.Locations.First().City, "Hello City");
 }
 [Test()]
 public void RemoveLocationTest()
 {
 var customerObject = new Customer() { Name = "Hello World!!" };
 customerObject = GenerateMultipleLocations(5, customerObject);
 var inintialCount = customerObject.Locations.Count;
 var custObjCopy = customerObject.Locations.First();
 var thirdlocation = customerObject.Locations[3];
 customerObject.RemoveLocation(1);
 customerObject.Locations.Count.ShouldEqual(inintialCount - 1);
 customerObject.Locations.First().ShouldEqual(custObjCopy);
 customerObject.Locations[3].ShouldNotEqual(thirdlocation);
 }
 private Customer GenerateMultipleLocations(int p, Customer customer)
 {
 int i = 0;
 while (i < p)
 {
 customer.AddLocation(AddressType.Business, EfficientlyLazy.IdentityGenerator.Generator.GenerateName().First, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().AddressLine, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().City, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().StateAbbreviation, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().ZipCode, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().City);
 i++;
 }
 return customer;
 }
 [Test()]
 public void ModifyLocationTest()
 {
 var customerObject = new Customer() { Name = "Hello World!!" };
 customerObject = GenerateMultipleLocations(5, customerObject);
 var inintialCount = customerObject.Locations.Count;
 var custObjCopy = customerObject.Locations.First();
 var thirdlocation = customerObject.Locations[3];
 customerObject.Locations[3].AddressId.ShouldEqual(3);
 customerObject.ModifyLocation(3, AddressType.Business, EfficientlyLazy.IdentityGenerator.Generator.GenerateName().First, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().AddressLine, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().City, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().StateAbbreviation, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().ZipCode, EfficientlyLazy.IdentityGenerator.Generator.GenerateAddress().City);
 thirdlocation.ShouldNotEqual(customerObject.Locations[3]);
 }
 [Test()]
 public void GetLocationTest()
 {
 Assert.Fail();
 }
 [Test()]
 public void GetAllLocationsTest()
 {
 Assert.Fail();
 }
 }
}

PS: I realize I haven't tested the bottom 2 methods.

Answer 1

I'm a little more concerned about the implementation class (as opposed to the testing class). It exposes List<T> and Dictionary<T,U> properties publicly, assigns them in the constructor but also allows them to be set via the property. Something smells, but I can't speak to it not knowing the full design. However, if you can, I'd recommend developing to interfaces, IList<T> and IDictionary<T,U> instead and not allowing setters on the properties. Here's what I'd code up:

namespace Vm.Models.CustomerNS
{
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using Vm.Models.Common;
 public class Customer
 {
 private readonly IList<Address> locations = new List<Address>();
 private readonly IList<User> users = new List<User>();
 private readonly IList<BuyerNum> buyerNumbers = new List<BuyerNum>();
 private readonly IList<Contact> contacts = new List<Contact>();
 private readonly IDictionary<string, string> vehicleList = new Dictionary<string, string>();
 public IList<BuyerNum> BuyerNumbers
 {
 get
 {
 return this.buyerNumbers;
 }
 }
 public IList<Contact> Contacts
 {
 get
 {
 return this.contacts;
 }
 }
 // ReSharper disable once InconsistentNaming
 public int ID
 {
 get;
 set;
 }
 public IList<Address> Locations
 {
 get
 {
 return this.locations;
 }
 }
 public string Name
 {
 get;
 set;
 }
 public IList<User> Users
 {
 get
 {
 return this.users;
 }
 }
 public IDictionary<string, string> VehicleList
 {
 get
 {
 return this.vehicleList;
 }
 }
 public void AddLocation(
 AddressType addtype,
 string addressName,
 string streetAddress,
 string city,
 string state,
 string zip,
 string country)
 {
 var lastOrDefault = this.Locations.LastOrDefault();
 var newAddress = new Address
 {
 AddressId = lastOrDefault == null ? 0 : lastOrDefault.AddressId + 1,
 AddressName = addressName,
 AddressType = addtype,
 City = city,
 State = state
 };
 if (streetAddress != null)
 {
 newAddress.StreetAddress = streetAddress;
 }
 if (zip != null)
 {
 newAddress.Zip = zip;
 }
 this.Locations.Add(newAddress);
 }
 public IList<Address> GetAllLocations()
 {
 return this.Locations;
 }
 public Address GetLocation(int addressId)
 {
 return this.Locations.First(x => x.AddressId == addressId);
 }
 public void ModifyLocation(
 int addressId,
 AddressType addtype,
 string addressName,
 string streetAddress,
 string city,
 string state,
 string zip,
 string country)
 {
 var addressid = this.Locations.Where(x => x.AddressId == addressId).ToList();
 if (addressid.Count() != 1)
 {
 throw new InvalidOperationException("Cannot Delete");
 }
 this.Locations.Remove(addressid.First());
 }
 public void RemoveLocation(int addressId)
 {
 Debug.Assert(this.Locations != null, "Locations != null");
 var addressid = this.Locations.Where(x => x.AddressId == addressId).ToList();
 if (addressid.Count() != 1)
 {
 throw new InvalidOperationException("Cannot Delete");
 }
 this.Locations.Remove(addressid.First());
 }
 }
}

Onto the unit test class. There are a number of stylistic issues I'd probably change, but I think only two real little testing bits that were off:

In method ModifyLocationTest, you get the location count, but it would seem prudent to Assert it:

GenerateMultipleLocations(5, customerObject);
var initialCount = customerObject.Locations.Count;
Assert.AreEqual(initialCount, 5);

And method GenerateMultipleLocations can be made static.

Back to the issue of the initial classes - if you can create interfaces for them, it would make dependency injection and mocking easier for unit testing. I can expound on that if you'd like, but I suggest reading up on those first.

• ChrisWue: ModifyLocation only needs the addressId - no need to pass in all the other stuff.
• user2759937: Hi Jesse, thank you very much for your answer. This was actually a refactor of a class, and I haven't gotten around to making the props private yet, I was actually planning to stick all access logic into the crud methods. I'm actually trying to read up on DI currently, my next goal is to learn Ninject
• user2759937: @Chris my logic for that was that the modified info would be passed into the method, and the method then modifies the object. Would there be an easier/better way to do this?

Answer 2

My first observations is: use injection!

for example: Instead of passing inAddressType addtype, string addressName, string streetAddress, string city, string state, string zip, string country into AddLocation, expect an Address instance. This simplifies the code greatly (a method should preferably have max 2 parameters), and in the future if you need functionality in the Address class, you will be able to pull an interface and mock the required return from the functionality.

I'm not sure where the ShouldNotBeNull, ShouldNotBeEqual, and ShouldBeEqual methods are coming from. While I understand what you are trying to do with them, I find them distracting from reading the tests. Maybe its just my brain trained to be looking for Assert for the checking part, I don't know, but on initial read through, I found it difficult to follow.

For your test when changing things, I always add an Assert on the base case.

private void TestSomething()
{
 var sut = new List<string>
 {
 "one",
 }
 Assert.That(sut.Count, Is.EqalTo(1));
 sut.Add("Two");
 Assert.That(sut.Count, Is.EqualTo(2));
}

I always name my class being tested sut (System Under Test). This allows anybody reading it to easily identify which class you are actually testing.

I would also rename your test methods. Having Test in the name is a little redundant, seems how this is a test fixture. I like having my method names reflect what is being tested and what the expected result is:

[Test]
public void CallingAddLocationWithValidDataShouldAddLocationToCustomer()
{
 // Code goes here
}

GenerateMultipleLocations does not have to return the Customer instance because it is being passed in as a ref automatically. Any changes made in the method will be reflected in the class in the original method.

As far as testing goes, this is much better than the mess I created when I first started. Keep working at it, eventually it will be very easy to see and write tests.

• user2759937: Hi Jeff, thanks for the suggestions. All the "shoulds" are coming from Should, which is an assertion library, it was used in test for a repository library, so I just decided to keep it, seems very simple :) The test boilerplate and method names are from test generator for Visual Studio, I'll look into renaming them though