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: Support tracking multiple analyzer and runtime errors in tests #99490

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Nov 21, 2024

Currently, the GDScript test system has a problem: it does not allow us to track multiple errors. Because of this, we have to have many small files for error tests, unlike warning and feature tests.

While this limitation is justified for parser tests due to cascading errors, I think there is no obstacle for analyzer and runtime tests to track multiple errors. Probably, using = instead of += in modules/gdscript/tests/gdscript_test_runner.cpp is even a bug, leading to overwriting the output, so we only track the last error.

This PR allows:

  1. Track analyzer warnings that occur before runtime errors.
  2. Track core runtime errors that occur before GDScript runtime errors.
  3. Track multiple GDScript analyzer errors.
  4. Track multiple GDScript runtime errors. This is possible because without an active debugger, only the current function is terminated on a runtime error, and the caller continues execution.

How to make multiple subtests of runtime errors within one test:

func subtest_1():
    # Code with one runtime error...

func subtest_2():
    # Code with one runtime error...

func subtest_3():
    # Code with one runtime error...

func test():
    subtest_1()
    subtest_2()
    subtest_3()

Note: I did not combine all groups of similar tests within this PR.

@dalexeev dalexeev added this to the 4.4 milestone Nov 21, 2024
@dalexeev dalexeev requested review from a team as code owners November 21, 2024 10:15
@dalexeev dalexeev force-pushed the gds-tests-track-multiple-errors branch 2 times, most recently from 20dd4f8 to f5b0d77 Compare November 21, 2024 14:05
@dalexeev dalexeev requested a review from a team as a code owner November 21, 2024 14:05
@dalexeev dalexeev force-pushed the gds-tests-track-multiple-errors branch from f5b0d77 to 1d7e920 Compare November 21, 2024 18:46
@dalexeev dalexeev changed the title GDScript: Add support for tracking multiple runtime errors in tests GDScript: Support tracking multiple analyzer and runtime errors in tests Nov 21, 2024
@dalexeev dalexeev force-pushed the gds-tests-track-multiple-errors branch from 1d7e920 to 5272b83 Compare November 21, 2024 18:54
@dalexeev dalexeev force-pushed the gds-tests-track-multiple-errors branch from 5272b83 to f86dcd4 Compare November 21, 2024 19:09
@Repiteo
Copy link
Contributor

Repiteo commented Nov 21, 2024

So warnings now have use a ~~ prefix instead of >> ? Is there a functional benefit to that, or is it wholly stylistic?

@dalexeev
Copy link
Member Author

So warnings now have use a ~~ prefix instead of >> ? Is there a functional benefit to that, or is it wholly stylistic?

Yes, warnings are not produced in release builds, so we need a marker to cut only them but leave errors.

#ifndef DEBUG_ENABLED
static String strip_warnings(const String &p_expected) {
// On release builds we don't have warnings. Here we remove them from the output before comparison
// so it doesn't fail just because of difference in warnings.
String expected_no_warnings;
for (String line : p_expected.split("\n")) {
if (line.begins_with("~~ ")) {
continue;
}

This doesn't matter now, since GDScript tests don't seem to run for release builds.

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Codestyle checks out! Should make creating tests MUCH more intuitive

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga merged commit b3a44a5 into godotengine:master Nov 29, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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