-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Port GDScript test/debugging tools #41355
Conversation
# Enable test framework and inform it of configuration method. | ||
env_tests.Append(CPPDEFINES=["DOCTEST_CONFIG_IMPLEMENT"]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this and define DOCTEST_CONFIG_IMPLEMENT
in test_macros.cpp
once. Including doctest header twice in source files doesn't protect the implementation to be included once, and leads to linker errors with duplicate symbols.
for (int i = 0; i < argc; i++) { | ||
args.push_back(String::utf8(argv[i])); | ||
} | ||
OS::get_singleton()->set_cmdline("", args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first argument is empty because there's no way to know the executable name with the existing testing interface, but I'm not sure whether that's essential in the first place.
Needs a rebase, then it can be merged. |
@@ -17,3 +17,7 @@ if env["tools"]: | |||
# Using a define in the disabled case, to avoid having an extra define | |||
# in regular builds where all modules are enabled. | |||
env_gdscript.Append(CPPDEFINES=["GDSCRIPT_NO_LSP"]) | |||
|
|||
if env["tests"]: | |||
env_gdscript.Append(CPPDEFINES=["TESTS_ENABLED"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? It should already be inherited from the main env
which was cloned for env_gdscript
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because we're actually outside of the main/
env here. Recall #40726, this is what I meant when saying that some modules may eventually need this. Defining TESTS_ENABLED
for all modules is also an option, but GDScript may be just an exception.
Technically, those are more like debug tools the way I see it, but they still qualify as something which facilitate manual testing process.
tests/test_macros.cpp
Outdated
#include "test_macros.h" | ||
|
||
#define DOCTEST_CONFIG_IMPLEMENT | ||
#include "thirdparty/doctest/doctest.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this include doctest.h
first without the define (via test_macros.h
), and then a second time with the define?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, removed doctest header.
Applied suggested changes and rebased. I haven't reported this before (to test out compiler after rebase as well), but I've revealed some crashes by testing out a simple script, they all happen in GDScript code, so I'm not sure whether they are caused by running commands in the test environment or whether those crashes simply need a fix (CC @vnen): # test.gd (at Godot source root)
extends Node
func _ready():
pass
with the following commands: --test gdscript-tokenizer test.gdI suspect this may have something to do with off-by-one error.
--test gdscript-parser test.gdWorks fine. 🙂 --test gdscript-compiler test.gdUninitialized map data root. If you add the following snippet, it would just prevent the crash there: if (GDScriptLanguage::get_singleton()->get_global_map().empty()) {
break;
}
Otherwise my changes are complete. |
Let's merge as is and then work on fixing the identified issues, which don't seem to be introduced by this PR. Could you open issues for them? |
Thanks! |
Helps #41338, @vnen:
You can now run those debug/test tools via command-line with this PR:
You have to compile the engine with
scons tests=yes
in order to be able to use this (if you usedev=yes
, you're already compiling the engine with tests).Implementation details
I haven't modified the inner workings of those existing tools, except for the removed
MainLoop
since it doesn't require it for testing. This should work now as core functionality is initialized with #40980. It's also much faster to iterate because most non-core subsystems are not initialized (rendering, audio etc.), so it kind of works like--no-window
option on Windows, visually.doctest tests are skipped entirely if any such test tool is detected. I'm also reusing the dynamic initialization technique as seen in doctest, with a similar approach.
Example output so you can see it actually works:
GDScript compiler will be provided by #41338. I moved those tools to
modules/gdscript/tests
so will need rebase in either case.