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 unit test runner for autocompletion #85178

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

HolonProduction
Copy link
Member

@HolonProduction HolonProduction commented Nov 21, 2023

To prevent GDScript changes from breaking auto completion capabilities.

This PR adds the possibility to write Auto Completion tests. However it does not add any tests. I want to have the runner merged before working on test cases.

@Calinou Calinou added this to the 4.x milestone Nov 21, 2023
@HolonProduction HolonProduction force-pushed the completion-tests branch 3 times, most recently from df43949 to 93be6e9 Compare November 30, 2023 17:03
@HolonProduction HolonProduction changed the title Add unit tests for autocompletion [WIP] Add unit tests for autocompletion Dec 7, 2023
@HolonProduction HolonProduction force-pushed the completion-tests branch 6 times, most recently from 550ecb9 to 321e9d9 Compare December 9, 2023 01:43
@HolonProduction
Copy link
Member Author

Just noticed that there is no Github CI that runs tests with mono enabled. Would this be possible? I see no reliable way to test auto completion with SCRIPT types without using another script language besides GDScript. Because any way of obtaining a SCRIPT typed reference to a GDScript might break if we decide to intelligently upgrade the type in the future.

@AThousandShips
Copy link
Member

This is because of issues, see the workflow file comment:

Disabled due freeze caused by mix Mono build and CI

@AThousandShips
Copy link
Member

AThousandShips commented Dec 9, 2023

I've tested running a few times with the tests enabled and don't see any freezing, but I think this should be enabled and tested extensively in a separate PR to avoid any problems, see here for the original reason

@HolonProduction HolonProduction changed the title [WIP] Add unit tests for autocompletion [WIP] Add unit test runner for autocompletion Dec 17, 2023
@HolonProduction
Copy link
Member Author

This should be ready for a first round of review now, since I added the features which I wanted to add.

What still remains to do:

  • squash the commits (will do at the end, but while in review having the history might be helpful)
  • remove dummy tests (will do at the end, but they are needed for testing)

@HolonProduction HolonProduction marked this pull request as ready for review December 17, 2023 21:35
@HolonProduction HolonProduction requested review from a team as code owners December 17, 2023 21:35
@HolonProduction HolonProduction changed the title [WIP] Add unit test runner for autocompletion Add unit test runner for autocompletion Dec 17, 2023
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, seems to pass tests and the tests makes sense to me

modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

The idea itself looks good to me, I just found some small details.

I also wonder if we can have an easy way to create the cfg files since writing them by hand might be tricky and prone to typos, especially if they get big (though tests should probably be small anyway).

modules/gdscript/tests/scripts/completion/foo.notest.gd Outdated Show resolved Hide resolved
var test = 1

func _ready():
$N￿
Copy link
Member

Choose a reason for hiding this comment

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

This 0xFFFF character might be annoying to add as there is no an easy way to insert it besides copying and pasting, and it's inconsistent how it's shown across editors. Though I can't think of a better alternative for this at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could choose some UTF8 emoji and replace it with 0xFFFF. The use of this emoji in the test would just be banned. Maybe ➡️ or 🖱️. But I'm not sure whether there could be problems because those emojis are longer then normal characters.

Copy link
Member Author

@HolonProduction HolonProduction Jan 3, 2024

Choose a reason for hiding this comment

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

I ended up using ➡. It doesn't solve the need for copy and pasting, but it should be correctly displayed by most editors.

Edit: I also thought about → or ⇒ since, at least on some systems they can be entered through the compose key (e.g. mine 😄). But some editors display ->/=> in the same (or a very similar) way. So I think it might get too confusing.

modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
modules/gdscript/tests/test_completion.h Outdated Show resolved Hide resolved
@vnen
Copy link
Member

vnen commented Dec 18, 2023

Thanks for working on this, BTW. It is something on my mind for a while so I'm glad to have tests for completion.

@HolonProduction
Copy link
Member Author

I also wonder if we can have an easy way to create the cfg files since writing them by hand might be tricky and prone to typos, especially if they get big (though tests should probably be small anyway).

Generating them through CLI like the other GDScript test would be quite inconvenient I think. It sounds more like an entry for the tools menu which gives you some UI that allows to configure the test and spits out the cfg and maybe also the script with the inserted sentinel char. But since it would be dev only, having it in the menu seems wrong.

Maybe I will write a little editor plugin for it, but that is something to think about when actually writing test cases. I can also imagine that copy and pasting an existing cfg will be quite sufficient for most cases.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 18, 2023
@HolonProduction HolonProduction force-pushed the completion-tests branch 2 times, most recently from ddb7e2b to 6f18cca Compare January 3, 2024 14:51
@HolonProduction HolonProduction force-pushed the completion-tests branch 2 times, most recently from ea65bd2 to d7c9ebe Compare January 3, 2024 15:01
@HolonProduction HolonProduction requested a review from vnen January 3, 2024 15:03
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

LGTM

@akien-mga akien-mga merged commit b88535f into godotengine:master Jan 8, 2024
15 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.

6 participants