How to test a public method that changes a variable with defined getter

Question

I have a game that has coins. There is an Add() method that changes the coins. I want to test if it adds correctly.

public class CoinsService 
{
    public ReactiveProperty<long> Coins { get { return State.SaveGame.Coins; } }

    public int Add(int coins)
    {
         Coins.Value += coins;
         return coins;
    }
}

The test:

public class CoinServiceTests
{
     [Test]
     public void AddCoins_WhenCalled_AddsUpToTotalCoins()
     {
          var coinsService = new CoinsService();
          coinsService.Add(10);
          Assert.That(coinsService.Coins.Value, Is.EqualTo(10));
     }
}

I have tried making a substitute of the class using NSubstitute like that:

var coinsService = Substitute.For<CoinsService>();

and instantiating a fresh instance of the Coins like that

coinsService.Coins.Returns(new ReactiveProperty<long>());

also like that

var coins = new ReactiveProperty<long>();
coinsService.Coins.Returns(coins);

I expect that that after doing any of the above, I will be able to check the Coins value. Instead I'm getting a null reference object exception that the coinsService.Coins object is null

To clarify, the null reference appears on the line

public ReactiveProperty<long> Coins { get { return State.SaveGame.Coins; } }

Your test of `CoinsService` seems legit, but I don't get what do you want to mock for? There should be another test code using mocked `CoinsService` which is not posted. Where is the code getting null reference? — Louis Go, Mar 25 '20 at 15:29
@LouisGo I may mock, because the coins are actually stored on a server. I'm getting the null reference on this line: `public ReactiveProperty Coins { get { return State.SaveGame.Coins; } }` — Lee Jaedong, Mar 25 '20 at 15:40
@LeeJaedong You appear to be tightly coupling your service to static implementation concerns. Which make it difficult to isolate the subject under test. — Nkosi, Mar 25 '20 at 15:43
@Nkosi this shouldn't be an issue in this particular case. I just want to mock/substitute the Coins object with a new one. Whenever the Add() method is called, it should just change the newly created clone/test double. Instead it throws an exception. — Lee Jaedong, Mar 25 '20 at 16:06
@Nkosi it's an object that persists data locally and on a server. But that's not an issue. If I declare a new variable that just has a private set, and try to modify it, the error is still there (but on the new var with the private set) — Lee Jaedong, Mar 25 '20 at 16:14
@LeeJaedong `State` **is** the issue as it appears to be static dependency, access to that dependency should be abstracted out of the service so that it can be replaced when testing in isolation. Based on what you have currently explained this appears to be an [XY problem](https://meta.stackexchange.com/questions/66377/what-is-the-xy-problem). — Nkosi, Mar 25 '20 at 16:28
@LeeJaedong Don't *mock* any object shall be tested. Mocked instance is used for "I tested it but I need a fake one for another test". — Louis Go, Mar 26 '20 at 01:00

score 3 · Accepted Answer · answered Mar 25 '20 at 17:10

You are trying to test CoinsService because it does the addition. Therefore, you have to use a real CoinsService rather than mocking it. Mocking is for classes that collaborate with the class you are trying to test.

Looking at your code, I see why you thought this should work... you have this line of code...

coinsService.Coins.Returns(new ReactiveProperty<long>());

This causes a new Coins property to be created each time it is accessed, which is why you have the error you are seeing.

I suspect that the root cause of this is that CoinsService is a large class, with lots of functionality, which you don't want to instantiate just to test the ability to add coints. That leads to wanting to mock it. (If that's not true, we can stop here - just don't mock it!)

If CoinsService is "too big to test", then it needs to be broken down in such a way that it uses collaborators. For example, imagine that Coins were a class than just a long. It could have an Add method... Add(long howmuch) for example.

Then CoinsService would be changed just a bit to do...

public int Add(int coins)
{
     Coins.Add(coins);
     return Coins.Value; // I believe your original return is in error
}

Now this makes it all a bit more indirect, but gives you the advantage that you can test the addition function by testing the Coins class, without using a service.

You can (and should) also test the service itself by creating a mock for Coins and ensuring that its Add method is called when CoinService.Add is called.

I haven't had the chance to test it yet, but it looks like a good option to me so I'm accepting this as an answer. Thank you. — Lee Jaedong, Mar 26 '20 at 09:27

score 1 · Answer 2 · answered Mar 26 '20 at 01:28

Don't mock any object shall be tested. Mocked instance is used for something "I tested it, but I need a fake one for another test"

In your case, what State.SaveGame.Coins shall be mocked but not CoinsService, because CoinsService is going to be tested.

To make CoinsService testable, most common way is refactor CoinsService to accept Dependency Injection container and get rid of static State.

// An interface to provide Coins
    public interface ICoinProvider
    {
        ReactiveProperty<long> Coins{ get; }
    }

    public class CoinsService{

        // Ctor take the instance in it
        public CoinsService( ICoinProvider provider )
        {
            _CoinProvider = provider;
        }

        private ICoinProvider _CoinProvider;

        public CoinWrap Coins
        {
            get
            {
                return _CoinProvider.Coins;
            }
        }

        public int Add( int coins )
        {
            Coins.Value += coins;
            return coins;
        }
    }

Then mock ICoinProvider

     [Test]
     public void AddCoins_WhenCalled_AddsUpToTotalCoins()
     {
          var mock = Substitute.For<ICoinProvider>();
          mock.Coins.Returns( new ReactiveProperty<long>( 10 ) );
          var coinsService = new CoinsService( mock );
          coinsService.Add(10);
          Assert.That(coinsService.Coins.Value, Is.EqualTo(10));
     }

If you found this refactor is hard to be done, then you may read Charlie's post.

Thanks for the reply, it does make sense. However adding that layer of abstraction on top of what I have would be pretty time-consuming. — Lee Jaedong, Mar 26 '20 at 09:27
@Louis FWIW I believe the DI approach may end up being needed here at some point but the OP didn't give us enough info to say that for sure. I'm generally in favor of introducing change incrementally anyway! — Charlie, Mar 26 '20 at 17:56

How to test a public method that changes a variable with defined getter

2 Answers2