-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add integration tests for GDScript #1429
Comments
I fully agree with this proposal. Doctests in the main repository make it very easy to get fully automated regression testing, and it lets people like me try new features in a test-driven way. Without starting their own setup, because they didn't spend enough time looking for already existing ones outside of the main repo... godotengine/godot#41616 ;) |
My main concern about keeping the test cases in the main repo is that there will likely be a lot of those. Most people who clone the repo won't have a need for that because there's not much reason to run the test suite if you're not changing GDScript. On CI it can clone the submodule for testing, so it shouldn't be much big of a deal. The only advantage I can see is matching a bug fix with a new regression test case, which could be sent in a single PR if the tests are in the main repo. |
Quoting @akien-mga godotengine/godot#41616 (comment):
Having a separate repo as a submodule is OK as long as it only contains the test data (scripts), and not the test runner of course. But I wonder what @reduz thinks about adding submodules to Godot, so far I've seen that we don't prefer using submodules according to Godot documentation:
Also, would it really impact the Also, perhaps you worry about that Godot repository will become GDScript-based, and not C++ 😃: I personally think that would be a great promotion for GDScript! If we're aiming for something like https://github.com/CyberShadow/DBugTests, then yeah I guess makes sense, but that's probably because more of maintenance aspect I suspect. So far the direction was "everything bundled" in Godot.
That's the entire point of this proposal, I estimate that only a few of the contributors would be willing to contribute to the regression suite outside of Godot. You'd also have to update the submodule itself everytime in Godot. I believe this should be done often enough because otherwise there's a chance of a bug slipping through the system.
If those GDScript tests take too long to execute, doctest has a feature of filtering out tests, either manually or hardcoded via code, But again even if we have 1000 scripts, it should be fast enough to run. |
I think worrying about too many tests cluttering the repo is a premature worry. In the lucky future where this actually becomes a problem for someone, you can always turn it into a submodule then. |
I don't think the GDScript testing data will surpass ~2 MB anytime soon, so it's probably fine to have it in the main repository. |
I fully agree with those statements because The problem has to exist (... in order for it to be solved):
I just follow the best practices here, of course there are exceptions to the rules and I think the decision is up to core developers anyways. But yeah if you think that including test data as part of the main repository will lead to more contributors who will write a lot of tests (which is more likely than having this outside the main repository), then yeah those tests will have to be maintained, surely, but I think the game is totally worth the candle in this case. 😛 |
Submodules for the actual code is different than submodules for GDScript test files. You would still be able to download and compile Godot without checking out the submodule. The main problem is that they are not included in the source tarball on GitHub, but for test files it isn't a big deal. To be clear: I'm talking about the test cases, not the test suite implementation. Also if we can tell something will be a problem we can take preventive measures. We don't need to wait until it actually becomes a problem. I just want to point out that rules exist for a reason when we might prefer breaking the rules than go against the reason itself (at which point we may tweak the wording of the rules). That is to say that I'm no prominently against keeping the test files in the repo. I just think twice before adding anything to the main repository TBH. If @akien-mga is fine with it we can keep it all in the main repo. |
Yeah we're on the same page.
I'm actually happy to hear those words, for a change. 😛 But yeah, I mean, it's almost the same as having tests written in C++, but it just happens to be that those "tests" will be written in GDScript. One can follow the same logic and say that unit tests written in C++ do not belong to the main repository (if fact all existing C++ doctests can be moved out from the So yeah up to @akien-mga or @reduz to decide I think. |
Legit curious (rhetorical) question: if
I think this kind of setup would be a hell to maintain. |
Describe the project you are working on:
Godot Engine. 🙂
Describe the problem or limitation you are having in your project:
GDScript is constantly evolving, bugs are getting fixed, but at the same time, I've experienced many regressions throughout my game development experience with Godot (starting with Godot 2.1), and probably some are left unnoticed, causing various crashes and/or undefined behavior in release builds of the engine.
Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I'd like that we have a proper system for testing GDScript implementation. It cannot be unit tested per se (languages are not typically unit tested), but providing a regression/integration suite is, in my opinion, essential to the prosperity of a programming language, which should be robust and stable by definition. See the many language bindings, and I wouldn't be surprised to find out if one of the motivations behind creating those bindings is in fact GDScript instability.
Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
I propose that we implement those kind of tests using already integrated doctest unit testing framework. It's still useful even for integration testing because we're interested that any changes to GDScript implementation are properly covered by our GitHub CI.
I insist that we maintain those tests directly within the main repository, so contributors can work on testing out GDScript implementation without much hassle, and to run tests on CI easily, as long as we provide proper documentation for this. Tests can also be marked as "allowed to fail" if GDScript is still under heavy development.
It's not possible to bloat the engine with tests either. You have to explicitly compile the engine with
scons tests=yes
in order to run tests in the first place. There is godotengine/gdscript-tests which is separated from the main repository. There's modules support for doctest as implemented in godotengine/godot#40720, so this can be made self-contained (modules/gdscript/tests
). GDScript being a module is already a separation from core, I don't see a reason why we should split it further away from Godot. Even the tests themselves run through a different entry point.On the other hand, there's also a fuzzing approach to testing: #168. For these kind of tests, yeah it would certainly make sense to run outside of main repository, because the task is to find bugs by breaking the parser, and not providing a deterministic regression suite, which must be reproducible in contrast.
There are a number of pull requests already in work (some of them already working) to showcase the power of GDScript testing:
Possible enhancements marginally related to this proposal: #1307.
If this enhancement will not be used often, can it be worked around with a few lines of script?:
It's possible to maintain a set of tests to check whether GDScript is working outside of the main repository, but then it defeats the purpose of the proposal. You need to be able to react to any regressions quickly enough, else those bugs will be left ignored.
Is there a reason why this should be core and not an add-on in the asset library?:
GDScript is implemented as a C++ module. It's possible to test GDScript for simple parse errors with
godot --script --check-only
command-line options, but it doesn't really makes sense because people can usually find bugs (if lucky) by simply working with the engine.The text was updated successfully, but these errors were encountered: