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

Add Timer unit tests #91395

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

2nafish117
Copy link
Contributor

Add timer tests
link to #43440

@Chaosus Chaosus added this to the 4.3 milestone May 1, 2024
@2nafish117 2nafish117 marked this pull request as ready for review May 1, 2024 09:28
@2nafish117 2nafish117 requested a review from a team as a code owner May 1, 2024 09:28
@2nafish117
Copy link
Contributor Author

The Timer start/stop test case is currently failing because it is not part of a scene tree, is there a test scene tree that i need to add it to, or should i make a scene tree in cpp and add it to it?

@Chaosus
Copy link
Member

Chaosus commented May 1, 2024

Thanks for your PR, CI failed, use the clang-format tool to prevent such errors (just an advice) (see https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#using-clang-format-locally). Also, commits need to be squashed (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#modifying-a-pull-request).

@2nafish117
Copy link
Contributor Author

Thanks for your PR, CI failed, use the clang-format tool to prevent such errors (just an advice) (see https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#using-clang-format-locally). Also, commits need to be squashed (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#modifying-a-pull-request).

@Chaosus ive managed to fix the clangfmt issue and squashed the commits, the only thing that remains is to fix the start/stop tests requiring a scene tree

@2nafish117 2nafish117 force-pushed the add-timer-tests branch 2 times, most recently from 85d473c to dc5a390 Compare May 1, 2024 10:38
Timer *test_timer = memnew(Timer);

SUBCASE("[Timer] Timer start and stop") {
// @TODO: how do i add the timer to a tree?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen the Node tests do it like this:

SceneTree::get_singleton()->get_root()->add_child(node);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you.

@RedMser
Copy link
Contributor

RedMser commented May 1, 2024

You might want to test the main functionality of timer: the timeout signal. But do so with a short wait time, so it does not take long for the test to complete:

  • Measure current time using Time singleton.
  • Start a one-shot timer with wait time 0.1 seconds.
  • On timeout, measure time again using Time singleton.
  • Ensure the difference between timestamps is at least 0.1 seconds (might be longer, but never shorter).

Could also check e.g. if timer is no longer running after timeout (or if it's not a one-shot timer, ensure that it is still running).

@2nafish117
Copy link
Contributor Author

You might want to test the main functionality of timer: the timeout signal. But do so with a short wait time, so it does not take long for the test to complete:

* Measure current time using Time singleton.

* Start a one-shot timer with wait time 0.1 seconds.

* On timeout, measure time again using Time singleton.

* Ensure the difference between timestamps is at least 0.1 seconds (might be longer, but never shorter).

Could also check e.g. if timer is no longer running after timeout (or if it's not a one-shot timer, ensure that it is still running).

to update the timer, do i also have to manually process the scene tree like so?

SceneTree::get_singleton()->process(0.1);
SceneTree::get_singleton()->physics_process(0.1);

@RedMser
Copy link
Contributor

RedMser commented May 1, 2024

to update the timer, do i also have to manually process the scene tree like so?

I do see calls to that function in the Node tests, so yeah I think so. Also you might want to test process and physics_process separately, depending on which process callback the Timer is set to (so only tick one of the two, not both at once).

@2nafish117 2nafish117 marked this pull request as draft May 2, 2024 12:54
@2nafish117
Copy link
Contributor Author

@RedMser I tried getting the timeout signal to be called by manually calling process on the scene tree, but the callback doesnt seem to be getting called. Do you know why that might be?

Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never wrote proper unit tests for Godot before, so I'm trying to figure it out based on looking at other tests haha. Maybe someone with more of an idea can help you out, e.g. in the godot developers chat...

tests/scene/test_timer.h Outdated Show resolved Hide resolved
@2nafish117 2nafish117 marked this pull request as ready for review May 3, 2024 06:46
@2nafish117
Copy link
Contributor Author

@RedMser ive rebased this with latest from master, could you have a look at this PR once again.

Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but you should wait for a maintainer to approve the workflow run, so you can see if the tests succeed in the CI/CD as well. Maybe you can ask in the Godot Contributors chat, if you don't want to wait.

@akien-mga akien-mga changed the title Add timer tests Add Timer unit tests May 6, 2024
@akien-mga akien-mga merged commit 3768498 into godotengine:master May 6, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@2nafish117 2nafish117 deleted the add-timer-tests branch May 7, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants