assert_eventually(predicate_function, ...) #609
Conversation
There was a problem hiding this comment.
Sorry for the delay, been a busy week. Great start. I'm not sure how I feel about having to relearn my own code in reviewing changes, but I've sen a ton of things I've done that I don't like, heh.
Besides the comments I've added, add the following tests to test_awaiter.gd and get the failing ones to pass
func test_did_last_wait_timeout_is_false_by_default():
var a = add_child_autoqfree(Awaiter.new())
assert_false(a.did_last_wait_timeout)
func test_did_last_wait_timeout_is_readonly():
var a = add_child_autoqfree(Awaiter.new())
a.did_last_wait_timeout = true
assert_false(a.did_last_wait_timeout)
func test_wait_for_sets_did_timeout_to_true():
var a = add_child_autoqfree(Awaiter.new())
a.wait_for(.2)
await a.timeout
assert_true(a.did_last_wait_timeout)
func test_wait_for_resets_did_timeout():
var a = add_child_autoqfree(Awaiter.new())
a.wait_for(.2)
await a.timeout
a.wait_for(20)
assert_false(a.did_last_wait_timeout)
func test_wait_frames_sets_did_timeout_to_true():
var a = add_child_autoqfree(Awaiter.new())
a.wait_frames(10)
await a.timeout
assert_true(a.did_last_wait_timeout)
func test_wait_frames_resets_did_timeout():
var a = add_child_autoqfree(Awaiter.new())
a.wait_frames(10)
await a.timeout
a.wait_frames(50)
assert_false(a.did_last_wait_timeout)
func test_wait_for_signal_sets_did_timeout_to_true_when_time_exceeded():
var a = add_child_autoqfree(Awaiter.new())
var s = Signaler.new()
a.wait_for_signal(s.the_signal, .5)
assert_true(a.did_last_wait_timeout)
func test_wait_for_signal_results_in_false_timout():
var a = add_child_autoqfree(Awaiter.new())
a.wait_for(.2)
await a.timeout # results in true timeout
var s = Signaler.new()
a.wait_for_signal(s.the_signal, 10)
await get_tree().create_timer(.1).timeout
assert_false(a.did_last_wait_timeout)| # This test passes, but prints the error log: | ||
| # ERROR: Lambda capture at index 0 was freed. Passed "null" instead | ||
| # Godot issue: https://github.com/godotengine/godot/issues/85947 | ||
| func test_wait_until_is_compatible_with_checking_if_an_object_is_freed(): |
There was a problem hiding this comment.
In addons/gut/utils.gd there is the following is_freed method:
func is_freed(obj):
var wr = weakref(obj)
return !(wr.get_ref() and str(obj) != '<Freed Object>')
If I change var is_freed to:
var is_freed = _utils.is_freed.bind(node)
the test passes without error.
I'm not sure if this should be another test case or not. Maybe using is_freed should be documented somewhere.
EDIT: I just searched the codebase and utils.is_freed is not used anywhere, but utils.is_not_freed is used in one spot. is_instance_valid is used in a lot of places. Maybe is_freed is a hold over from the godot 4 conversion and should be removed. Do you have any thoughts?
There was a problem hiding this comment.
(can't speak to which one's better between is_instance_valid vs is_freed)
I think a high level test like this should only use what a user would be allowed to use.
That leaves 2 options:
- keep
is_instance_validfor now. Hopefully the Godot team will fix what's causing the error messages - expose
is_freedin the public API, document, etc- (!) once public, it's tough to walk back, so make sure it fits in your vision
- could be in separate PR
- longer term it would be awesome to expose a bunch of helper predicate functions (
is_freed,is_gt,is_almost_equal, etc)- Kinda like Hamcrest matchers or Jest matchers
Either way I left a comment in the code so to help keep track of it
|
Well done. I really like this feature and it has a lot of potential. Thanks for all your work. I'm reserving the right to change some names before 9.3 is released, but I don't want to hold up merging this when I don't have any better ideas yet. If I change anything, I'll mention you in the PR so you can change any tests you are using prior to release. |
|
Thanks! I liked your thoughtful questions and ideas. Nice code base to work with too! |
First step for issue: #585
Replaces #577. Closing it.