Skip to content

initial setup of unity testing framework #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AlexChesser
Copy link

Do you have a forum thread linked to this PR? https://forum.unity.com/threads/what-about-unit-testing.981069/
What will this PR bring to the project for everyone?

Setting up unit tests is a non trivial task within unity. Specifically creating a working Assembly Definition File requires an understanding of DLL references and increases project complexity.

The advantage once the tests are further fleshed out are two fold:

  1. local automated of tests before the commit is pushed will allow developers to make changes without (as much) fear of breaking everything.
  2. automated tests can be scripted to run on-commit via github actions - allowing direct and immediate feedback of a working build.

Why are these changes necessary?

Within the discussion on the thread the is some debate with respect to the value of testing and when to start. Based on my experience having tests in place from the early stages allows things to be developed in a testable way from the start. Adding testing later in the project lifecycle is MUCH more challenging.

How did you implement them?

In this instance I have created a proof of concept which will prove that tests can run within the context of the project.

I have stopped short of including a test of any of the monobehaviors because I'd like to get feedback on the decision to put the ASMDEF file in the root of the project versus having one for scripts and one for the input folder.

Additionally as a next-step goal I'd like to go through the effort of creating a MOCK and demonstrating a MOCK based test. On that front I'd also like to see some feedback on how the wider team would like to see the inclusion of NUGET packages handled. I know how I'd do it on my own, but there may be global preferences about that sort of thing.

I expect this PR will need more feedback - before being accepted - but it is a foundational piece to get the ball rolling.

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2020

CLA assistant check
All committers have signed the CLA.

@AlexChesser
Copy link
Author

Note the one thing that does concern me with this PR is found within the UnityOpenProject1.asmdef assembly definition file

Any libraries referenced in any scripts have to be included as references. So for example: using Cinemachine in one of the scripts, requires it be added to the ASMDEF. Without this definition file, it just seems to "work" based on unity-magic but with this in place every assembly has to be specifically referenced.

I feel like that does create a level of complexity that will be new to a lot of unity developers and possibly that perfect combination of unwelcome, annoying, frustrating and, stupid. I don't know if there is a solution to that outside of updated documentation.

Does anyone know of a way to auto-add referenced assemblies via ASMDEF?

@neon-age
Copy link
Contributor

AFAIK, there is no way to auto-reference every package with asmdef.
The only way I see is not to use them, and just put all tests in Editor sub-folder.

@AlexChesser
Copy link
Author

Hmmm ... Interesting. Thanks for the insight. That'd be a pretty reasonable workaround, except the ASMDEF on the main project is required in order to be able to reference its code from within the context of the tests.

So for example, you can't actually create a "Protagonist" controller to run tests on it unless you can reference the assembly.

@AlexChesser
Copy link
Author

AlexChesser commented Oct 19, 2020

Update: I've put together a bit of reorganization.

  • names for ASMDEF files are more fully qualified and specific for easier identification
  • main SCRIPTS have been moved to their own ASMDEF rather than pointing all scripts to the root
  • Auto Generated Input scripts have been placed in their own ASMDEF
  • DLL packages and dependencies for using Moq within unit testing have been included and configured

Still outstanding:

Ideally having a runnable unit test framework in place when things start coming in LIKE the finite state machine will make it easier to stay on top of testing and quality from the start of the project.

@AlexChesser
Copy link
Author

Per a forum comment, this PR adds 4 external dependencies each is listed here with a link to the licenses included

These license files have been extracted or downloaded as appropriate and included in the Assets/Plugins/UnitTesting.Mock folder

@ciro-unity ciro-unity changed the base branch from master to main October 20, 2020 13:25
@ciro-unity ciro-unity added the enhancement New feature or request label Oct 21, 2020
@rpgrca
Copy link

rpgrca commented Oct 29, 2020

Just leaving some comments here instead of the forum post. Disclaimer: I have no clue about Unity but I'm somewhat good at unit/integration/functional testing and TDD/BDD/ATDD methodologies.

  • Personally I'd suggest using Xunit, NUnit is a testing framework that has some vices which might encourage people to abuse them without knowing it (the Setup/Teardown pattern, for example, encourages contributors to fatten the Setup with initializations that are not necessary for other tests, Assert.Pass is an assert that tests nothing, instead it encourages programmers to add logic to tests, the Assert.Inconclusive method that marks a test as not passed but still passing it as okay, and the ThrowsNothing which tests no exception is thrown, which is already the default for every test line).
  • Test names should be descriptive from the beginning, and I don't consider TestingTest as a descriptive name. I see that it's a sample test and that it just tests the framework (which I guess has already been tested by the NUnit folks) but people are bound to copy/paste from examples so you might end up with hundreds of "TestingTestXXXYYY" everywhere. Copying the name of the class into the test method is also a smell, you certainly don't want that as it will eventually turn into an enumeration of tests as TestingTest0, TestingTest1, etc.
  • Personally (note that this is my personal way of writing names and I'm just offering it as an example) I use a class ending with Must (I've seen others use Should instead but it doesn't enforce the correct idea that tests must pass) along a descriptive method name that complements the class name. For example, I'd rename the first one to SimpleTestMust.PassImmediately, and would probably move the second test to its own class as you'd certainly not want people to mix different types of tests (unit tests must fly).
  • Finally, if you're adding tests you should also modify the yaml file to run them automatically. These tests will never break though, which is why I suggest starting writing tests that actually test something instead of just placeholders.

Regards,

@kgc00
Copy link
Contributor

kgc00 commented Nov 3, 2020

Hi , I have an example of testing input using the new input system. https://github.com/kgc00/BeatEmUpSandbox/blob/combat_v2/Assets/Scripts/TestSandbox/PlayMode/KeyLoggerTests.cs
(more info here: https://docs.unity3d.com/Packages/[email protected]/manual/Testing.html)

Maybe I'll take a look tomorrow and see if there is a quick fix

Alternatively you could just not test the input, assume it works since it is unity core, and test the event systems / downstream code that uses it.

EDIT: Hi, i was able to get some Inputreader unit tests in. They're in this PR #144 but feel free to use my approach here as well, Alex.

@ciro-unity ciro-unity added the on hold Depends on functionality we haven't decided on yet, or is beyond scope. Will be revisited later. label Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request on hold Depends on functionality we haven't decided on yet, or is beyond scope. Will be revisited later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants