-3

Summary

Is there harm in exposing concrete class data as get-only public properties solely for the purposes of writing better unit tests?


Background

In general, I prefer to keep as much, if not all, of my data as private readonly. This ensures consumers aren't bound to the data held but rather to the behaviors a type provides. One (potential) issue I've noticed recently is writing good unit tests for factory types that implement an interface. Consider the following illustrative example:

public interface IComponent
{
    void DoStuff();
}
public interface IComponentDependency
{
    void HelpInDoingStuff(int data);
}
public interface IFactory
{
    IComponent CreateComponent(IComponentDependency dependency)
}
public class SpecialComponent : IComponent
{
    private readonly int someOtherDependency;
    private readonly IComponentDependency dependency;
    public SpecialComponent(IComponentDependency dependency, int someOtherDependency)
    {
        this.dependency = dependency;
        this.someOtherDependency = someOtherDependency;
    }
    public void DoStuff()
    {
        dependency.HelpInDoingStuff(someOtherDependency);
    }
}
public class SpecialComponentFactory : IFactory
{
    private readonly int someOtherDependency;
    public SpecialComponentFactory(int someOtherDependency)
    {
        this.someOtherDependency = someOtherDependency;
    }

    public  IComponent CreateComponent(IComponentDependency dependency)
    {
        return new SpecialComponent(dependency, this.someOtherDependency);
    }
}

When writing tests for SpecialComponentFactory the only thing available publicly to test is that SpecialComponentFactory returns an instance of SpecialComponent. What if I'd also like to confirm that SpecialComponent was indeed constructed with someOtherDependency, or in fact, that he was constructed with the specified dependency. To do this without reflection hacks we could make them protected and subclass SpecialComponent and then expose it publicly via a test fake:

public class SpecialComponent
{
    protected readonly int someOtherDependency;
    protected readonly IComponentDependency dependency;
    public SpecialComponent(IComponentDependency dependency, int someOtherDependency)
    {
        this.dependency = dependency;
        this.someOtherDependency = someOtherDependency;
    }
    public void DoStuff()
    {
        dependency.HelpInDoingStuff(someOtherDependency);
    }
}
public class FakeSpecialComponent : SpecialComponent
{
    public FakeSpecialComponent(IComponentDependency dependency, int someOtherDependency)
        : base(dependency, someOtherDependency)
    {
        this.dependency = dependency;
        this.someOtherDependency = someOtherDependency;
    }
}

But this doesn't seem ideal to me. Another option is to simply expose the dependencies as public. While this would permit the test to assert that the dependencies used are correct, it also goes against my long held belief in keeping data as private as possible and only exposing the behavior.

public class SpecialComponent
{
    public int SomeOtherDependency;
    public IComponentDependency Sependency;
    public SpecialComponent(IComponentDependency dependency, int someOtherDependency)
    {
        Sependency = dependency;
        SomeOtherDependency = someOtherDependency;
    }
    public void DoStuff()
    {
        Dependency.HelpInDoingStuff(SomeOtherDependency);
    }
}

Concluding Thoughts

If the entire solution never actually references SpecialComponent directly, but instead always references IComponent, then is there any harm (or smelliness) in the second option of exposing the dependencies as public get-only properties. But now I'm butting up against another long held belief in modifying the accessibility of members for the sole purpose of unit testing.


Question

Is there any harm in actually making the private readonly fields into public get-only properties on my concrete types? I'm trying to rationalize it as being OK since consumers of SpecialComponent only reference IComponent, so they won't have direct access to the data, but the tests could simply cast to SpecialComponent and perform the desired assert statements.

Am I over thinking all of this? :)

EDIT: I've updated the above code samples to include method declarations and implementations for IComponent to 'fill out' the example. As noted in the below comment, I could unit test that the IComponent instance returned from SpecialComponentFactory.CreateComponent behaves a certain way which would provide coverage in proving that the required dependencies were used, but this to me sounds like the least acceptable option since the unit tests for SpecialComponentFactory would unnecessarily be testing SpecialComponent's implementation, thereby giving it reasons to fail outside of it's own implementation and coupling the SpecialComponentFactory tests to the behavior of SpecialComponent

mhand
  • 1,231
  • 1
  • 11
  • 21
  • 1
    a) This will almost certainly be closed as opinion based. b) My opinion is to make it `public` (with basically the same reasoning as your text under **Question**). – mjwills Jan 04 '19 at 05:12
  • @mjwills -- thanks for the comment -- and yes, I half anticipated it to be closed, but was hopeful that there was some reference in a book that supports my claims. Thank you for your opinion. – mhand Jan 04 '19 at 05:30
  • The real problem here is you are storing values that aren't used, so you don't have a valid example. The point of a unit test is to validate a single method **logic**. Validating that [a variable is set a value has no value](https://stackoverflow.com/questions/6197370/should-unit-tests-be-written-for-getter-and-setters). – Erik Philips Jan 04 '19 at 05:33
  • @ErikPhilips - Storing values that aren't used? I think I was pretty clear that the example is purely illustrative. If you noticed, the interface definitions are empty and in the real code the dependencies are actually used, so I think your down vote is fairly unsubstantiated. If you prefer, I can add methods that use these values, but then again, I'm writing a unit test for the factory and I don't think that a unit test for the factory should test the behavior of the created class `SpecialComponent`, which would be required in order to confirm that certain dependencies were in fact used. – mhand Jan 04 '19 at 06:07
  • Well you answered your own question with the last sentence. – Erik Philips Jan 04 '19 at 06:09
  • @ErikPhilips -- I'm not sure how any of your responses are of value to me or the community. While I agree that testing a variable is set has no value, I don't agree with the implication that testing that a factory constructs an object correctly has no value. The example provided is certainly contrived and merely meant to be illustrative of a general situation and instead you choose to share your negative opinions on the example itself instead of providing some insight toward the actual question, which was clearly stated and properly formed. – mhand Jan 04 '19 at 06:28
  • @mjwills -- do you know of a better forum for this type of opinion based question? – mhand Jan 04 '19 at 06:44
  • Closing my own issue as opinion based. Regarding the down votes -- not sure down votes are intended for the purpose of encouraging the closure of an issue. The issue is clearly stated, well formed, and evident that personal research had been conducted and other options had been evaluated and personal opinions referenced. In addition it is very specific and well organized. Thank you to those who shared your opinions. – mhand Jan 04 '19 at 11:05

1 Answers1

1

This is one of the symptoms of approaching unit tests literally.
I would say factory is implementation details of the class which uses IComponent.
And factory would be tested through tests for that class.

I don't care how those tests categorized(unit, integration etc.) - I care only about is my tests slow to run or complex to setup.
So I will mock only things which will make my tests slow or complicated. The logic will be tested within application boundaries which keep tests fast.

For example in Web application it can be Controller -> Database(in-memory or mocked).

Consider unit as "unit of behaviour".

Writing unit tests only because every class should be tested - can be considered as wasting of time. Because any change in internal design will force me to rewrite unit tests or remove obsolete or writ new ones, only for class, which I think is usable at his moment.

Fabio
  • 31,528
  • 4
  • 33
  • 72
  • Thanks for the comment. I don't disagree with your premise. Some projects, whether for better or worse, have requirements to maintain a certain level of unit test coverage. I overwhelmingly agree with your comments on test speed. I think it's imperative that an entire test suite runs in a matter of seconds, not minutes -- which makes tools like `git rebase -ix 'script-to-run-tests'` exceedingly powerful. Also, I've found that if the test suite even takes a full minute, developers tend to either lose focus while running the tests or simply don't run them. – mhand Jan 04 '19 at 11:00
  • RE: only mocking things that make tests slow -- in your experience, do you find that changing/updating an implementation in one part of the code base often requires changing multiple, potentially unrelated, unit tests? I've experienced this on a previous project where it was too time consuming and/or darn near impossible to mock some of the components so we just didn't and often times small bug fixes required updating many seemingly unrelated tests. I suppose that experienced scarred me to far in the other direction :P – mhand Jan 04 '19 at 11:02