Some Types #773

AdminCrystal · 2025-10-01T01:30:08Z

No description provided.

bitwes · 2025-10-02T16:33:11Z

TL;DR

Here's what I'm thinking right now, but I'm open to discussing any of these:

Either add Variant to all parameters or none, unless you have a justifiable reason one way or the other.
Do not add types to assert_string_* methods for now. We can do this later, as a different PR that would be easier to rollback if it is a problem.
More or less use of StringName.
Adding return types is great.

Make any changes you see fit and let me know when the PR is ready to review and I'll start evaluating individual methods more thoroughly.

The TL part

I'm good with adding return types to everything. I especially like it with the wait_ methods since it's not explicitly clear how the magic of await wait_seconds works.

I'm unsure about where to use String vs StringName. It seems like more of these should be StringName. Maybe file paths should all be StringName. All property-name and method-name parameters should be StringName, unless they can be null. I'm not 100% sure of all the implications of StringName other than it can improve performance if you use it consistently enough. Sparse/inconsistent StringName use confuses me more than having no type.

I'm on the fence with assert_string_ methods. I like the GUT message a little better than the engine one, but the method only accepts strings, so types make sense. We wouldn't have been able to change this earlier since Godot didn't have the ability to notify us of errors. Now that GUT can fail when an error occurs, these could be changed to have types.

I think we have to add Variant to everything that does not have a type, or nothing. There might be a few cases where adding it helps clarify things, but I can't think of any off the top of my head.

AdminCrystal · 2025-10-03T01:32:57Z

I believe adding Variant to params doesn't help much because everything is a Variant by default, so if its mostly all variant or nothing nothing is probably better since its less code to read through. However could probably make the argument we label everything as either Variant, Object, or Node. Then you know its one of the trifecta. Honestly I would probably forget what the difference between Variant and Object is but maybe the difference between Variant and Node could be helpful

Even going through and adding correct types even if that is something we wanted to do is hard as it requires going through a lot of code that is also missing types so have to have a lot of understanding of the codebase and even then the end user may send something different anyways and its somehow working for them and I break it. Way I look at it since types aren't already there by adding them now it may break someone unknowingly which you may or may not be ok with, fully understand if not since it is a testing framework after all. For return types since Gut controls that it should be safe to add the type

Ya wherever possible StringName will have performance benefit over String and whats nice is to the end user they are same thing as Strings get auto converted to Stringnames automagically so they don't have to do anything different. Generally I think the places we would use it is in function calls to engine internals functions which they themselves specify the type as StringName so those are obvious users of StringName, but any place we need to modify the String then we can't use StringName
https://docs.godotengine.org/en/4.4/classes/class_node.html#class-node-method-rpc

For number 4 I have this pr. The test class and spy class are the first ones that come to my mind for most used but there is probably more I haven't interacted with yet that I could add to.
#776

bitwes

Based on the conversation and the code I think this is what a first step would be.

Add return types
Any method where we add a type hint to a parameter should get hints for all parameters. This cuts down the scope of where we us Variant at least. Adding return types does not require all parameters to be typed.
StringName for any parameter that is a method name or a property name (probably other rules but this is the only one I could think of).

I looked through each of the methods that were changed and left comments on some.

is hard as it requires going through a lot of code that is also missing types so have to have a lot of understanding of the codebase

I'm happy to answer any questions about a method so you don't have to dig through and try to understand everything just to add types. Make a list of any methods that you have questions about and I can let you know what I think they should be.

by adding them now it may break someone unknowingly which you may or may not be ok with

Yea, there is a lot of grey area around this. Now that GUT can fail when an error occurs, I think we have a lot more freedom to add types. This feature is very new, and maybe we shouldn't trust it yet though. In the past a type mismatch when calling a GutTest method would have caused execution to exit the test. If you hadn't asserted by that point then the test would be marked risky, but if you had a passing assert then the only way you knew something went wrong was to see it in the log.

I'll look at the return types one next and we can go from there. Let me know if you have any other questions.

EDIT I got PR's mixed up so I updated this last line to actually make sense.

bitwes · 2025-10-06T17:32:09Z

addons/gut/test.gd

 ##    assert_string_contains('abc 123', '012')
 ## [/codeblock]
-func assert_string_contains(text, search, match_case=true):
+func assert_string_contains(text: String, search: String, match_case: Variant = true) -> void:


match_case should be bool. Make text : Variant.

bitwes · 2025-10-06T17:33:54Z

addons/gut/test.gd

 ## and check the backing variable value.  It will then reset throught the setter
 ## and set the backing variable and check the getter.
-func assert_property_with_backing_variable(obj, property_name, default_value, new_value, backed_by_name=null):
+func assert_property_with_backing_variable(obj, property_name, default_value, new_value, backed_by_name=null) -> void:


could be: Object, StringName, Variant, Variant, StringName

bitwes · 2025-10-06T17:35:08Z

addons/gut/test.gd

 ##[/codeblock]
 ## See [wiki]Awaiting[/wiki]
-func wait_while(callable, max_time, p3='', p4=''):
+func wait_while(callable, max_time, p3='', p4='') -> bool:


Callable, float, Variant, Variant

bitwes · 2025-10-06T17:52:10Z

addons/gut/test.gd

 ## returns what is passed in so you can save a line of code.
 ##   var thing = autofree(Thing.new())
-func autofree(thing):
+func autofree(thing: Variant) -> Variant:


I think this should be Node. If we change it, it will cause issues for anyone that is misusing it. Misusing it isn't a big deal. It should at least be Object. autofree.gd ignores anything that is RefCounted (except doubles of RefCounted, but this method isn't used to add those).

bitwes · 2025-10-06T17:52:19Z

addons/gut/test.gd

 ## instead.  This also imparts a brief pause after the test finishes so that
 ## the queued object has time to free.
-func autoqfree(thing):
+func autoqfree(thing: Variant) -> Variant:


Same as autofree

bitwes · 2025-10-06T17:53:32Z

addons/gut/test.gd

 ## See also [method wait_while][br]
 ## See [wiki]Awaiting[/wiki]
-func wait_until(callable, max_time, p3='', p4=''):
+func wait_until(callable, max_time_seconds, p3='', p4='') -> bool:


Callable, float, Variant, Variant

bitwes · 2025-10-06T17:53:51Z

addons/gut/test.gd

 		message = p3

-	_awaiter.wait_until(callable, max_time, time_between, message)
+	_awaiter.wait_until(callable, max_time_seconds, time_between, message)


Callable, float, float, String

bitwes · 2025-10-06T18:02:14Z

addons/gut/test.gd

 ##    assert_string_starts_with('abc 123', 'abc 1234')
 ## [/codeblock]
-func assert_string_starts_with(text, search, match_case=true):
+func assert_string_starts_with(text: String, search: String, match_case: bool =true) -> void:


Change to text : Variant

bitwes · 2025-10-06T18:02:22Z

addons/gut/test.gd

 ##    assert_string_ends_with('abc 123', 'nope')
 ## [/codeblock]
-func assert_string_ends_with(text, search, match_case=true):
+func assert_string_ends_with(text: String, search: String, match_case: bool = true) -> void:


Change to text : Variant

Working

02f04a7

AdminCrystal changed the title ~~Working~~ Some Types Oct 1, 2025

bitwes reviewed Oct 6, 2025

View reviewed changes

Some Types #773

Are you sure you want to change the base?

Some Types #773

Uh oh!

Conversation

AdminCrystal commented Oct 1, 2025

Uh oh!

bitwes commented Oct 2, 2025

TL;DR

The TL part

Uh oh!

AdminCrystal commented Oct 3, 2025

Uh oh!

bitwes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bitwes left a comment •

edited

Loading