Skip to content
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 dozens of new integration tests to the GDScript test suite #48029

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 19, 2021

This also makes all scripts use tab indentation (except the ones specifically designed to test space indentation or mixed tab/space indentation).

Reviewing tip: Use the file filter to hide .out files in the Files changed tab:

image

dictionary_consecutive_commas (test that currently crashes)
func test():
	# Dictionaries with consecutive commas are not allowed.
	var dictionary = { "hello": "world",, }

	dictionary = { hello = "world",, }
GDTEST_PARSER_ERROR
Expected expression as dictionary key.

@Calinou Calinou requested a review from a team as a code owner April 19, 2021 22:58
@Calinou Calinou added this to the 4.0 milestone Apr 19, 2021
@Calinou Calinou force-pushed the gdscript-add-integration-tests branch 8 times, most recently from e8d2502 to 8e4d8c5 Compare April 20, 2021 09:50
@Xrayez
Copy link
Contributor

Xrayez commented Apr 20, 2021

Seems like static checks occasionally cause CI errors with *.out files, I think those files should be ignored from static checks eventually. I remedied this problem by stripping and appending the newline for each generated .out file in the original PR, but seems like it's not enough to eliminate those errors completely.

@Calinou
Copy link
Member Author

Calinou commented Apr 28, 2021

Seems like static checks occasionally cause CI errors with *.out files, I think those files should be ignored from static checks eventually. I remedied this problem by stripping and appending the newline for each generated .out file in the original PR, but seems like it's not enough to eliminate those errors completely.

This should be fixed now. I've also added a test for the as keyword.

Edit: Removed the parser-features/as.gd test as it caused CI to crash on all platforms with target=release_debug and the default compiler. (It works on my machine with target=debug and Clang on Linux.)

For reference, the code of the test is below:

func test():
	var some_bool = 5 as bool
	var some_int = 5 as int
	var some_float = 5 as float
	print(typeof(some_bool))
	print(typeof(some_int))
	print(typeof(some_float))

	print()

	var some_bool_typed := 5 as bool
	var some_int_typed := 5 as int
	var some_float_typed := 5 as float
	print(typeof(some_bool_typed))
	print(typeof(some_int_typed))
	print(typeof(some_float_typed))

Crash backtrace from macOS:

===============================================================================
./modules/gdscript/tests/gdscript_test_runner_suite.h:43:
TEST SUITE: [Modules][GDScript]
TEST CASE:  Script compilation and runtime

./modules/gdscript/tests/gdscript_test_runner_suite.h:43: FATAL ERROR: test case CRASHED: SIGSEGV - Segmentation violation signal

===============================================================================
[doctest] test cases:  11 |  10 passed | 1 failed | 349 skipped
[doctest] assertions: 104 | 104 passed | 0 failed |
[doctest] Status: FAILURE!
[1] 1   libsystem_platform.dylib            0x00007fff6a3835fd _sigtramp + 29
[2] 2   ???                                 0xf6b0000070001000 0x0 + 17775707731110400000
[3] GDScriptParser::parse_precedence(GDScriptParser::Precedence, bool, bool) (in godot.osx.opt.tools.64) + 397
[4] GDScriptParser::parse_assignment(GDScriptParser::ExpressionNode*, bool) (in godot.osx.opt.tools.64) + 728
[5] GDScriptParser::parse_precedence(GDScriptParser::Precedence, bool, bool) (in godot.osx.opt.tools.64) + 427
[6] GDScriptParser::parse_statement() (in godot.osx.opt.tools.64) + 1080
[7] GDScriptParser::parse_suite(String const&, GDScriptParser::SuiteNode*, bool) (in godot.osx.opt.tools.64) + 600
[8] GDScriptParser::parse_function() (in godot.osx.opt.tools.64) + 678
[9] void GDScriptParser::parse_class_member<GDScriptParser::FunctionNode>(GDScriptParser::FunctionNode* (GDScriptParser::*)(), GDScriptParser::AnnotationInfo::TargetKind, String const&) (in godot.osx.opt.tools.64) + 149
[10] GDScriptParser::parse_class_body() (in godot.osx.opt.tools.64) + 168
[11] GDScriptParser::parse_program() (in godot.osx.opt.tools.64) + 1415
[12] GDScriptParser::parse(String const&, String const&, bool) (in godot.osx.opt.tools.64) + 1891
[13] GDScriptLanguage::get_global_class_name(String const&, String*, String*) const (in godot.osx.opt.tools.64) + 221
[14] GDScriptTests::GDScriptTestRunner::generate_class_index() (in godot.osx.opt.tools.64) + 523
[15] GDScriptTests::GDScriptTestRunner::run_tests() (in godot.osx.opt.tools.64) + 58
[16] GDScriptTests::_DOCTEST_ANON_SUITE_3863::_DOCTEST_ANON_FUNC_3864() (in godot.osx.opt.tools.64) + 112
[17] doctest::Context::run() (in godot.osx.opt.tools.64) + 4566
[18] test_main(int, char**) (in godot.osx.opt.tools.64) + 1244
[19] Main::test_entrypoint(int, char**, bool&) (in godot.osx.opt.tools.64) + 118
[20] main (in godot.osx.opt.tools.64) + 245
[21] 21  libdyld.dylib                       0x00007fff6a186cc9 start + 1
[22] 22  ???                                 0x0000000000000002 0x0 + 2
-- END OF BACKTRACE --

Edit 2: I'm still getting a crash elsewhere unfortunately (but still not locally). I guess I'll have to remove scripts one by one until it stops crashing…

@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from 6b7ebf7 to 6cdabaa Compare April 28, 2021 23:27
@Calinou Calinou marked this pull request as draft April 29, 2021 21:34
@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from 6cdabaa to 6fb5d26 Compare August 23, 2021 18:25
@mhilbrunner
Copy link
Member

mhilbrunner commented Aug 23, 2021

Built and tested.

tests/scripts/parser/errors/dictionary-consecutive-commas.gd currently crashes the GDScript parser:

func test():
	# Dictionaries with consecutive commas are not allowed.
	var dictionary = { "hello": "world",, }

	dictionary = { hello = "world",, }

@Calinou
Copy link
Member Author

Calinou commented Aug 23, 2021

Built and tested.

tests/scripts/parser/errors/dictionary-consecutive-commas.gd currently crashes the GDScript parser.

Fixed 🙂

@mhilbrunner
Copy link
Member

@Calinou Whats the state of this? :)

@Calinou
Copy link
Member Author

Calinou commented Sep 3, 2021

@Calinou Whats the state of this? :)

It should be mergeable now. We can add more tests later as needed. I rebased it on the latest master branch and ran all tests locally, they all pass.

@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from cb72bf8 to c584bdf Compare September 3, 2021 13:16
@Calinou Calinou marked this pull request as ready for review September 11, 2021 00:06
@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from c584bdf to 8a49851 Compare September 11, 2021 00:07
@mhilbrunner
Copy link
Member

CI disagrees :P

@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from 8a49851 to ca095f8 Compare September 13, 2021 21:18
@Calinou
Copy link
Member Author

Calinou commented Sep 13, 2021

I've added many tests related to heavy nesting of various things in parser/features.

@akien-mga akien-mga requested a review from a team September 14, 2021 09:34
Comment on lines +2 to +5
>> WARNING
>> Line: 2
>> UNUSED_LOCAL_CONSTANT
>> The local constant '_TEST' is declared but never used in the block. If this is intended, prefix it with an underscore: '__TEST'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a fix in the GDScript code.

Copy link
Member Author

@Calinou Calinou Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to fix that warning in the GDScript code. Should I ignore it or should I wait for a fix PR to be made first?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say leave like this for now, but open an issue about it.

This also ignores `.out` files in the file format static checks.
@Calinou Calinou force-pushed the gdscript-add-integration-tests branch from ca095f8 to c0083c0 Compare September 14, 2021 16:42
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on this.

@akien-mga akien-mga merged commit 32f8f74 into godotengine:master Sep 15, 2021
@akien-mga
Copy link
Member

Thanks!

@@ -0,0 +1,3 @@
func test():
# Error here.
print(2 << 4.4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be >>.

Copy link
Member Author

@Calinou Calinou Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch – I'll fix this in an upcoming PR 🙂

Edit: Fixed in #52718.

@Calinou Calinou deleted the gdscript-add-integration-tests branch September 15, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants