4 ms·
Well thought-out article with good points and deductions! I have a few counterpoints and I think the example is not the best way to argue against unit testing a
by palotasb 6y ago
Well thought-out article with good points and deductions! I have a few counterpoints and I think the example is not the best way to argue against unit testing as there are better ways to implement the same features and write better unit tests without bloating the code.
Looking at the SolarCalculator class, I would go another way first and refactor it like so:
public class SolarCalculator
{
public static SolarTimes GetSolarTimes(Location location, DateTimeOffset date) { /* ... */ }
}
1. Made the method static (make it a free function in other languages)
2. Take an explicit Location parameter
3. Return a SolarTimes object directly, not async, not a Task<solarTimes>, and remove Async from the name
4. Drop the now unnecessary class LocationProvider member
This becomes more easily unit testable without any excess Arranges steps at the beginning.
public class SolarCalculatorTests
{
[Fact]
public Task GetSolarTimes_ForKyiv_ReturnsCorrectSolarTimes()
{
// Arrange
var location = new Location(50.45, 30.52);
var date = new DateTimeOffset(2019, 11, 04, 00, 00, 00, TimeSpan.FromHours(+2));
var expectedSolarTimes = new SolarTimes(
new TimeSpan(06, 55, 00),
new TimeSpan(16, 29, 00)
);
// Act
var solarTimes = solarCalculator.GetSolarTimes(location, date);
// Assert
solarTimes.Should().BeEquivalentTo(expectedSolarTimes);
}
}
The GetSolarTimes function is now purely computational and has no plumbing at all (stealing terms from chimprich). I think the original author would also agree that unit testing SolarTimes GetSolarTimes(Location location, DateTimeOffset date) has none of the problems that unit testing async Task<SolarTimes> GetSolarTimesAsync(DateTimeOffset date) had.
(Added benefit: the interface is more flexible, it can be reused in more use cases without modification but that is not the point.)
I find that such a refactoring often solves all of the problem entirely. It might seem like we just swapped the problem under the rug and forced the calling code to add the complexity (and the tests, interfaces, mocks etc.) that we discarded, but in practice this is often not the case.
// Uses original async implicit-location interface
// (assumes an existing solarCalculator instance)
var solarTimes = await solarCalculator.GetSolarTimesAsync(date);
// Uses proposed non-async explicit-location interface
// (assumes an existing locationProvider instance)
var solarTimes = SolarCalculator.GetSolarTimes(await locationProvider.GetLocationAsync(), date);
The reason we can often get away with this in practice is that the complexity increase in the caller is small. We did not add additional state to the caller, we did not push more testing/mocking complexity to the caller. The assumed locationProvider instance in the caller replaces the solarCalculator instance in the caller. If testing/mocking locationProvider is required, testing/mocking solarCalculator ought to have been tested too. We require the caller to test/mock something else, not something new.
If the original async Task<SolarTimes> GetSolarTimesAsync(DateTimeOffset date) interface is required nonetheless, it can be implemented as a pure "plumbing" function. As such I would agree that unit testing it would provide less value than integration testing. A simple pattern that can be applied here instead of an ILocationProvider interface and all the baggage that comes with it is using a Func<Task<Location>> or lambda instead. This allows both testing the instance with custom location providers and unhindered usage of the SolarCalculator class without always needing to inject a dependency.
public class SolarCalculator
{
private readonly Func<Task<Location>> _locationProvider;
// default constructor for normal usage
public SolarCalculator() {
internalReaLLocationProvider = LocationProvider();
_locationProvider = async () => internalReaLLocationProvider.GetLocationAsync();
}
// constructor for custom locations and testing
public SolarCalculator(Func<Task<Location>> locationProvider) {
_locationProvider = locationProvider;
}
// Gets solar times for current location and specified date
public async Task<SolarTimes> GetSolarTimesAsync(DateTimeOffset date) {
return GetSolarTimes(_locationProvider(), date);
}
public static SolarTimes GetSolarTimes(Location location, DateTimeOffset date) { /* ... */ }
}
(Sorry, I haven't implemented the IDisposable pattern for internalReaLLocationProvider and I might have misplaced an async keyword or two because C# is not my most recent language.)
To support the "pyramid-driven" paradigm I argue that the most complicated part of this feature is the solar time calculation and it would well deserve a large test suite containing many test cases like GetSolarTimes_ForKyiv_ReturnsCorrectSolarTimes above (edge cases, diverse locations, etc.). Conversely, the higher-level functions don't need this level of testing. Since they contain no complex logic, I usually assume that if they work for one input, they will work for any other. Testing the async, automatic-location version with the simple Kyiv-based input is enough, there is no need to test it with midnight sun and all the same edge cases as the base function.
The point I'm making is that unit testing is not as overrated as the original example suggests. The code can be modularized better (not by making everything an interface), with a well-unit-testable "computational" part and a part which is mostly "plumbing". I agree with those saying that the second part benefits more from integration testing than unit testing, and I can agree with keeping them "as highly integrated as possible, while keeping their speed and complexity reasonable". But I insist that unit testing the 1st part and writing it in a way that it is unit testable is important.
- dorianh_ 6y agoI totally agree with your refactoring. The code you wrote is simpler, less surprising, reusable, easily testable, and so on. Unfortunately I think that the article only shows that the OP made poor design choices, (which he probably wouldn't have if he had used TDD, ironically). Even if the article is well written, the code shown in the three first blocks kind of invalidate the whole argumentation :/