Test serialization #132

dbnicholson · 2024-07-15T18:47:43Z

This is an effort to start testing how the serialization works. In actuality, I only tested how the GDScript is generated and didn't get to how the resource is serialized. I hope to look at that next but need to understand those parts better first. In order to run this from headless tests, some refactoring was needed. In a nutshell, I want to move as much of the data handling out of the UI elements with proper interfaces.

This will conflict with #107. I'm happy to hold off on this until that lands.

dylanmccall · 2024-07-16T01:09:20Z

addons/block_code/ui/blocks/block/block.gd

-@export var bottom_snap_path: NodePath
+@export var bottom_snap: SnapPoint


I was wondering about this. Thanks for doing it! 👍

I wouldn't have even noticed or bothered except it was making it a lot harder to test it without manually calling _ready().

dylanmccall · 2024-07-16T01:24:35Z

tests/test_instruction_tree.gd

+	# print_block.param_input_strings = {"text": "this is a test"}
+	# print_block._ready()
+
+	# XXX: Why does insert_snapped_block add_child but not set snapped_block?


Ooh, that's a bit of a mistake with our API there. The setter for snapped_block should really be a private setter - it gets set by the SnappedBlock node itself when a child is added. But gdscript doesn't have a concept of private setters, so instead it's just a mistake waiting to happen. We should probably get rid of that setter altogether and instead have a SnappedBlock._set_snapped_block.

Good to know. I got a little lost between SnapPoint and DragManager on what the exact semantics were there. Among other things, one of the reasons I like to write these tests is it often forces you to tighten up the API. I've seen several places where some object gets mutated in a seemingly unrelated place. That's completely understandable for where this project is at, but I'd like to clean up some of those parts.

dylanmccall · 2024-07-16T03:24:55Z

tests/test_instruction_tree.gd

+	# XXX: It seems like this should substitute {text} in the statement,
+	# but it doesn't. I can't make sense of StatementBlock.
+	# print_block.param_input_strings = {"text": "this is a test"}
+	# print_block._ready()


I puzzled over this for a little bit, and this error seems pertinent:

SCRIPT ERROR: Invalid set index 'text' (on base: 'Nil') with value of type 'String'. at: ParameterInput.set_raw_input (res://addons/block_code/ui/blocks/utilities/parameter_input/parameter_input.gd:57)

GutTest is a Node, so we can call add_child(ready_block) instead of having to call print_block._ready() and I think that moves things a little closer. I also changed this…

ready_block.bottom_snap.insert_snapped_block(print_block) ready_block.bottom_snap.snapped_block = print_block

to…

ready_block.bottom_snap.add_child(print_block)

… but I still ran into an issue where SnapPoint._on_child_entered_tree isn't being called, even though SnapPoint is definitely initialized.

Yeah, I completely missed the errors when I was running it until today 🙄 I have a refactor in ParameterInput that allows it to work without the UI components but it's a bit messy. Since I have to rebase this after #107, I'll try to clean it up and add it.

I did get this working in tests, but I was unsure whether I was breaking the option handling, so I'm going to punt on that part for the moment.

print_block.param_input_strings = {"text": "this is a test"} add_child(print_block) # Add print_block to scene tree to have it initialize properly remove_child(print_block) # Make sure it's available to snap to the ready_block ready_block.bottom_snap.insert_snapped_block(print_block) # Attach to ready_block ready_block.bottom_snap._update_snapped_block_from_children() # Set snapped_block in ready_block assert_true(ready_block.bottom_snap.has_snapped_block()) assert_eq(ready_block.bottom_snap.get_snapped_block(), print_block)

Changing the test to this works, but it's pretty weird, see the explanation of why it works in the comments. What Dylan said is exactly why the error is happening, but this is a solution. Once the new serialization is in, this test will look more clean.

Oh, duh. I had convinced myself that _ready would never be called in these tests even though I knew I'd seen it run before in these contexts. Turns out GUT has a helper to add the child and have it automatically removed and freed at the end of the test.

On the other hand, I was make ParameterInput work outside of a scene by storing raw_input in the object and reworking when it's transferred to and from the UI controls. I'll try to polish that up, but if I can't I'll do it this way.

manuq

Besides the initial tests, this sets a good floor for improving the data model. And the cleanup commits are very welcome.

Needs a rebase to fix conflicts.

manuq · 2024-07-16T10:32:53Z

addons/block_code/block_code_plugin.gd

@@ -24,7 +24,6 @@ const DISABLED_CLASSES := [
 	"StatementBlock",
 	"DragDropArea",
 	"SnapPoint",
-	"NodeBlockCanvas",


👍 not worth the generalization, there is a single kind of canvas.

Yep, at one point I thought maybe we could have scripts that aren't attached to nodes, which is an interesting idea, but for now BlockCanvas has already started accumulating code that is reliant on attaching scripts to a node, so it makes sense to combine them.

manuq · 2024-07-16T10:33:38Z

addons/block_code/instruction_tree/instruction_tree.gd

@@ -14,13 +14,13 @@ class TreeNode:
 		children.append(node)


-func generate_text(root_node: TreeNode, start_depth: int = 0) -> String:
+static func generate_text(root_node: TreeNode, start_depth: int = 0) -> String:


👍 yes, these should be static.

manuq · 2024-07-16T10:43:41Z

addons/block_code/ui/blocks/block/block.gd


 func _ready():
-	bottom_snap = get_node_or_null(bottom_snap_path)


👍 yes! no need to do this.

These methods don't use any state, so there's no reason to allocate an InstructionTree object. This likely cleans some memory leaks as the allocated objects weren't being freed in NodeBlockCanvas.

Handling the script generation in the UI canvas code seems misplaced, and it depends on nothing in the UI except for the current canvas window. Move the generation to InstructionTree using an interface taking an array of Node. This will allow testing the code generation without using the block canvas.

NodeBlockCanvas extends BlockCanvas, but the only thing it was providing was the method to generate the script from the current window. Might as will just put that in BlockCanvas and drop the extension.

Godot knows how to serialize a reference to a node directly, so there's no need to store a path and restore the node manually. This is clearer, but it also means that bottom_snap can be used without waiting for it the Block to be ready.

These have been unused since 9a3e010. They can be recovered from the history if needed again in the future.

The block_name property isn't really used for anything, so the value isn't really critical. However, for debugging it's helpful if they're unique. Unique names also allow building a dictionary of the available blocks, which is useful when building a block script in code for testing.

This gets a little further to verifying how we get from Blocks to actual scripts. Parameter substitution isn't currently tested as that seems difficult to manage without the UI.

dbnicholson · 2024-07-16T16:53:27Z

Rebased now. Seems to be working...

wnbaum

Tests and changes look good to me. Once we do our serialization differently the test_X_script tests will be a lot cleaner, so not to worry.

wnbaum · 2024-07-16T19:18:15Z

tests/test_instruction_tree.gd

+	# XXX: It seems like this should substitute {text} in the statement,
+	# but it doesn't. I can't make sense of StatementBlock.
+	# print_block.param_input_strings = {"text": "this is a test"}
+	# print_block._ready()


print_block.param_input_strings = {"text": "this is a test"} add_child(print_block) # Add print_block to scene tree to have it initialize properly remove_child(print_block) # Make sure it's available to snap to the ready_block ready_block.bottom_snap.insert_snapped_block(print_block) # Attach to ready_block ready_block.bottom_snap._update_snapped_block_from_children() # Set snapped_block in ready_block assert_true(ready_block.bottom_snap.has_snapped_block()) assert_eq(ready_block.bottom_snap.get_snapped_block(), print_block)

Changing the test to this works, but it's pretty weird, see the explanation of why it works in the comments. What Dylan said is exactly why the error is happening, but this is a solution. Once the new serialization is in, this test will look more clean.

dbnicholson requested review from manuq, dylanmccall and wnbaum July 15, 2024 18:47

dylanmccall mentioned this pull request Jul 16, 2024

Add _to_string for Block #134

Merged

dylanmccall reviewed Jul 16, 2024

View reviewed changes

manuq approved these changes Jul 16, 2024

View reviewed changes

dbnicholson added 7 commits July 16, 2024 09:24

InstructionTree: Make generation methods static

edd1f65

These methods don't use any state, so there's no reason to allocate an InstructionTree object. This likely cleans some memory leaks as the allocated objects weren't being freed in NodeBlockCanvas.

Remove NodeBlockCanvas

8b41b8d

NodeBlockCanvas extends BlockCanvas, but the only thing it was providing was the method to generate the script from the current window. Might as will just put that in BlockCanvas and drop the extension.

Block: Handle bottom_snap property directly

6a11ebe

Godot knows how to serialize a reference to a node directly, so there's no need to store a path and restore the node manually. This is clearer, but it also means that bottom_snap can be used without waiting for it the Block to be ready.

Remove unused NodeCanvas, NodeList and friends

eca64ac

These have been unused since 9a3e010. They can be recovered from the history if needed again in the future.

InstructionTree: Add tests for serializing scripts from Blocks

6ff5dbd

This gets a little further to verifying how we get from Blocks to actual scripts. Parameter substitution isn't currently tested as that seems difficult to manage without the UI.

dbnicholson force-pushed the test-serialization branch from 846b118 to 6ff5dbd Compare July 16, 2024 16:50

wnbaum approved these changes Jul 16, 2024

View reviewed changes

wnbaum merged commit 633e4a1 into main Jul 16, 2024
2 checks passed

wnbaum deleted the test-serialization branch July 16, 2024 19:26

		@export var bottom_snap_path: NodePath
		@export var bottom_snap: SnapPoint


		func _ready():
		bottom_snap = get_node_or_null(bottom_snap_path)

Test serialization #132

Test serialization #132

Uh oh!

Conversation

dbnicholson commented Jul 15, 2024

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!

manuq left a comment

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!

dbnicholson commented Jul 16, 2024

Uh oh!

wnbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!