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

GDScript: Replace assert() with Utils.check() in tests #96229

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

dalexeev
Copy link
Member

assert() is not evaluated in non-debug build, so some conditions in our GDScript tests are currently not checked in non-debug builds. Please do not use assert() for anything other than testing the assert() itself. Thanks to @kleonc.

@dalexeev dalexeev added this to the 4.4 milestone Aug 28, 2024
@dalexeev dalexeev requested a review from a team as a code owner August 28, 2024 14:47
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 The changes look trivial.

The only potential concerns:

  • Any reason why const Utils = preload("../../utils.notest.gd") was used before over class_name Utils?
  • Any reason why push_error was used before over printerr?

@dalexeev
Copy link
Member Author

dalexeev commented Aug 28, 2024

  • Any reason why const Utils = preload("../../utils.notest.gd") was used before over class_name Utils?

I wasn't sure previously if class_name is supported in the testing system. I probably confused it with the --test gdscript-{tokenizer,parser,compiler} commands.

  • Any reason why push_error was used before over printerr?

push_error() and push_warning() are handled in a special way. >> lines are added to the beginning of .out files. These lines are removed in non-debug builds, since GDScript warnings are not produced in non-debug builds.

There is also an issue with the fact that an active debugger is required to get the GDScript call stack. For this reason Utils.check() is less convenient than assert() when analyzing failed tests. But I have not figured out how to get around this, except to add the required parameter message to Utils.check().

@kleonc
Copy link
Member

kleonc commented Aug 28, 2024

There is also an issue with the fact that an active debugger is required to get the GDScript call stack. For this reason Utils.check() is less convenient than assert() when analyzing failed tests. But I have not figured out how to get around this, except to add the required parameter message to Utils.check().

Hmm, kinda hacky but wouldn't adding assert(condition) into Utils.check do the thing? I mean it shouldn't print on its own anyway when everything is fine, and if the condition fails it would output the stack trace in the debug builds for easy finding the failing line, no? 🤔

@dalexeev
Copy link
Member Author

dalexeev commented Aug 28, 2024

wouldn't adding assert(condition) into Utils.check do the thing?

No, assert(false) in Utils.check() will only point to its location in utils.notest.gd, backtrace will not be available even in debug builds, it requires an active debugger. But if #91006 is merged, we will be able to improve Utils.check(), as in the current implementation the PR removes the requirement for an active debugger.

@akien-mga akien-mga merged commit 2e4c07b into godotengine:master Aug 30, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips mentioned this pull request Aug 30, 2024
10 tasks
@dalexeev dalexeev deleted the gds-replace-assert-in-tests branch August 30, 2024 10:02
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.

3 participants