I’ve been doing research into how to write test code properly, but one thing has been stumping me. It seems like the only options for setting up a class under test (or evaluating results) is:
Having members be public (breaks encapsulation principle).
Having members be internal, with use of the [InternalsVisibleTo] attribute and friending the test assembly (entire containing assembly has access).
Creating methods specific for testing, which results lots of bloat in original code (maybe partial classes could be good here?).
Reflection, which I’d rather avoid if at all possible since it makes the tests more fragile to changes.
I’m also aware of loads of testing (unit testing in this case) trying to be “black box” testing, only testing publicly available methods and asserting against outwardly visible effects (an object’s position being influenced by a Move method for instance). However there’s often times where the class’s design involves setting private fields in the inspector. Properties help here to a point, until you require a private setter. Sometimes, this problem extends to having something to Assert against as well, as I’ll explain below.
In my current case, I’m working on an AudioManager script which takes in SoundCategory classes (mostly for organization) set in the inspector. These hold multiple Sound classes (also setup in the inspector), which hold AudioClip and AudioSource references created by the AudioManager on Awake. AudioManager has public methods for playing them, handling finding the correct source under the hood by the Sound’s given string name. However, for testing this Play method, I would have to find the correct AudioSource and Assert AudioSource.IsPlaying. I could go with either making a public GetSource method, or (preferably not) loop over the AudioSource instances in the scene during the test to see if it’s the correct one and from there check IsPlaying. But these carry the issues of test code bloat in the class under test, or just being a bit hacky in the latter option. I’m also sure similar problems will keep cropping up with more complex code.
Is there any other method of accomplishing testing without exposing more than their class is originally designed as? Internals seems the best way, but exposing members to the entire assembly they’re a part of seems a bit much. I suppose good naming of internal fields (leading _, “m_”, etc) could be enough to call attention to “you shouldn’t access this in another class”. But I’d like to make sure there isn’t some other option out there. It seems like there should exist an option for “expose this for testing, but not to anyone else even in the same assembly”. I definitely can bite the bullet if need be, but I want to make sure I’m developing good practices wherever possible.
could be worth a look into scriptable objects if you want to avoid a huge mess of public variables that show up in the game object properties.
this could even be a plus for testing because you can have multiple scriptable objects with different values that you can swap at runtime instead of doing a lot of manual editing of various values
There’s a bunch of ways you could to this. But I would really urge you to reconsider the base issues at the heart. Namely: Should this field actually be private. Do you really need to test it? Are you testing it as a readonly value? If not, why isn’t it just publicly available in the first place? I see that a lot where people make things private with it exposed to the editor because, “We don’t want people mucking with this value” and I think: don’t you though? Maybe they (or future you) wants to be able to configure things at runtime.
Outside of those really important questions to ask yourself, the old tool I tend to lean on is simply reflection. Yeah, it’s brittle and breaks easily but if something changes you’ll want to know your tests are broken anyway so that you can once again reconsider your choices from before. At least with hard-coded reflection you’ll see an exception pop up in your tests to let you know something important changed. You could also consider sub-classing with an internal testable version that exposes the internal value through a public API but that just seems gnarly to me and since you’d likely want to make that sub-class internal anyway it’s about the same as your provided solution only with a slightly more obvious naming convention such as ‘internal class MockTestClassForFoo_DontEverUseInProduction’.
You don’t want to test that! You can assume that the AudioSource is fully tested, by Unity. So what’s your actual goal here? It seems like you want to confirm that your Audio class is picking the correct audio source for playback. Whether it actually does that or not doesn’t matter.
This sounds like a case for a mock class where you’d work with an interface in place of the AudioSource, so that you can replace the AudioSource with a TestAudioSource instance which provides a public IsPlaying as well as the audio’s string identifier. If that matches, you know the code up to that point works fine.
If you’re testing things that require objects to be set up like they’re in a scene, why not use a scene? As a part of your test, load a test scene with the correct objects and values set up, and call the required functions on those objects.
It’s a bit clunky, but unlike reflection or mocking, you’re testing actual code that would actually run in the real application.
You’d have to write some scaffolding to make that be practical - Unity’s built in test framework has no good way of doing “load this scene before you do the tests”, as it will insist on running stuff before the scene has initialized properly.
I mean if you want to test things like serialized private fields, then you can always just make a SerializedObject out of a given UnityEngine.Object and test those properties.
I’ve just looked more into use cases such as this for scriptable objects (so far I had mainly used them for item stats and the like). Just finished watching Ryan Hipple’s talk at Unite 2017 on the subject and it was super eye opening and fits this sort of use case, thank you for the lead!
It’s not necessarily the fields I’m trying to run a test on, more that I would be using a private field to assert against to make sure the method under test has actually accomplished what it sought to do (ex. method takes some data, does some maths or validation etc., check to see if it sets a bool to true when it should be true). The string based nature of reflection has seemed a bit off to me usually, however after talking with a friend who had the same idea of wanting test code to at least mildly break if changes are made as a reminder to check the code functions properly, I think I’ve come around to it making sense in this implementation at least.
Testing Unity’s AudioSource.Play method isn’t the method I was testing, and instead a custom Sound class (which holds an AudioClip asset, reference to an AudioSource generated by an audio manager class, and some other values) with a Play method (which does a few other things such as setting the volume/pitch to a random range each time it’s called). Since no other script needs access to its reference to the AudioSource, it’s stored as a private field, but testing the values its supposed to set was the intent of the test (such as making sure the method is actually calling AudioSource.Play, randomizing the pitch, etc).
I’ll have to do a little more research into mock classes, I haven’t made any use of mocks as it seems like it’s not built in to Unity on it’s own (correct me if I’m wrong about that). Though I’ve heard of various packages that implement them that I can take a look see into.
This sounds like a super unique way of tackling it. Definitely a bit messy but could have some pros and cons that make it worth keeping in my back pocket for sure, I’ll do more research into this area should more cleaner solutions fall through.
I haven’t thought about using editor code for grabbing the value, though I’m fairly sure finding fields with SerializedObject requires the use of string literals, which could break the test even if nothing changes other than a field being renamed (my main reason for not using reflection, though I’ve loosened up on being against that thought here). I haven’t looked into the Properties API though, I’ll give it a look see! I’m trying not to rely too much on the asset store so I can fully experience what Unity on it’s own provides, but definitely good callouts if other solutions don’t provide quite what I need here.
Appreciate all the responses from all of you, it’s been very insightful For now I’ll look into using ScriptableObjects as @altepTest suggested since there are benefits outside of just testing, and maybe use Reflection specifically when I just need to check if a value is set properly, now that I can see the benefit of a test being broken after a code change serving as a reminder to make sure the test code is working as it should be.
Well that’s exactly what I’m saying too. Is it actually necessary that those fields are private? In my experience lots of people claim it is because of some abstract dogma of ‘data hiding’. But I find in practicality a lot of times I’d rather have it exposed specifically so that I can a) test with it and b) adjust it at runtime for a more flexible system.
Regardless, it sounds like you need to break the problem down a bit more. If you are just testing a single method, why not give it a parameter? You can still use reflection, or internal access, or whatever but now you’ve completely isolated it from everything else so you know your test will be pure.
You can enumerate child properties of serialized properties, so you can definitely test a whole serialized object without strings for specific fields. Though this would be a more ‘general’ testing. Ergo, making sure values are assigned or correct or whatever else you can define using attributes as well. Though that’s is in essence what the Odin Validator does.
Something that sounds related that always bugged me. I have a MonoBehaviour A that has a field of MonoBehaviour B wired up in the inspector. I want to test the public methods of A which makes calls on B which I need to mock. For want of finding a better solution this is my kludgey way of doing it.
Create an interface for B and also a second field of that interface in A. In A’s Awake do a null check on the interface field, if it’s null assign it the value of the wired field. if it’s not null do nothing as it’s been mocked in a test before the game object is active.
For example:
public class NetworkManager : MonoBehaviour, INetworkManager
{
[SerializeField] TransportManager transportManagerInjected;
ITransportManager transportManager;
private void Awake()
{
if (transportManager == null) transportManager = transportManagerInjected;
}
}
I was never happy with this two field setup, is there a better way of doing this?
I mean I don’t see the point in your code. If TransportManager already implements ITransportManager, then there’s no need to assign it to a less derived type field.
What does doing so actually accomplish? How does this help with testing at all? If you want to expose it as the interface, then just do that. There’s no need to have a redundant field.
But it’s a reference to the same object just through a less derived type. The result would be the same if you accessed it through the transportManagerInjected field.
The transport manager object doesn’t exist in testing as it’s not needed, just the interface to allow it to be mocked. There are more complex calls elsewhere to mocked MonoBehaviours with return values and multiple calls.
Sorry spiney I need to head off to bed, will check back in tomorrow morning.
Then wouldn’t that code run into a null-reference error as nothing is then assigned to transportManager? Or is there something here I’m missing? I’m not familiar with networking.
But from what I can see in the example code. You would add a connection, call SendMessage, the connection is retrieved, then then you hit a null-ref as nothing is assigned to transportManager.
Was it perhaps meant to be transportManager?.SendMessage(connection, message); with the null-conditional operator?
Additionally, would it not be easier to just divert calls between testing and playmode with something like Application.isPlaying?
Or potentially use a proper service-locator type pattern, and have services for testing and for actual runtime?
There’s a thing you can do to fix this, though it’s a bit annoying. We do it for when we want to have [SerializeField] private variables and reference them in custom editors while still being resilient to field renames:
[SerializeField] private Foo foo;
....
#if UNITY_EDITOR
public const string nameof_foo = nameof(foo);
#endif
...
var fooProperty = serializedObject.FindProperty(TheBehaviour.nameof_foo);