tags:

views:

237

answers:

6

Given the following SUT, would you consider this unit test to be unnecessary?

**edit : we cannot assume the names will match, so reflection wouldn't work.

**edit 2 : in actuality, this class would implement an IMapper interface and there would be full blown behavioral (mock) testing at the business logic layer of the application. this test just happens to be the lowest level of testing that must be state based. I question whether this test is truly necessary because the test code is almost identical to the source code itself, and based off of actual experience I don't see how this unit test makes maintenance of the application any easier.

//SUT
public class Mapper
{
  public void Map(DataContract from, DataObject to)
  {
    to.Value1 = from.Value1;
    to.Value2 = from.Value2;
    ....
    to.Value100 = from.Value100;
  }
}

//Unit Test
public class MapperTest()
{
   DataContract contract = new DataContract(){... } ;
   DataObject do = new DataObject(){...};
   Mapper mapper = new Mapper();
   mapper.Map(contract, do);
   Assert.AreEqual(do.Value1, contract.Value1);
   ...
   Assert.AreEqual(do.Value100, contract.Value100);

}
+3  A: 

i would question the construct itself, not the need to test it

[reflection would be far less code]

Steven A. Lowe
Writing the test I may have gotten to about the seventh property before I would have thought, "this would be easier with reflection." Writing the test with reflection would have led me to the obvious conclusion that writing the code using reflection would be easier.
tvanfosson
I would use code emission rather than reflection to do this if all property names matched. It'd be a bit speedier :p
Otter
@[Otter]: don't let the reflection comment distract from the main point - this class is of questionable value, regardless of the unit test. What is the point of assigning a hundred fields from one class to another? why not just put them into a dictionary and copy that? or just make another reference
Steven A. Lowe
Steven, I appreciate your responses but I was hoping the discussion would focus on unit testing philosophy rather than justifying why this class is needed. But to answer your question the contract class is exposed to users of our service and we map them to data objects representing our actual db.
Otter
What I mean is that in reality our mapper classes are not literally mapping all fields 1 to 1 and the to and from classes also have different fields and methods from each other.
Otter
@Otter: in that case then yes, it is necessary to test it. Personally, I would not unit test it, I would test it indirectly as a consequence of testing a feature that involved its use (My interpretation of TDD is only test features, not unit test everything).
Steven A. Lowe
A: 

Not if this was the only unit test of this kind in the entire app. However, the second another like it showed up, you'd see me scrunch my eyebrows and start thinking about reflection.

Morendil
+2  A: 

It would be better if you could test at a higher level, i.e. the business logic that requires you to create the Mapper.Map() function.

RickL
Doesn't that multiply the possible reasons for test failure, though? I was under the impression that, ideally, all levels are tested, so that the failure can be spotted in the correct location as soon as a regression occurs.
Devin Jeanpierre
IMHO, the problem with testing at too low a level is that it makes the tests too rigid/brittle. If the low-level implementation is changed, but the business requirement is still met then a low level test such as this could start failing, whereas the higher level test wouldn't.
RickL
+2  A: 

I'd argue that it is necessary. However, it would be better as 100 separate unit tests, each that check one value. That way, when you something go wrong with value65, you can run the tests, and immediately find that value65 and value66 are being transposed. Really, it's this kind of simple code where you switch your brain off and forget about that errors happen. Having tests in place means you pick them up and not your customers.

However, if you have a class with 100 properties all named ValueXXX, then you might be better using an Array or a List.

David Kemp
Of course, these 100 tests would be in their own test fixture, with the mapping happening in the SetUp method.
David Kemp
Wow, you'd really create 100 separate unit tests to test that function? Seems a bit OTT to me.
RickL
@Rick - I'd soon have tired of writing 100 unit tests, and found a better way to solve the problem.
David Kemp
A: 

Not Excesive.

I agree the code looks strange but that said:

The beauty of unit test is that once is done is there forever, so if anyone for any reason decides to change that implementation for something more "clever" still the test should pass, so not a big deal.

I personally would probably have a perl script to generate the code as I would get bored of replacing the numbers for each assert, and I would probably make some mistakes on the way, and the perl script (or what ever script) would be faster for me.

webclimber
What if in most cases all the values will be basic types? I think it's reasonable to assume that no one will come up with something better than using the equals operator on basic types, so now the only thing to worry about is adding/removing value fields.
Otter
+1  A: 

It is not excessive. I'm sure not sure it fully focuses on what you want to test.

"Under the strict definition, for QA purposes, the failure of a UnitTest implicates only one unit. You know exactly where to search to find the bug."

The power of a unit test is in having a known correct resultant state, the focus should be the values assigned to DataContract. Those are the bounds we want to push. To ensure that all possible values for DataContract can be successfully copied into DataObject. DataContract must be populated with edge case values.

PS. David Kemp is right 100 well designed tests would be the most true to the concept of unit testing.

Note : For this test we must assume that DataContract populates perfectly when built (that requires separate tests).

Paxic