Is it wrong to use reflection to assert complex objects for a unit test?
I was reading this section which is about using reflection to test a private variable ...
But I have no such problem in my unit test and my code is fully tested.
The only problem I have figured out is very time consuming doing an assertion for each property of a complex object with the expected result; especially for a list of complex objects.
Since this is a complex object, doing normal Assert.AreEqual
will not give me the correct result unless I implement IEquality
for each of the objects.
But even if I do, it doesn't tell me what is the property / field name, expected value and actual value at the time of assertion.
That's right, we manually put each of the property values into a list and make one CollectionAssertion
, but it still takes a long time, and when this statement happens, it only tells me that the index of the item value is not equal; it won't tell me the name of the property. This is very difficult to debug (I had to go into debug mode and look at the item in the collection).
So, I wonder if I write a recursive reflection method that will perform an assertion on two complex objects, which will tell me each property name, expected value, actual value.
Is this good practice or bad practice?
source to share
IMHO, this is bad practice because:
- Reflection code is slow and difficult to write on the right
- This is even harder to maintain, and code like this might not be refactoring friendly.
- Reflection is slow, unit tests should be fast
- It also doesn't feel right
It seems to me that you are trying to connect the hole and not fix the problem. To fix this problem, I can suggest separating a large and complex class into a set of smaller ones. If you have many properties, group them into separate classes
So a class like this
class Foo
{
T1 Prop1 {get; set;}
T2 Prop2 {get; set;}
T3 Prop3 {get; set;}
T4 Prop4 {get; set;}
}
Would become
class Foo
{
T12 Prop12 {get; set;}
T34 Prop34 {get; set;}
}
class T12
{
T1 Prop1 {get; set;}
T2 Prop2 {get; set;}
}
class T34
{
T3 Prop3 {get; set;}
T4 Prop4 {get; set;}
}
Note that it Foo
now only has one property (that is, the "grouped" view). If you can group properties so that any change in state is localized to a specific group, your task becomes much more simplified. Then you can assert that the "grouped" property is equal to the expected state.
source to share
I find that a lot of people won't even think about reflection, but it has its place. It definitely has drawbacks regarding performance, type safety, etc., as other posters have stated, but I actually think unit tests are a great place to use it. Until this is done it is instructive.
Trying to enforce equality implementations for all objects running on walls when you don't own all the types you use in your properties. And implementing hundreds of mini-mapping classes takes a lot of time like writing out assertions manually.
In the past, I've written an extension method that does what you describe:
- Compares two objects of the same type (or that implement a common interface)
- Reflection is used to find all public properties.
- If property is a value type, forward Assert.AreEquals
- For reference types, it makes a recursive call
By no means do my tests care about property names, so refactoring for renames doesn't matter. In fact, new properties are found automatically and deleted ones are forgotten.
I've never used it with really complex objects, but it worked great with the ones I have without slowing down my tests.
So, in my opinion, in a unit test, feel free to use Reflection.
Edit: I'll try to dig my method up for you.
source to share
In my opinion, using Reflection is not a good option. Using Reflection means that we lose type safety at compile time. And also, behind the use of Reflection, there is a (possibly) string insensitive search for strings through the Assembly metadata. This results in poor performance. With these aspects in mind, I believe that splitting the original type (as recommended by oleksii) is one good approach.
Another approach might be to write separate tests, validate a separate set of attributes using pure accessors. This may not be acceptable in all cases. But this happens in some cases.
For example: if I have a Customer class, I can write one test to check the Address-type fields; I could write another test to check order type fields, etc.
source to share
Under normal circumstances, you don't need to speculate to unit test anything. This is stated in the answer to the question you asked:
Reflection should really only be a last resort
If you need to test if complex objects are equal, implement such an equality test in a unit test . There's nothing wrong with additional code being used exclusively for unit testing:
public void ComplexObjectsAreEqual()
{
var first = // ...
var second = // ...
AssertComplexObjectsAreEqual(first, second);
}
private void AssertComplexObjectsAreEqual(ComplexObject first,
ComplexObject second)
{
Assert.That(first.Property1, Is.EqualTo(second.Property1),
"Property1 differs: {0} vs {1}", first.Property1, second.Property1);
// ...
}
You don't have to treat unit tests like other code. If you need to write something to make them more readable, cleaner, customizable - write. This is the same code as elsewhere. Are you comparing objects via reflection in your production code?
source to share
I would argue that there are many good reasons to use reflection for simple unit testing. To quote https://github.com/kbilsted/StatePrinter
problems with manual unit testing
It is time consuming.
As I type and retype over and over again: Assert.This, Assert.That, ... can't help but wonder why the computer can't automate this stuff for me. Anything that unnecessary typing takes time and drains my energy.
With Stateprinter, statements are generated for you when there is a mismatch between expected and actual values.
Code and test don't sync
When the code changes, say by adding a field to the class, you need to add assertions to some of your tests. Finding where, however, is a completely manual process. In a larger project where no one has a complete view of all classes, the necessary changes are not made in all places where it should be.
A similar situation occurs when merging code from one branch to another. Suppose you merged a bug fix or feature from the release branch into the development branch, which I observe over and over again is that the code is merged, all tests are executed, and then the merge is done. People forget to revisit and double check the entire test suite to see if tests exist that exist on the development branch and not on the merged branch, adjust them accordingly.
When using Stateprinter, graphs of objects are compared rather than individual fields. Thus, when a new field is created, all of the corresponding tests fail. You can customize printing to specific margins, but you will lose the ability to automatically detect changes in graphics.
Poor readability I
You've come a long way with good naming of test classes, test methods, and standard naming of test items. However, no naming conventions can compensate for the visual clutter claims. Further confusion is added when indices are used to extract items from lists or dictionaries. And don't force me to start by combining this with for, foreach loops, or LINQ expressions.
When using StatePrinter, graphs of objects are compared rather than individual fields. Thus, there is no need for test logic to collect data.
Poor readability II
When I read tests like below. Think about what's really important here.
Assert.IsNotNull(result, "result");
Assert.IsNotNull(result.VersionData, "Version data");
CollectionAssert.IsNotEmpty(result.VersionData)
var adjustmentAccountsInfoData = result.VersionData[0].AdjustmentAccountsInfo;
Assert.IsFalse(adjustmentAccountsInfoData.IsContractAssociatedWithAScheme);
Assert.AreEqual(RiskGroupStatus.High, adjustmentAccountsInfoData.Status);
Assert.That(adjustmentAccountsInfoData.RiskGroupModel, Is.EqualTo(RiskGroupModel.Flexible));
Assert.AreEqual("b", adjustmentAccountsInfoData.PriceModel);
Assert.IsTrue(adjustmentAccountsInfoData.IsManual);
when distilled is really what we try to express,
adjustmentAccountsInfoData.IsContractAssociatedWithAScheme = false
adjustmentAccountsInfoData.Status = RiskGroupStatus.High
adjustmentAccountsInfoData.RiskGroupModel = RiskGroupModel.Flexible
adjustmentAccountsInfoData.PriceModel = "b"
adjustmentAccountsInfoData.IsManual = true
Poor persuasion
When business objects grow in number of fields, the opposite is true for test persuasion. Are all fields covered? Are the fields incorrectly mapped multiple times? Or against the wrong fields? You know the pain when you have to make 25 statements on an object and make sure that the correct fields are checked against the correct fields. And then the reviewer must go through the same exercise. Why isn't it automated?
When using StatePrinter, graphs of objects are compared rather than individual fields. You know that all fields are covered since all fields are printed.
source to share