bitwes / Gut

Godot Unit Test. Unit testing tool for Godot Game Engine.
1.85k stars 100 forks source link

Trying get filename for dynamic scripts #655

Open Scrawach opened 2 months ago

Scrawach commented 2 months ago

Versions

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:

https://github.com/bitwes/Gut/blob/26e0a80da156af1515d63032f5cf0bd4048609f0/addons/gut/strutils.gd#L85-L87

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.