godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Backport the engine unit testing framework to Godot 3.x #1533

Open Xrayez opened 4 years ago

Xrayez commented 4 years ago

Describe the project you are working on: Goost - Godot Engine Extension.

Describe the problem or limitation you are having in your project: Running unit tests on CI is difficult using third-party plugins like GUT. Difficult to import assets using headless build of Godot for them to be tested.

As you may know, Godot Engine has unit tests now: godotengine/godot#40659. A lot of effort was put to make it functional, so most classes (if not all) can be unit tested currently in Godot 4.0 with the help of godotengine/godot#40980, which opened up a door to making this actually work for most use cases.

Because of that, we've decided that it's time to document unit testing in Godot, so I've written preliminary documentation in godotengine/godot-docs#4017 (CC @RevoluPowered).

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Most users don't use Godot 4.0 in production, and rightly so. But it's also important for the stable version of Godot to have unit testing system in place.

I propose that we backport the existing doctest unit testing framework to Godot 3.2, so users can benefit from using the unit testing system in Godot via C++ modules. Note that it's totally possible to write self-contained unit tests within C++ modules with the help of godotengine/godot#40720.

I've been working on various C++ modules in 3.2, and I see how this could benefit other developers.

People writing C++ unit tests in modules could also write tests for core functionality as well, so more people could test the engine internals externally, with the chance of such tests to be "upstreamed" to Godot, or when features written in C++ modules could be made part of Godot core if deemed useful by many developers, with tests already written.

That said, this proposal is more about community support rather than core development.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: Tests go through a different entry point, and people would need to compile the engine with scons tests=yes anyway, so this mostly shouldn't impact the stable production code, I think it's just a matter of work to be done to port existing doctest functionality to 3.2.

I can implement this proposal myself.

If this enhancement will not be used often, can it be worked around with a few lines of script?: People can still stick to using GUT or other GDScript-based test frameworks in their C++ modules/projects.

Is there a reason why this should be core and not an add-on in the asset library?: doctest is implemented in C++, and the test runner must be implemented under tests/, so no.

aaronfranke commented 4 years ago

I think this would be fine if you wanted to do it yourself, but do note that 3.2 is a stable branch and mostly receives bug fixes and minor features. Most major changes should only happen on the unstable "master" branch.

Xrayez commented 4 years ago

From Release Policy:

Version Support level
3.2 Backwards-compatible new features (backported from the master branch) as well as bug, security, and platform support fixes.

Nothing is said towards new features being big or small. 🐘

The only "breaking" change the original doctest PR introduced is that the previous tests got inaccessible via command-line interface as described in https://github.com/godotengine/godot/pull/40148#issuecomment-696646329/

But again, according to the release policy:

In the interest of stability and usability, patch releases may occasionally introduce small breaking changes as well.

Not a patch, but I measure the extent of those breaking changes by the number of people using those features, and I suspect there would be only a few.

Of course it can be made so that those tests could still be able to run, and add doctest alongside of existing "tests". That would potentially mean more work to do, if you think that it's important to retain those. @vnen, would you need those GDScript debugging tools on the 3.2 branch as well?

That said, I think it shouldn't impact the functionality of export templates at all, nor editor builds, if done right, unlike those (even small) features which do impact the behavior for most users.

As I said, you still have to compile the engine manually with scons tests=yes in order to benefit from this, which is disabled by default.

vnen commented 4 years ago

I'm not sure if it's worth the effort. The 3.2 branch will eventually be phased out like what happened with the previous versions. I would rather focus on improving tests on master so 4.0 onwards can have a better situation in this regard.

Xrayez commented 4 years ago

I understand the rationale but I guess we have different priorities here.

The 3.2 branch will eventually be phased out like what happened with the previous versions.

I tend to disagree here. In my experience, going from 2.13.03.13.2 was mostly more or less a smooth experience even with some compatibility breakage, and I've been using the latest master branch throughout all this time for my own project development. Far more people use 3.2 when comparing with previous releases such as 2.1 as well.

Now, I really tried to stay on the master branch despite those compatibility breakage and do the necessary changes in my own project to keep up with the development. Unfortunately, one of the major blocks that prevented me from switching to 4.0 was that the GLES3 graphics backend got removed from the engine, and I relied on those specific GLES2 vs GLES3 features quite heavily, and of course the DisplayServer rewrite lead to quite unusable editor. Hopefully we'll resurrect GLES3 at some point so I can postpone switching to 4.0. And of course, the many GDScript 2.0 changes now which I'm not ready to adopt currently.

That said, it makes sense to me to provide a usable testing framework implementation to 3.2, not necessarily because of engine development itself, but because it would benefit other developers in their own projects in the present moment, where the problems actually exist in production even with 3.2-stable.

I'm not sure if it's worth the effort.

So, does it mean that my work in this direction wouldn't be approved nor supported? It's not that much work as you may think.

Of course, the more stable 4.0 becomes and the more features get resurrected, like GLES3 #877, the less relevant this proposal is for me personally, so I could further improve the testing framework on master indeed.

I would rather focus on improving tests on master so 4.0 onwards can have a better situation in this regard.

I'd like that we review and eventually merge documentation for the unit testing in Godot, there's not much people currently involved with this process unfortunately: godotengine/godot-docs#4017.

Calinou commented 4 years ago

I'd like that we review and eventually merge documentation for the unit testing in Godot, there's not much people currently involved with this process unfortunately: godotengine/godot-docs#4017.

Done.

vnen commented 4 years ago

The 3.2 branch will eventually be phased out like what happened with the previous versions.

I tend to disagree here. In my experience, going from 2.13.03.13.2 was mostly more or less a smooth experience even with some compatibility breakage, and I've been using the latest master branch throughout all this time for my own project development. Far more people use 3.2 when comparing with previous releases such as 2.1 as well.

Not really talking about usage, I'm talking about new features (which would justify the need of testing to avoid regressions). We'll be getting less and less new stuff in 3.2 even if there are people using it.

That said, it makes sense to me to provide a usable testing framework implementation to 3.2, not necessarily because of engine development itself, but because it would benefit other developers in their own projects in the present moment, where the problems actually exist in production even with 3.2-stable.

Well, if there are really a bunch of devs needing it, it could make sense.

I'm not sure if it's worth the effort.

So, does it mean that my work in this direction wouldn't be approved nor supported? It's not that much work as you may think.

No, it just means I personally wouldn't invest time in it. I wouldn't stop you from doing it.


Also, regarding existing tests: we should not break compatibility of the CLI options. So whatever is there currently should still work. If this testing system is added, we should use a new option that doesn't conflict with the existing ones.

Xrayez commented 3 years ago

So, is this proposal approved? If yes, I could start working on this (without introducing breaking changes in 3.2.).