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

Idea - Generic wait_until(predicate_func, timeout) & reaching max_time should fail the test & providing predicate functions like is_object_freed #585

Open
WebF0x opened this issue Apr 1, 2024 · 12 comments

Comments

@WebF0x
Copy link

WebF0x commented Apr 1, 2024

Context

Awaiting provides wait_seconds, wait_frames and wait_for_signal, which are great.

A pattern that often comes up in my tests is to wait for a particular condition to be true.
For example

  • repeatedly hit an enemy until it is dead
  • hit an enemy then wait until its HP regenerates to 100%
  • accelerate something until it has velocity X

In all cases, if a certain timeout is reached, I want the test to fail.

Ideas

Generic wait_until(predicate_func, timeout)

  • predicate_func: Callable returning a boolean. Returns true when the success condition is reached
  • max_wait: same as wait_for_signal. Timeout after which we stop blocking the test

Fail on max_time reached

I think wait_for_signal and wait_until should fail the test when the max_time is reached, just like a failed assertion.

is_object_freed predicate

GUT could provide a predicate function returning true when an object has been freed.

Other predicates

Many conditions used in assertions would be great as conditions to wait_until if they were converted into predicate functions (excuse the bad names hehe)

await wait_until(has(vegetables_array, 'potato'))
await wait_until(is_signal_connected(signaler, connector, 'the_signal'))
await wait_until(has_call_count(doubled, 'set_value', 1, [4]))

Usage

func test_when_sword_collides_with_wall_then_it_is_stopped() -> void:
  wall = add_child_autofree(WallScene.instantiate())
  wall.set_position(Vector2.RIGHT * 100)
  sword = add_child_autofree(SwordScene.instantiate())
  sword.set_linear_velocity(Vector2.RIGHT * 300)

  var is_immobile := func() -> bool: return sword.get_linear_velocity().is_zero_approx()
  await wait_until(is_immobile, 5)

Bonus idea: wait until freed

Here's a PR I made for wait_until_freed. Another way to wait until something is freed could be done with GUT just providing a predicate to check if something is freed.

func test_when_enemy_collides_with_sword_then_it_dies() -> void:
  enemy= add_child_autofree(EnemyScene.instantiate())
  enemy.set_position(Vector2.RIGHT * 100)
  sword.set_linear_velocity(Vector2.RIGHT * 300)
  sword = add_child_autofree(SwordScene.instantiate())

  var is_dead := func() -> bool: return is_object_freed(enemy) # is_object_freed being a new predicate provided by GUT
  await wait_until(is_dead , 5)
@WebF0x WebF0x changed the title Idea - Generic wait_until(predicate_func, timeout) & reaching max_time should fail the test & wait until freed Idea - Generic wait_until(predicate_func, timeout) & reaching max_time should fail the test & providing predicate functions like is_object_freed Apr 1, 2024
@bitwes
Copy link
Owner

bitwes commented Apr 24, 2024

I really like this idea. I guess the method would just be called in awaiter's _process method and the wait would end the first time the method returns true.

I think wait_for_signal and wait_until should fail the test when the max_time is reached, just like a failed assertion.

I've often thought this too, but I think some I think some wait_then_assert_ or assert_eventually_ methods should be added instead of making wait_for_signal fail the test. It feels inconsistent with the other wait methods since they don't have a fail worthy result. It also seems like there might be some cases where you might not care if the signal gets emitted. Maybe some complicated setup scenarios.

assert_eventually_ seems to capture the spirit of this idea, but it is an awful long name.

Many conditions used in assertions would be great as conditions to wait_until if they were converted into predicate functions (excuse the bad names hehe)

await wait_until(has(vegetables_array, 'potato'))

could be

await assert_eventually_true(vegetables_array.has.bind('potato'), 5)

I kinda like this

await assert_eventually_eq(vegetables_array.size, 20, 5, 'we need 20 vegetables in 5 seconds or we did not do our job')

Could there be a unwieldy generic assert_eventually?

# your example of 
await wait_until(is_signal_connected(signaler, connector, 'the_signal'))

# could be something like this
await assert_eventually(
    signaler.is_connected.bind('the_signal', connector.callback), 
    assert_signal_connected.bind(signaler, connector, 'the_signal'),
   5,
   'wait for it to be connected or 5 seconds, then assert it is connected')

@WebF0x
Copy link
Author

WebF0x commented Apr 27, 2024

wait_XXXXXXXX shouldn't fail

Agreed. Very good point for the complex setup and not caring if the condition becomes true

Naming it as wait_then_assert..... VS assert_eventually.....

I like both. Small preference for assert_eventually(...) because it stands out from the never-failing wait_XXXXX functions.
I'm cool with long names :)

Generic assert_eventually(...)

Yes to me it's a BIG plus if it's generic, because it lets users

  • create high-level predicates for their game. E.g. is_evolved_pokemon(), is_max_level(), is_bowser_dead(), etc.
  • use the built-in GUT predicates (I'm thinking to extract a predicate from each of these assert methods: is_equal, is_not_equal, is_almost_equal, etc)
  • use the bind operator on built-in Godot methods to make a Callable (I hadn't thought of that but great idea!)

Plus, you'll only need to define each predicate once and people can use them in many places

  • Waits until the condition is true or the timeout, but never fails
    • wait_until(Callable, timeout)
  • Waits until the condition is true or fail after the timeout
    • assert_eventually(Callable, timeout)
  • If we're feeling bold, it could even be expanded by combining the predicates
    • assert_eventually(all([Callable, Callable, Callable]), timeout) All conditions must be true
    • assert_eventually(some([Callable, Callable, Callable]), timeout) At least one condition must be true
    • assert_eventually(none([Callable, Callable, Callable]), timeout) All conditions should be false

@WebF0x
Copy link
Author

WebF0x commented Apr 27, 2024

await assert_eventually(
    signaler.is_connected.bind('the_signal', connector.callback), 
    assert_signal_connected.bind(signaler, connector, 'the_signal'),
   5,
   'wait for it to be connected or 5 seconds, then assert it is connected')

I'm not sure I understand the example here. It has 2 Callable predicates?

@bitwes
Copy link
Owner

bitwes commented Apr 27, 2024

I'm not sure I understand the example here. It has 2 Callable predicates?

The syntax might be slightly off, but the idea was to do the predicate thing with any assert passed as a Callable. If this works the way I think it will, this would be a good first step. Get a generic one to work then use it to implement wrappers like assert_eventually_eq, assert_eventually_between, etc.

await assert_eventually(
    # predicate (Callable that is waited on until it returns true)
    signaler.is_connected.bind('the_signal', connector.callback), 
    # An assert passed as Callable that will be run if the predicate
    # returns true before timeout
    assert_signal_connected.bind(signaler, connector, 'the_signal'),
    # Max time in seconds to wait for predicate to return true, 
    # fails if timeout reached
    5,
    # Optional message to print
    'wait for it to be connected or 5 seconds, then assert it is connected')

@WebF0x
Copy link
Author

WebF0x commented May 4, 2024

Ok I understand now. I think having both a waiting function AND an asserting function makes it too complicated.

In most cases, one could split it in two lines like this:

await assert_eventually(
    signaler.is_connected.bind('the_signal', connector.callback),
    5)
assert_connected(signaler, connector, 'the_signal')

Predicate functions as a building block

I think this will help to implement this proposal and keep the GUT code & user tests simple.

Inspiration

How to keep it DRY (pseudocode)

# New GUT public function
# Nothing fancy, takes 2 objects, returns a bool
# Code to extract from assert_eq
# There would be a function like this for each existing assert_XXXXXX functions
# This is a simple example, but some other asserts would be super useful to extract into predicate functions. Like assert_file_exists, assert_signal_emitted_with_parameters, etc
func is_equal(expected, actual) -> bool:
    return expected == actual
# Existing GUT public function
# Could be refactored to reuse code from above
# Interface doesn't change
func assert_eq(expected, actual) -> void:
    if !is_equal(expected, actual):
        fail(actual, ' is not equal to ', expected)
# New GUT PRIVATE function
# Waits until the condition is true or the timeout, but never fails
# Returns true if successful (predicate_func returned true)
func _has_successfully_waited_until(predicate_func, timeout) -> bool:
    #Whatever magic GUT already does for the existing wait_XXXXXX functions that don't block the game from running
    var has_timed_out := # ... magic here ...
    while !has_timed_out: # ... magic here ...
        if predicate_func.call():
            return true
    return false
# New GUT public function (OPTIONAL)
# No return value. Users don't care if it succeeds or not
# Personally I don't need this, but we could add it if you think people would find it useful
func wait_until(predicate_func, timeout) -> void:
    var _ignored := await _has_successfully_waited_until(predicate_func, timeout)
#New GUT public function
# This is what I actually need for my project 🎉 
func assert_eventually(predicate_func, timeout) -> void:
    var has_succeeded := await _has_successfully_waited_until(predicate_func, timeout)
    if !has_succeeded :
        fail('timed out after ', timeout, ' seconds')
# New GUT public function (OPTIONAL)
# Wrappers that are easier to use
func assert_eventually_eq(expected_value, func_returning_actual_value, timeout) -> void:
    var is_successful_func = func():
        var actual_value := func_returning_actual_value.call()
        return is_equal(expected_value, actual_value )
    await assert_eventually(is_successful_func , timeout)

Usage

# Users can define their own custom predicate functions
func test_when_monster_gets_hit_then_it_dies():
    var is_dead := func(): return monster.get_hitpoints() <= 0
    
    throw_axe_towards_monster()

    await assert_eventually(is_dead, 10)

@bitwes
Copy link
Owner

bitwes commented May 7, 2024

I think this is easy enough and we can do this right away.

#New GUT public function
# This is what I actually need for my project 🎉 
func assert_eventually(predicate_func, timeout) -> void:
    var has_succeeded := await _has_successfully_waited_until(predicate_func, timeout)
    if !has_succeeded :
        fail('timed out after ', timeout, ' seconds')

Breaking the asserts up will get a little messy. Messages change based on inputs, and we'd like to see those same outputs with assert_eventually_eq for both passing and failing tests. I made it through about 3 layers of pseudo code and didn't come up with anything I liked so I stopped.

Getting awaiter.gd to wait for a function to return true and implementing assert_eventually(name still pending) will probably clear up things.

@WebF0x
Copy link
Author

WebF0x commented May 7, 2024

Great! Yeah the custom messages is gonna be tricky. Good point that we don't need to figure those yet for assert_eventually() to work.

@bitwes
Copy link
Owner

bitwes commented May 7, 2024

Based on everything else, I'm assuming you are wanting to implement this (which is great). I just wanted to confirm, so I don't get over-eager and start on it myself.

After a quickish look, I think it would look like the following. This approach isn't written in stone, and somethings might be wrong. So let me know if you have any questions or issues.

awaiter.gd

  • Add wait_for_value(predicate, value, timeout)(name debatable) which will take in a predicate function and call it in _process_physics and end the wait when it returns the expected value.
  • add a property did_last_wait_timeout(also debatable name).
    • This should be set to false at the start of all wait_* methods, and set to true when _wait_time or wait_frames is exceeded in _physics_process.
    • This should be read only. Right now awaiter does not have any public variables, so this should be accomplished with a backing variable and accessors.
var _did_last_wait_timeout = false # this will be used everywhere internally
var did_last_wait_timeout = false :
  get:  return _did_last_wait_timeout
  set(val):  pass

This will give us a general way to wait for something and differentiate between a timeout and a "thing" occurring. The value won't mean anything for wait_frames or wait_seconds but it will be useful with wait_for_signal and wait_for_value. We have to make it work "right" for wait_seconds and _wait_frames though (it will always be true after one of those waits finish), so the awaiter acts predictably.

gut.gd

  • expose _awaiter so that test.gd can use it. For consistency's sake, we should add a get_awaiter(). This is how other read only vars are handled in gut.gd and consistency at the file level is more important than doing it the same way everywhere.

test.gd

  • Add assert_wait_for_true(predicate, seconds, msg='')(name debatable)
    • passes if predicate returns true before timeout
    • fails if seconds elapse before predicate returns true
    • Uses new awaiter.wait_for_value(predicate, true, seconds) and awaiter.did_last_wait_timeout to determine pass/fail.

It might be as simple as this:

func assert_wait_for_true(predicate, seconds, msg=''):
    await gut.get_awaiter().wait_for_value(predicate, true, seconds)
    if(gut.get_awaiter().did_last_wait_timeout):
        _fail(str('predicate function did not return true before timeout:  ', msg))
    else:
        _pass(str('predicate function returned true: ', msg))

@bitwes
Copy link
Owner

bitwes commented May 7, 2024

Hit "comment" too soon. Here's some addition points of consideration.

  • Should wait_for_value return the last value it got? I think you only really care if you get the expected value since, in the general case, the value could be anything each time it is called; so the last value doesn't tell you much of the story. It might be useful in constructing the pass/fail messages though.
  • Or! Should awaiter track all the values that it gets so we can print them when the test fails?
  • Do we need a "check interval" for wait_for_value? I can see someone passing an "expensive" predicate which shouldn't be run on every frame. I think this could be added later since the function signature would still make sense if we added a default parameter at the end wait_for_value(predicate, value, timeout, check_interval=-1).

@WebF0x
Copy link
Author

WebF0x commented May 8, 2024

Yes I'm happy to do it. Might take a while though, I'm just hobbying around. So if you get inspired feel free to do it! We can post progress here, whoever starts first :)

@WebF0x
Copy link
Author

WebF0x commented May 18, 2024

Starting work on it

@WebF0x
Copy link
Author

WebF0x commented May 18, 2024

Resolved by #609

@bitwes your Awaiter trick works. Let me know what you think 🤞

I called it assert_eventually, but it's not a strong preference. Pick whichever name fits best with your naming convention!

I think we could merge it as is and iterate later as needed.
To recap this thread, future ideas could be to make wait_until(predicate_function) public, and provide built-in predicate functions like is_signal_emitted(), is_freed(), file_exists(), etc

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

No branches or pull requests

2 participants