ChatGPT解决这个技术问题 Extra ChatGPT

Unit testing and checking private variable value

I am writing unit tests with C#, NUnit and Rhino Mocks. Here are the relevant parts of a class I am testing:

public class ClassToBeTested
{
    private IList<object> insertItems = new List<object>();

    public bool OnSave(object entity, object id)
    {
        var auditable = entity as IAuditable;
        if (auditable != null) insertItems.Add(entity);

        return false;            
    }
}

I want to test the values in insertItems after a call to OnSave:

[Test]
public void OnSave_Adds_Object_To_InsertItems_Array()
{
     Setup();

     myClassToBeTested.OnSave(auditableObject, null);

     // Check auditableObject has been added to insertItems array            
}

What is the best practice for this? I have considered adding insertItems as a Property with a public get, or injecting a List into ClassToBeTested, but not sure I should be modifying the code for purposes of testing.

I have read many posts on testing private methods and refactoring, but this is such a simple class I wondered what is the best option.

Why does OnSave always return false? =/ Is there supposed to be a return true inside the condition for "if (auditable != null)" after the .Add? It would need to be wrapped in brackets of course.
This question is nearly 9 years old so I can't remember for certain. It's possible as I was using TDD I hadn't got as far as implementing the return logic of the OnSave method when I posted this question.

M
Mark Seemann

The quick answer is that you should never, ever access non-public members from your unit tests. It totally defies the purpose of having a test suite, since it locks you into internal implementation details that you may not want to keep that way.

The longer answer relates to what to do then? In this case, it is important to understand why the implementation is as it is (this is why TDD is so powerful, because we use the tests to specify the expected behavior, but I get the feeling that you are not using TDD).

In your case, the first question that comes to mind is: "Why are the IAuditable objects added to the internal list?" or, put differently, "What is the expected externally visible outcome of this implementation?" Depending on the answer to those questions, that's what you need to test.

If you add the IAuditable objects to your internal list because you later want to write them to an audit log (just a wild guess), then invoke the method that writes the log and verify that the expected data was written.

If you add the IAuditable object to your internal list because you want to amass evidence against some kind of later Constraint, then try to test that.

If you added the code for no measurable reason, then delete it again :)

The important part is that it is very beneficial to test behavior instead of implementation. It is also a more robust and maintainable form of testing.

Don't be afraid to modify your System Under Test (SUT) to be more testable. As long as your additions make sense in your domain and follow object-oriented best practices, there are no problems - you would just be following the Open/Closed Principle.


Your 'wild guess' is spot on. I have changed my test to call both the OnSave method followed by the method that writes the log data. I then assert the data was logged accordingly paying no attention to the IList. If calling two methods from the SUT in a single test is acceptable practice then this will be my preffered solution. Thanks.
Event in Arrange-Act-Assert or Four-Phase Tests, it's perfectly legal to invoke more than one member of the SUT. One should really strive to follow the principle of only TESTING one thing at a time, but I think your approach fits that principle nicely. In essence, invoking the OnSave method is just part of your Arrange/Setup Fixture phase. The Act phase is carried out when you invoke the logging method. No violation of best practices there :)
Statements like this (one shouldn't ever be testing private interfaces) puzzle me. Let me probe a bit: Take, for example, a factory which should return lent resources to the free pool when all clients have released them. Since there's no need to expose pool state to clients, it is private. How should one verify correct behavior in situations like this? (pt.1 of 2)
I do agree with the approach you propose above. But what if measuring the state of the free pool is impractical? Wouldn't pragmatism push one toward testing internal state, with full understanding that, as with any case where behavior changes, these tests would need refactoring if someone later implemented a different scheme? In fact, they'd light up red and tell the implementer, which would be by design. I'm not disputing the wisdom of your answer in general; I'm genuinely interested in how you would recommend addressing a somewhat less common situation such as this. (pt. 2 of 2)
@bRadGibson I consider changing the interface of a SUT a perfectly normal part of the TDD process: blog.ploeh.dk/2011/11/10/TDDimprovesreusability
E
Esteban Araya

You shouldn't be checking the list where the item was added. If you do that, you'll be writing a unit test for the Add method on the list, and not a test for your code. Just check the return value of OnSave; that's really all you want to test.

If you're really concerned about the Add, mock it out of the equation.

Edit:

@TonE: After reading your comments I'd say you may want to change your current OnSave method to let you know about failures. You may choose to throw an exception if the cast fails, etc. You could then write a unit test that expects and exception, and one that doesn't.


Thanks - to add a bit more context, the OnSave is an override which must return false so I can't use the return value here. I guess what I want to do is check the Add method of the list is called, rather than checking the contents of the list. This brings me back to mocking the list and injecting it, or is there some other way...
Thanks for the suggestion but it is an expected conditon for OnSave to be called with objects that don't implement IAuditable - I'm not keen on throwing an exception in this case. In many situations I can see your suggestion would be the way forward.
C
Community

I would say the "best practice" is to test something of significance with the object that is different now that it stored the entity in the list.

In other words, what behavior is different about the class now that it stored it, and test for that behavior. The storage is an implementation detail.

That being said, it isn't always possible to do that.

You can use reflection if you must.


The only way the class will behave differently is in when a second method is called. This checks the list for items. Check this behviour to see if the item was added to the list would entail testing two methods in the same test.
I don't see anything wrong with having to call two methods to make a test, but I'm more of a TDD/BDD guy than a test coverage guy. I think the alternative couples your test too much to the implementation details of your class, making your tests a burden, not an aide. But reasonable people disagree on this point.
J
Jeff Sternal

If I'm not mistaken, what you really want to test is that it only adds items to the list when they can be cast to IAuditable. So, you might write a few tests with method names like:

NotIAuditableIsNotSaved

IAuditableInstanceIsSaved

IAuditableSubclassInstanceIsSaved

... and so forth.

The problem is that, as you note, given the code in your question, you can only do this by indirection - only by checking the private insertItems IList<object> member (by reflection or by adding a property for the sole purpose of testing) or injecting the list into the class:

public class ClassToBeTested
{
    private IList _InsertItems = null;

    public ClassToBeTested(IList insertItems) {
      _InsertItems = insertItems;
    }
}

Then, it's simple to test:

[Test]
public void OnSave_Adds_Object_To_InsertItems_Array()
{
     Setup();

     List<object> testList = new List<object>();
     myClassToBeTested     = new MyClassToBeTested(testList);

     // ... create audiableObject here, etc.
     myClassToBeTested.OnSave(auditableObject, null);

     // Check auditableObject has been added to testList
}

Injection is the most forward looking and unobtrusive solution unless you have some reason to think the list would be a valuable part of your public interface (in which case adding a property might be superior - and of course property injection is perfectly legit too). You could even retain a no-argument constructor that provides a default implementation (new List()).

It is indeed a good practice; It might strike you as a bit overengineered, given that it's a simple class, but the testability alone is worth it. Then on top of that, if you find another place you want to use the class, that will be icing on the cake, since you won't be limited to using an IList (not that it would take much effort to make the change later).


I have those three test methods in my actual code and as you say the problem is checking that the item is actually added to the list when the return value isn't available for use. Injecting a list solves my problem but the injection will only be used for testing - in production the list will be completely internal to the class as it is a detail of the implementation. Is it acceptable practice to add a constructor injecting a list purely for testing? Thanks!
Gah, sorry to be so pedantic in my answer - I didn't notice on the first read of your question that you already had an excellent grasp of the issues. I'm editing it to be a little less pompous and address your core question better.
Injection does solve my problem in a clean manner. However I have opted to avoid any code changes by testing the behaviour of other methods which results from the OnSave call. The IList is still an implementation detail in my mind so I have left it as such. Again in many scenarios I would certainly use the injection option.
k
kyoryu

If the list is an internal implementation detail (and it seems to be), then you shouldn't test it.

A good question is, what is the behavior that would be expected if the item was added to the list? This may require another method to trigger it.

    public void TestMyClass()
    {
        MyClass c = new MyClass();
        MyOtherClass other = new MyOtherClass();
        c.Save(other);

        var result = c.Retrieve();
        Assert.IsTrue(result.Contains(other));
    }

In this case, i'm asserting that the correct, externally visible behavior, is that after saving the object, it will be included in the retrieved collection.

If the result is that, in the future, the passed-in object should have a call made to it in certain circumstances, then you might have something like this (please forgive pseudo-API):

    public void TestMyClass()
    {
        MyClass c = new MyClass();
        IThing other = GetMock();
        c.Save(other);

        c.DoSomething();
        other.AssertWasCalled(o => o.SomeMethod());
    }

In both cases, you're testing the externally visible behavior of the class, not the internal implementation.


C
Chris Golledge

The number of tests you need is dependent on the complexity of the code - how many decision points are there, roughly. Different algorithms can achieve the same result with different complexity in their implementation. How do you write a test that is independent of the implementation and still be sure you have adequate coverage of your decision points?

Now, if you are designing larger tests, at say the integration level, then, no, you would not want to write to implementation or test private methods, but the question was directed to the small, unit test scope.


关注公众号,不定期副业成功案例分享
Follow WeChat

Success story sharing

Want to stay one step ahead of the latest teleworks?

Subscribe Now