add_sibling() - Warns of orphans - Might need a new assert_new_orphans(nb_orphans)

[WebF0x]

Using Node.add_sibling results in an orphans warning:

Reproduce

potato.gd

extends Node

class_name Potato

func spawn_another_potato() -> void:
	add_sibling(Potato.new())

test_potato.gd

extends GutTest

func test_spawn_another_potato() -> void:
	var potato: Potato = add_child_autofree(Potato.new())

	potato.spawn_another_potato()

	assert_eq(get_child_count(), 2)

Current behaviour

res://test/unit/test_potato.gd
* test_spawn_another_potato
    [Passed]:  [2] expected to equal [2]:  
[WARNING]:  Test script still has children when all tests finisehd.
  @Node@7:<Node#44845499894>(potato.gd)
You can use autofree, autoqfree, add_child_autofree, or add_child_autoqfree to automatically free objects.
1/1 passed.

[......]

[Orphans]:  1 new orphan in total.
Note:  This count does not include GUT objects that will be freed upon exit.
       It also does not include any orphans created by global scripts
       loaded before tests were ran.
Total orphans = 2

Expected behaviour?

The orphans count warnings is useful to catch bugs or unexpected nodes being created. But in some cases, creating orphans is the expected behaviour, for example if a bullet splits into multiple bullets.

I'm not sure what the right behaviour should be, here are some ideas

• Tell the test how many orphans are expected with assert_new_orphans(nb_orphans). Do not produce warning if the count is OK.
• If using add_sibling() is a bad practice, document an alternative

[bitwes]

You should free the children at the end of the test. In the case of using add sibling, you could create a parent node that gets autofreed, so you don't have to free each one manually. ``` var temp = add_child_autofree(Node2D.new()) temp.add_child(potato) ... ```

…

On Sat, Mar 30, 2024, 2:33 PM Maxime Dupuis ***@***.***> wrote: Using Node.add_sibling <https://docs.godotengine.org/en/stable/classes/class_node.html#class-node-method-add-sibling> results in an orphans warning: Reproduce potato.gd extends Node class_name Potato func spawn_another_potato() -> void: add_sibling(Potato.new()) test_potato.gd extends GutTest func test_spawn_another_potato() -> void: var potato: Potato = add_child_autofree(Potato.new()) potato.spawn_another_potato() assert_eq(get_child_count(), 2) Current behaviour res://test/unit/test_potato.gd * test_spawn_another_potato [Passed]: [2] expected to equal [2]: [WARNING]: Test script still has children when all tests finisehd. @***@***.***:<Node#44845499894>(potato.gd) You can use autofree, autoqfree, add_child_autofree, or add_child_autoqfree to automatically free objects. 1/1 passed. [......] [Orphans]: 1 new orphan in total. Note: This count does not include GUT objects that will be freed upon exit. It also does not include any orphans created by global scripts loaded before tests were ran. Total orphans = 2 Expected behaviour? The orphans count warnings is useful to catch bugs or unexpected nodes being created. But in some cases, creating orphans is the expected behaviour, for example if a bullet splits into multiple bullets. I'm not sure what the right behaviour should be, here are some ideas - Tell the test how many orphans are expected with assert_new_orphans(nb_orphans). Do not produce warning if the count is OK. - If using add_sibling() is a bad practice, document an alternative — Reply to this email directly, view it on GitHub <#584>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI2LKFC6JJ2LLLWAQMDKDLY24AOLAVCNFSM6AAAAABFPX2JTGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGIYTMNRRG4YTOMY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[bitwes]

The main reason you should free the children is so that state does not leak from one test to the other. For example, if you had a method called add_two_more potatoes, and you tried to test that as well, then your assert for child_count will fail eventually, since add_another_potato added a child that wasn't freed.

extends GutTest

func test_spawn_another_potato() -> void:
	var potato: Potato = add_child_autofree(Potato.new())
	# Adds a child to this test that isn't freed
	potato.spawn_another_potato()
	assert_eq(get_child_count(), 2)

func test_spawn_two_more_potato() -> void:
	var potato: Potato = add_child_autofree(Potato.new())
	# Adds two more children that aren't freed
	potato.spawn_two_more_potatoes()
	# This will fail b/c the extra potato from the first test was not freed.
	# The actual count will be 4.
	assert_eq(get_child_count(), 3) 

[WebF0x]

Yes that works, or we can do my current workaround:

func after_each() -> void:
	for child in get_children():
		autofree(child)
```

[WebF0x]

Are you aware of a use-case where someone would NOT want to free all children?
If everyone, like me, always autofree all children, maybe we could move that to GUT core.

=> By default all children get deleted after each test. Deprecate the need to use autofree, add_child_autofree, etc

[WebF0x]

It's not easy to express with just an isolated example. But imagine a bigger project, with many tests instantiating a object that might add siblings.

For a concrete example, in my game, every time you hit something with a sword, the sword might break, which adds sword shards as siblings. Tons of tests use a sword to hit tons of stuff.

So it becomes cumbersome to handle orphans, even in tests where the shards are not the focus. E.g. when hitting something then it is knocked back.

[bitwes]

Are you aware of a use-case where someone would NOT want to free all children?

After each test? Yes.
After each script? No.

You may want a few things to stick around for all tests in a script/inner class. This is why the warning is shown after all tests in a script are run, and not after each test. There are cases where you may want to setup a reusable nodes in before_all, but you should remove those nodes in after_all. Test scripts are not freed after they finish (this is maybe something that could change), so anything added to a test script will stick around for the entire test run. These children can add up overtime and slow things down or pollute your test results. It's bad practice (in general) to not clean up after each test. It's really bad practice to not clean up after each script (not in general, just bad).

So it becomes cumbersome to handle orphans, even in tests where the shards are not the focus. E.g. when hitting something then it is knocked back.

I think the best way to handle this kind of thing is to create a parent node that everything is added to instead of adding to the test node. Maybe something like this:

var _temp_parent = null

func before_each():
    _temp_parent = Node2D.new()
    add_child_autofree(_temp_parent)

func test_something():
    var sword = Sword.instantiate()
    _temp_parent.add_child(sword)
    ...

This gets you a new parent for all your siblings in each test and the parent is freed automatically after each test.

By default all children get deleted after each test. Deprecate the need to use autofree, add_child_autofree, etc

There are two things that autofree and add_child_autofree help with, nodes that are not in the tree (orphans) and nodes that have been added as children to the test script (if you just call add_child or use add_sibling on something that is a child of the test script). These methods have to stick around to address orphans, it just so happens that you can use them to avoid having children in your test script at the end as well.

The only change to GUT that I think makes sense would be to free test scripts after they complete. I think I'd still issue the warning if there were children at the end, but this would address the build-up of nodes and the chance that these nodes could pollute other tests.

[WebF0x]

I agree that it's good testing practice to clear children between scripts. And it's usually a good practice to clear them between each test. Good tests should be independent.

I think GUT should nudge people towards these good practices. It should be the default.

normal_test.gd

extends GutTest

func test_spawn_another_potato():
	# No need to use add_child_autofree, it's freed by default now
	var potato = add_child(Potato.new()) 
	
	# Adds a child that GUT will also automatically free
	potato.spawn_another_potato()
	
	assert_eq(get_child_count(), 2)

# By default, GUT now frees all children between each tests, no worries

func test_spawn_two_more_potato():
	# At this point, there are no children because GUT cleared them all
	var potato = add_child(Potato.new())
	
	potato.spawn_two_more_potatoes()
	
	assert_eq(get_child_count(), 3) 

special_case_test.gd

extends GutTest

var persisting_potato_farm

func before_all() -> void:
	# Special case tests need to call this special GUT method
	# This child will persist between tests
	persisting_potato_farm = add_persisting_child(PotatoFarm.new())

func test_spawn_another_potato():
	var potato = add_child(persisting_potato_farm.make_potato())
	
	# Adds a child to this test that isn't freed
	potato.spawn_another_potato()
	
	assert_eq(get_child_count(), 3) # 2 potatoes & 1 potato farm

# GUT frees all children between each tests, EXCEPT persisting_potato_farm 

func test_spawn_two_more_potato():
	var potato = add_child(persisting_potato_farm.make_potato())
	
	potato.spawn_two_more_potatoes()
	
	assert_eq(get_child_count(), 4) # 3 potatoes & 1 potato farm

In my opinion, the only downside is it would be a breaking change for anyone relying on the current behaviour.

[bitwes]

I think this would break a lot of test suites. I think it would be best to add some documentation around the warning, or improve the warning text, so that it's clear how GUT should be used.

The real fix is probably to free the test scripts after they are done, though the warning should stick around.

[WebF0x]

Yeah that's a tricky one to avoid breaking existing tests.

IF you agree GUT should clear children between each test, maybe it could be in the next major version. Meanwhile people can use workarounds. Possibly have a GUT config option that defaults to the current behaviour but make it possible to toggle the new behaviour.

Or maybe it's just against the project's philosophy. Better to keep GUT consistent even if it adds a little friction for my use case.