Type that returns a result based on some logic [closed]

I have a type that returns a result based on some logic. The result contains a message. Should a good unit test check that the message is correct?

A negligible amount has changed as I’m not allowed to post IP:

class MyFilter
{
    private readonly int _state;

    public Result FilterResult { get; private set; }

    public MyFilter(int value)
    {
        _state = value;
    }

    public void Initialise()
    {
        string message;

        if (_state == 0)
        {
            message = "filtered because zero";
            FilterResult = new Result {ShouldFilter = true, Reason = message};
            log(message);
        }
        else if(_state == 1)
        {
            message = "filtered because one";
            FilterResult = new Result {ShouldFilter = true, Reason = message};
            log(message);
        }
        else
        {
            message = "not filtered because...";
            FilterResult = new Result {ShouldFilter = false, Reason = message};
            log(message);
        }
    }

    public bool ShouldFilter()
    {
        return FilterResult.ShouldFilter;
    }

    private void log(string message) { }
}

I want to test this type. I want to test that it correctly sets the ShouldFilter flag on the result. But I also want to test why it thinks it should filter or not, so I’m checking the string on the result too:

    [Test]
    public void When_zero()
    {
        MyFilter sut = new MyFilter(0);
        sut.Initialise();
        Assert.AreEqual(true, sut.FilterResult.ShouldFilter);
        Assert.AreEqual("filtered because zero", sut.FilterResult.Reason);
    }

I had some feedback on an internal code review saying that testing strings was a code smell; if someone changes the string, the test will fail even though the behaviour may still be the same.

I personally think that, in this case, it’s OK to test the string: the fact that the type thinks something is filtered is just part of the answer; I want to know why it thinks it should be filtered.

That’s the crux of the question. It’s not my ideal style of code, so, to give a little context, I’ll explain what the code did before I inherited it:

The filter just used to log the reason why it was filtering. FilterResult didn’t exist. The code was just this:

class MyFilter
{
    private readonly int _state;
    private bool _filtered;

    public MyFilter(int value)
    {
        _state = value;
    }

    public void Initialise()
    {
        if (_state == 0)
        {
            _filtered = true;
            log("filtered because zero");
        }
        else if(_state == 1)
        {
            _filtered = true;
            log("filtered because one");
        }
        else
        {
            _filtered = false;
            log("not filtered because...");
        }
    }

    public bool ShouldFilter() => _filtered;

    private void log(string message) { }
}

This type was hard to test. The logic could change for the worse and it would be likely that the tests would still pass as they were just checking the binary ShouldFilter().

What I would’ve liked to have done, and what I normally do for important ‘messages’, is inject an INotifySupport interface as a dependency (have used this for years since reading this fine book), and rather than just logging these messages, I would call _notifySupport.OfSomethingFilterRelated(reason). I could then write a test named liked SupportWereNotified.

But I couldn’t do that this time, so I exposed the Result property.

Anyway, bit of a convoluted post, so I’d better reiterate the question:

Should a good unit test check that the message is correct?

The problem is the Initialise method.

‘Initialise‘, ‘Init‘, ‘PostConstruct‘, … are always a code smell. All these methods perform actions/checks on an object after it has been created.
But most of the time, these actions/checks need to happen before, or while an objects is created.

Refactor the code by using a factory-method or builder where you put the ‘initialise’ code in the method/class, instead of in the object itself.

Then you can write an easy test on this object creation or validation method/class.

I would rewrite to something like this:

enum Reason{
    ZERO,ONE,OK
}


class MyFilterCreator
{
    public MyFilter create(int value)
    {
        if(shouldFilter(value) == Reason.OK){
            return new MyFilter(value);
        }else{
            return null;
        }
    }

    // write a test for this method
    public Reason shouldFilter(int _state)
    {
        if (_state == 0)
        {
            return Reason.ZERO;
        }
        else if(_state == 1)
        {
            return Reason.ONE;
        }
        else
        {
            return Reason.OK;
        }
    }
}

But to answer your question: No, do not check on messages, but on statuses.