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

Trying get filename for dynamic scripts #655

Open
Scrawach opened this issue Aug 5, 2024 · 4 comments
Open

Trying get filename for dynamic scripts #655

Scrawach opened this issue Aug 5, 2024 · 4 comments

Comments

@Scrawach
Copy link

Scrawach commented Aug 5, 2024

Versions

  • Godot: 4.2.2
  • GUT: 9.3.0
  • OS: Linux

The Bug

I'm experimenting with dynamic scripts here, partly because of the suggestion about type hints in #649 . So I've encountered the following bug related to dynamically created scripts. When assert methods are used, they try to get the filename, assert crashes with an error:

Error calling GDScript utility function "inst_to_dict()": Not base on resource file.

The error concerns code in the strutils.gd file, 86 line:

elif(!GutUtils.is_native_class(thing)):
var dict = inst_to_dict(thing)
filename = _get_filename(dict['@path'])

Seems logical. Dynamic scripts don't have a file, so they can't have a file name.

Possible fixes

Instead of inst_to_dict, you can check the path of the script attached to the object and form the file name on this basis.

	elif(!GutUtils.is_native_class(thing)):
		var script = thing.get_script() as Script
		var path_to_script: String = script.resource_path
		filename = _get_filename(path_to_script)

But there's a problem here. Unfortunately, it doesn't work for inner classes. Such classes have empty resource path too.

Steps To Reproduce

  1. Create test:
func test_get_obj_filename_from_dynamic_script() -> void:
	var utils = load("res://addons/gut/strutils.gd").new()
	var instance = create_dynamic_script().new()
	var filename = utils._get_obj_filename(instance)

func create_dynamic_script() -> GDScript:
	var script := GDScript.new()
	
	script.source_code = """
extends RefCounted
		
func do_something() -> int:
	return 42
	"""
	
	script.reload()
	return script
  1. Run tests.
  2. utils._get_obj_filename throw error.
@bitwes
Copy link
Owner

bitwes commented Aug 5, 2024

I think the problem is that you are not setting the resource path for the script you created. The path does not have to exist and should not be the same as an existing script. I can't remember if there was a problem if you created multiple dynamic scripts that had no resource_path. GUT uses a counter to make sure all dynamically generated scripts have a unique resource_path. Check these out:

Using create_script_from_source looks to fix the issue:

func test_get_obj_filename_from_dynamic_script() -> void:
	var utils = load("res://addons/gut/strutils.gd").new()
	# this method will dedent the script so you don't have to make your dynamic source
	# left justified...which is the best.
	var instance = GutUtils.create_script_from_source("""
		extends RefCounted

		func do_something() -> int:
			return 42
	""").new()
	var filename = utils._get_obj_filename(instance)
	assert_ne(filename, "")
* test_get_obj_filename_from_dynamic_script
    [Passed]:  ["gut_dynamic_script_3.gd"] expected to not equal [""]:  

I messed with this a little bit. I got stuck trying to get the new dynamic script to pass an is test or be accepted by a typed parameter (for example, if you are trying to create a new version of an existing script with a class_name). You can use take_over_path, but I remember having issues. You might be able to just ignore the errors if you set the resource_path to something that already exists, I didn't get far enough to test that out.

This might not be important tough, since we are trying to test how the original thing used something else, and so we wouldn't be passing the new dynamic version to anything.

Good luck and let me know if you have any other questions!

@Scrawach
Copy link
Author

Scrawach commented Aug 5, 2024

@bitwes thanks for answer. I agree. Using GutUtils.create_script_from_source solves the problem, resource_path is set there.

But there's an issue of testability of external code here, when someone creates dynamic scripts in their project (or framework) and decides to write tests with GUT and gets an error when testing, because it can't parse filename. And here's the question: is it a developer's problem that he creates such dynamic scripts, or is it a framework's problem that it doesn't allow assert_* methods for instances with script without resource_path?

So here I'm not quite sure how important this task is. GUT just can say something like: "hey, set resource_path before you check it!". And that's okay.

@bitwes
Copy link
Owner

bitwes commented Aug 6, 2024

I get it now. I saw you calling "private" methods on utils and was thinking you were just trying to get something working, not testing dynamic code. I agree, GUT should be able to handle a missing resource_path.

FYI, don't try to double a dynamic script. That will error because it won't be able to find the file for inheritance. I tried to get it to work to streamline internal tests but I wasn't able to get it work unless I wrote the file to disk first. I know you didn't mention this, but I wanted to save you some trouble since this is the kind of work that led me to try it myself.

@Scrawach
Copy link
Author

Scrawach commented Aug 6, 2024

I get it now. I saw you calling "private" methods on utils and was thinking you were just trying to get something working, not testing dynamic code. I agree, GUT should be able to handle a missing resource_path.

My bad. I reduced all code to problematic place. In reality, it was something like this:

var builder = Builder.new()
var instance = builder.build() # return instance from dynamic script 
assert_not_null(instance)

FYI, don't try to double a dynamic script. That will error because it won't be able to find the file for inheritance. I tried to get it to work to streamline internal tests but I wasn't able to get it work unless I wrote the file to disk first. I know you didn't mention this, but I wanted to save you some trouble since this is the kind of work that led me to try it myself.

Thank you for that. I'll keep that in mind.

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

No branches or pull requests

2 participants