Reputation: 6307
I have a fairly simple class that I'm trying to unit test. I'm very new to unit testing in general, and I'm not sure what I should be testing here.
The only test case that I can figure out how to code is a null argument of stream
. Besides that, I'm not sure how to test the results of a PutObjectRequest
or what else. If I should be using mocks here, how?
public class AmazonS3Service : IAmazonS3Service
{
private readonly Uri baseImageUrl;
private readonly Uri s3BaseUrl;
private readonly string imageBucket;
public AmazonS3Service()
{
imageBucket = ConfigurationManager.AppSettings["S3.Buckets.Images"];
s3BaseUrl = new Uri(ConfigurationManager.AppSettings["S3.BaseAddress"]);
baseImageUrl = new Uri(s3BaseUrl, imageBucket);
}
public Image UploadImage(Stream stream)
{
if (stream == null) throw new ArgumentNullException("stream");
var key = string.Format("{0}.jpg", Guid.NewGuid());
var request = new PutObjectRequest
{
CannedACL = S3CannedACL.PublicRead,
Timeout = -1,
ReadWriteTimeout = 600000, // 10 minutes * 60 seconds * 1000 milliseconds
InputStream = stream,
BucketName = imageBucket,
Key = key
};
using (var client = new AmazonS3Client())
{
using (client.PutObject(request))
{
}
}
return new Image
{
UriString = Path.Combine(baseImageUrl.AbsoluteUri, key)
};
}
}
Upvotes: 1
Views: 6334
Reputation: 40818
You are having trouble unit testing UploadImage
because it is coupled to many other external services and state. Static calls including (new
) tightly couple the code to specific implementations. Your goal should be to refactor those so that you can more easily unit test. Also, keep in mind that after unit testing this class, you will still need to do the big tests involving actually using the Amazon S3 service and making sure the upload happened correctly without error or fails as expected. By unit testing thoroughly, hopefully you reduce the number of these big and possibly expensive tests.
Removing the coupling to the AmazonS3Client
implementation is probably going to give you the biggest bang for your testing buck. We need to refactor by pulling out the new AmazonS3Client
call. If there is not already an interface for this class, then I would create one to wrap it. Then you need to decide how to inject the implementation. There are a number of options, including as a method parameter, constructor parameter, property, or a factory.
Let's use the factory approach because it is more interesting than the others, which are straight-forward. I've left out some of the details for clarity and read-ability.
interface IClientFactory
{
IAmazonS3Client CreateAmazonClient();
}
interface IAmazonS3Client
{
PutObjectResponse PutObject(PutObjectRequest request); // I'm guessing here for the signature.
}
public class AmazonS3Service : IAmazonS3Service
{
// snip
private IClientFactory factory;
public AmazonS3Service(IClientFactory factory)
{
// snip
this.factory = factory;
}
public Image UploadImage(Stream stream)
{
if (stream == null) throw new ArgumentNullException("stream");
var key = string.Format("{0}.jpg", Guid.NewGuid());
var request = new PutObjectRequest
{
CannedACL = S3CannedACL.PublicRead,
Timeout = -1,
ReadWriteTimeout = 600000, // 10 minutes * 60 seconds * 1000 milliseconds
InputStream = stream,
BucketName = imageBucket,
Key = key
};
// call the factory to provide us with a client.
using (var client = factory.CreateAmazonClient())
{
using (client.PutObject(request))
{
}
}
return new Image
{
UriString = Path.Combine(baseImageUrl.AbsoluteUri, key)
};
}
}
A unit test might look like this in MSTest:
[TestMethod]
public void InputStreamSetOnPutObjectRequest()
{
var factory = new TestFactory();
var service = new AmazonS3Service(factory);
using (var stream = new MemoryStream())
{
service.UploadImage(stream);
Assert.AreEqual(stream, factory.TestClient.Request.InputStream);
}
}
class TestFactory : IClientFactory
{
public TestClient TestClient = new TestClient();
public IAmazonS3Client CreateClient()
{
return TestClient;
}
}
class TestClient : IAmazonS3Client
{
public PutObjectRequest Request;
public PutObjectResponse Response;
public PutObjectResponse PutObject(PutObjectRequest request)
{
Request = request;
return Response;
}
}
Now, we have one test verifying that the correct input stream is sent over in the request object. Obviously, a mocking framework would help cut down on a lot of boilerplate code for testing this behavior. You could expand this by starting to write tests for the other properties on the request object. Error cases are where unit testing can really shine because often they can be difficult or impossible to induce in production implementation classes.
To fully unit test other scenarios of this method/class, there are other external dependencies here that would need to be passed in or mocked. The ConfigurationManager
directly accesses the config file. Those settings should be passed in. Guid.NewGuid
is basically a source of uncontrolled randomness which is also bad for unit testing. You could define an IKeySource
to be a provider of key values to various services and mock it or just have the key passed from the outside.
Finally, you should be weighing all the time taken for testing/refactoring against how much value it is giving you. More layers can always be added to isolate more and more components, but there are diminishing returns for each added layer.
Upvotes: 2
Reputation: 38077
Things I would look at:
Mock your configuration manager to return invalid data for the bucket and the URL. (null, invalid urls, invalid buckets)
Does S3 support https ? If so mock it, if not, mock it and verify you get a valid error.
Pass different kinds of streams in (Memory, File, other types).
Pass in streams in different states (Empty streams, streams that have been read to the end, ...)
I would allow the timeouts to be set as parameters, so you can test with really low timeouts and see what errors you get back.
I would also test with duplicate keys, just to verify the error message. Even though you are using guids, you are storing to an amazon server where someone else could use the S3 API to store documents and could theoretically create a file that appears to be a guid, but could create a conflict down the road (unlikely, but possible)
Upvotes: 1