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 doubled parent #660

Open
HotariTobu opened this issue Sep 8, 2024 · 7 comments
Open

Add doubled parent #660

HotariTobu opened this issue Sep 8, 2024 · 7 comments

Comments

@HotariTobu
Copy link
Contributor

Versions

Godot: v4.2.2.stable.official [15073afe3]

The Feature

double(MyClass) makes a class that extends like:

graph LR;
    A --> B --> C --> D
    
    A[Doubled MyClass]
    B[MyClass]
    C[Doubled MyClass parent]
    D[MyClass parent]
Loading

Background

Current double(MyClass) seems to make a class that extends like:

graph LR;
    A --> B --> C
    
    A[Doubled MyClass]
    B[MyClass]
    C[MyClass parent]
Loading

This cannot spy calling super methods from inside MyClass script.
It is because the methods in MyClass context refer to the actual parent's methods.

For example:

# my_class.gd
extends Node


func _process(_delta):
	get_index()
# test_my_class.gd
extends GutTest

const MyClass = preload("res://path/to/my_class.gd")

var _my_class


func before_each():
	_my_class = double(MyClass, DOUBLE_STRATEGY.INCLUDE_NATIVE).new()
	stub(_my_class, "get_index").param_defaults([false])


func test_process_passes():
	_my_class.get_index()
	assert_called(_my_class, "get_index")


func test_process_fails():
	_my_class._process(1 / 30)
	assert_called(_my_class, "get_index")

This feature may be out of the testing philosophy because tests can be made regarding the target implementation.
However, I want to spy on native super methods and request this feature.

@HotariTobu
Copy link
Contributor Author

In the other case, we cannot stub mouse position getters:

# my_class.gd
extends Node2D

var something_pos: Vector2


func _process(_delta):
	something_pos = get_local_mouse_position()
# test_my_class.gd
extends GutTest

const MyClass = preload("res://path/to/my_class.gd")

var _my_class


func before_each():
	_my_class = double(MyClass, DOUBLE_STRATEGY.INCLUDE_NATIVE).new()
	stub(_my_class, "get_local_mouse_position").to_return(Vector2.ONE)


func test_process_passes():
	var pos = _my_class.get_local_mouse_position()
	assert_eq(pos, Vector2.ONE)


func test_process_fails():
	_my_class._process(1 / 30)
	var pos = _my_class.something_pos
	assert_eq(pos, Vector2.ONE)

@bitwes
Copy link
Owner

bitwes commented Sep 9, 2024

I am pretty sure that this has to do with optimizations that Godot is making in the background, which is making stubbing more and more difficult. Godot will call some methods directly, without going through inheritance. This is why they introduced the "Native Method Override" warning, and why you must use INCLUDE_NATIVE to include these methods in a double.

When you call the method directly from a test it appears that it honors the inheritance and calls the doubled method. When the methods are called from within _process Godot is skipping the inheritance and calling the methods as they are in Node2D.

I tried to confuse it enough that it might call the doubled method but that didn't work. Even in the code below, the doubled methods are not called:

#my_class
...
var me = null

func _process(_delta):
	something_pos = me.get_local_mouse_position()
	me.get_index()
...

# test script
_my_class = double(MyClass, DOUBLE_STRATEGY.INCLUDE_NATIVE).new()
_my_class.me = _my_class

I also tried this with a method call foo on my_class and the doubled methods were not called.

Workarounds

In this particular example, the only thing I can think to do is to wrap get_local_mouse_position with another method or pass the result of get_local_mouse_position to something you could test directly with different values.

func handle_mouse_position(mpos):
    # do stuff with the mouse position here

func _process(_delta):
    handle_mouse_position(get_local_mouse_position())

See Also

#649
#633
#484

@HotariTobu
Copy link
Contributor Author

HotariTobu commented Sep 10, 2024

Oh... I see. Thank you.

How's an idea like this:

  • Replace all native method tokens (such as adding __) using RegEx
  • Insert methods (names start with __) and call super native methods in the inserted
  • Use the result script as a doubled class script
  • Add an experimental func to return the new doubled class

Sample code for replace:

extends Node

const PATTERN = "(?<=[^\\p{ID_Continue}])%s(?=[^\\p{ID_Continue}])"
const REPLACEMENT = "__$0"

const SCRIPT = '''

func _ready():
	var value =native_method()
	var value = native_method()
	native_method(native_method())
	native_method(native_method)
	native_method(
		native_method(),native_method()
	)
	native_method ()
	native_method \
	()

	not_native_method()
	native_method_hack()
	
'''

const METHOD_NAME = "native_method"


func _ready():
	var regex = RegEx.new()
	regex.compile(PATTERN % METHOD_NAME)

	var new_script = regex.sub(SCRIPT, REPLACEMENT, true)
	print(new_script)

@bitwes
Copy link
Owner

bitwes commented Sep 16, 2024

I've been thinking about this for a bit, but I don't think it will work. If you are asking to replace internal calls to native methods with a wrapper method then GUT can't do this as it currently works. When GUT creates a double, it doesn't include any of the original source, it only extends and overrides methods. Since it doesn't have any code from the body of methods, there aren't any calls to replace with the regex.

If we added an extra class between the original and the double then maybe this could work. You'd have to apply the regex and then generate all native wrappers, then create a double of the "Regexed" class. If you did this with MyClass what you would get back would be: MyClass -> MyClassRegexed -> double(MyClassRegexed). GUT should probably know not to generate wrappers for the native-wrappers (though it should still work if it did).

It seems this would allow stubbing/spying for any internal calls to native methods, but external calls will still be unreliable.
Even though some external calls go through GUT's wrappers, there are scenarios where native methods are called directly (if the reference to a double is typed, native methods are called directly as described in one of the "See Also" links above). We also don't know how smart Godot will be in the future when calling native methods.

I was excited about this idea's potential, but it feels overly complicated and likely won't solve enough of the issues with stubbing/spying on native methods.

If I didn't understand your original idea, or you have other ideas let me know.

@HotariTobu
Copy link
Contributor Author

My text has misled you a bit.

In my regex idea, MyClass is not used directly.

Let DOUBLE_STRATEGY.REPLACE_NATIVE be a new experimental enum value to double classes based on my regex idea.

MyClass:

# my_class.gd
extends Node


func _process(_delta):
	get_index()

double(MyClass, DOUBLE_STRATEGY.REPLACE_NATIVE) returns Doubled MyClass:

# doubled_my_class.gd
extends "path/to/replaced_my_class.gd"

...

func _process(p__delta=__gutdbl.default_val("_process",0)):
	__gutdbl.spy_on('_process', [p__delta])
	if(__gutdbl.is_stubbed_to_call_super('_process', [p__delta])):
		return await super(p__delta)
	else:
		return await __gutdbl.handle_other_stubs('_process', [p__delta])

...

and Doubled MyClass extends Replaced MyClass:

# replaced_my_class.gd
extends Node

...

func _process(_delta):
	__get_index()

...

func __get_index(p_include_internal=__gutdbl.default_val("get_index",0)):
	__gutdbl.spy_on('get_index', [p_include_internal])
	if(__gutdbl.is_stubbed_to_call_super('get_index', [p_include_internal])):
		return await super.get_index(p_include_internal)
	else:
		return await __gutdbl.handle_other_stubs('get_index', [p_include_internal])

...

Therefore, the result class extends like:

graph LR;
    A --> B --> C
    
    A[Doubled MyClass]
    B[Replaced MyClass]
    C[MyClass parent]
Loading

@HotariTobu
Copy link
Contributor Author

It's good to unify Doubled MyClass and Replaced MyClass if possible.

# replaced_my_class.gd
extends Node

...

func ___process(_delta):
	__get_index()

...


func _process(p__delta=__gutdbl.default_val("_process",0)):
	__gutdbl.spy_on('_process', [p__delta])
	if(__gutdbl.is_stubbed_to_call_super('_process', [p__delta])):
		return await ___process(p__delta)
	else:
		return await __gutdbl.handle_other_stubs('_process', [p__delta])

...

func __get_index(p_include_internal=__gutdbl.default_val("get_index",0)):
	__gutdbl.spy_on('get_index', [p_include_internal])
	if(__gutdbl.is_stubbed_to_call_super('get_index', [p_include_internal])):
		return await super.get_index(p_include_internal)
	else:
		return await __gutdbl.handle_other_stubs('get_index', [p_include_internal])

...
graph LR;
    A --> B
    
    A[Replaced MyClass]
    B[MyClass parent]
Loading

@bitwes
Copy link
Owner

bitwes commented Sep 20, 2024

Ok. I think this is what I was saying in the 2nd paragraph ("If we added an extra class between the original and the double...."). I like the idea, but I'm not sure if adding it is worth the trouble, given all the scenarios where this will not help.

This would go low on my priority list, but I would entertain PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants