CODEWITHSUNDEEP
c#unit-testing
Redzon
Redzon

Reputation: 401

Making methods 'internal' to remove dependencies (for unit tests) - a good practice? Any better way?

I have a class as follows. 

public class MyClass  
{  
    public MyMethod()  
    {  
      int x = CalculateSomething();  
    }

   private int CalculateSomething()  
   {  
      // Do something and return an int    
      return 100;  
   }  
}

To unit test this I added [assembly: InternalsVisibleTo("MyTests")] and changed the private method to be internal virtual. In the unit test project I created a class MockMyClass and created private method as follows.

public class MockMyClass : MyClass  
{  
   public bool MadeHappyNoise {get; set;}
   internal override int CalculateSomething()  
   {  
      MadeHappyNoise = true;  
      return base.CalculateSomething();  
   }  
}

The unit test is now as follows

[TestMethod()]
public void WasCalculateSomethingCalledOK()  
{  
   MockMyClass mk = new MockMyClass();
   mk.MyMethod();  
   Assert.IsTrue(mk.MadeHappyNoise, "Oops...CalculateSomething not called...");  
}

Few questions: Is this is a good way to remove dependencies? I personally don't like to change a method from private to internal but have no option (other than to use Reflection perhaps). Also, the attribute InternalsVisibleTo("MyTests") residing in the production code is not good. Can someone point me to a better solution please? Thanks.

Upvotes: 5

Views: 622

Answers (5)

Alb
Alb

Reputation: 3681

Can you maybe refactor it to be like this:

public class MyClass  
{  
    private Calculator calculator;  

    public myMethod()  
    {  
      int x = calculateSomething();  
    }

    public void SetCalculator( Calculator c ){
        calculator = c;  
    }

   private int calculateSomething()  
   {  
        return calculator.CalculateSomething();
   }  
}

And then have calculator as a separate class and set an instance on MyClass

public Class Calculator {

   public virtual int CalculateSomething()  
   {  
      // Do something and return an int    
      return 100;  
   }  
}

You could make Calculator implement an interface and then have a different Calculator implementation or a mock that you use in your tests.

Upvotes: 1

TToni
TToni

Reputation: 9391

Hmm. I have some issues with that code, but we'll do one at a time.

  1. Why would you want to test if MyMethod calls CalculateSomething? It's an implementation detail that is probably likely to change (what if it calls CalculateSomething2 tomorrow but apart from that still does what it's supposed to do?). If you want to test the code structure of MyMethod, do a code review, not a unit test.

  2. You say that MyMethod is complex and you want to test the code flow inside. If there are multiple paths inside, you still have to write a unit test for each path, so why can't you check the result of calling MyMethod instead of checking the inside of it?

    Another thought would be to try and refactor MyMethod into methods that lend themselves to easier testing (that's almost automatic if you do test-driven-development, a practice I recommend if you want to do serious unit testing. The "test later" approach almost always leads to code that is much more difficult to test).

  3. If you still want to check the inner workings of MyMethod, maybe you can refactor the private methods you need to check this into another class (say "Calculations" in your example).

    Then you can use a mock framework (like RhinoMocks for example), to mock that class. The framework lets you define what functions you expect to be called in what order and what they should return.

    Usually you use mocks to lessen the environment requirements for unit tests, but you can use them in this way also.

Upvotes: 1

Raghu
Raghu

Reputation: 2768

If this is a piece of legacy code that you are too scared to touch, i would advise you to create a unit test which would construct MyClass. Tentatively create a public property in MyClass to expose the value of x.

In the unit test just created assert that value of x is 100 after MyClass is instantiated. Once you have that in place, refactor like @alb suggests. Run the test again, make sure x is still 100 and test the calculator class separately and eventually remove the tentative public property for x in MyClass. hope that helps.

Upvotes: 0

Gishu
Gishu

Reputation: 136633

Too much work for too little value. All that test tells me (if it passes) is that calling MyMethod calls another private method. The unit test should be testing the behavior provided by MyMethod() - what should happen/change after a call to MyMethod?.

The title of the question is a bit misleading too - there is no dependency-related issue that I can see.

You do not need InternalsVisibleTo for the most part.. simply test the private method through the public method that exercises it e.g. Write unit tests for MyMethod(). Also if you practice test-first programming, you'd already have tests that cover every line of code in the private method.

Upvotes: 1

David Neale
David Neale

Reputation: 17038

It rather depends on the methods you are changing the scope of. A unit is the smallest testable component of a piece of software - it rarely means one test per method.

I find that comprehensively testing my public methods is enough to establish correct behaviour. You might find that your tests start to constrain your code development if you wrap the private methods with tests.

If your program is more procedural you might find that you need to test at the granular level you describe, in which case using friend assemblies is a fine solution. However, I'd suggest that you would rarely need to test methods that aren't public.

Upvotes: 1

Related Questions