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-based unit testing #19

Merged
merged 12 commits into from
Nov 22, 2024
Merged

GDScript-based unit testing #19

merged 12 commits into from
Nov 22, 2024

Conversation

limbonaut
Copy link
Collaborator

@limbonaut limbonaut commented Nov 15, 2024

Adding gdUnit4 unit testing framework to the demo project and a couple of test suites. The framework supports headless mode, and has a marketplace CI action. However, it can't be deployed as a submodule, unless we use symlink (it's not cross-platform then).

Review notes: First commit is the framework source (those 26K lines is this commit alone).

EDIT:

  • Now importing as a submodule. See GDScript-based unit testing #19 (comment) below.
  • Creating a symlink for the testing framework during the build process on Linux and macOS.
    • Creating an NTFS junction on Windows due to elevated permissions required for symlinks.

image

@limbonaut limbonaut added enhancement New feature or request testing labels Nov 15, 2024
@limbonaut
Copy link
Collaborator Author

Can we use git-subtree as a potential alternative to submodule here?

@limbonaut limbonaut linked an issue Nov 15, 2024 that may be closed by this pull request
@bruno-garcia
Copy link
Member

Can we use git-subtree as a potential alternative to submodule here?

Doesn't look like we're using git submodule though, right? The files were added directly

Review notes: First commit is the framework source (those 26K lines is this commit alone).

Can this be a submodule?

@limbonaut
Copy link
Collaborator Author

limbonaut commented Nov 15, 2024

Can this be a submodule?

No, because we need a subfolder from that repo at a specific location in our repo.
We need their addons/gdUnit4 at ours project/addons/gdUnit4. It's a common problem with Godot add-ons.

EDIT:
I also mentioned above, that we can use a symlink, but it won't be cross-platform. The way how it can be done is this:
We add it as a submodule: project/modules/gdunit. And we symlink project/addons/gdUnit4/ => project/modules/gdunit/addons/gdUnit4. However, it will not work on Windows then.

@bruno-garcia
Copy link
Member

You suggested git-tree, is that an alternative?

It might be worth excluding development from Windows then, I dunno. Vendoring all this code seems quite suboptimal, it's 26kloc. And keeping things updated then with upstream will be painful this way

@limbonaut
Copy link
Collaborator Author

limbonaut commented Nov 17, 2024

It might be worth excluding development from Windows then, I dunno. Vendoring all this code seems quite suboptimal, it's 26kloc.

Both GDScript frameworks have this disadvantage, maybe the other one is smaller LoC-wise.

And keeping things updated then with upstream will be painful this way

There is a built-in update routine, which checks for the update when you launch the Godot project. So it's a semi-automated process, and perhaps it handles the compatibility of the plugin version vs Godot version.

You suggested git-tree, is that an alternative?

git-subtree is an alternative to submodule, and I'm wondering if it could be utilized here to better solve this issue. It's tailored for the same use-case as submodule, but much different in implementation - you keep the history locally. This history can be squashed, synced with upstream, so like a submodule that is in-tree. This command can also extract history of a specific subfolder from a repository into a separate tree, but I couldn't find a way to "import" history from a subfolder, which is what we need.

@limbonaut limbonaut force-pushed the feat/gdscript-unit-testing branch 4 times, most recently from 09b6b9e to 93eface Compare November 20, 2024 21:24
@limbonaut
Copy link
Collaborator Author

Changes

  • Import gdUnit4 testing framework as a submodule
  • Creating a symlink for the testing framework during the build process (on Linux and macOS)
    • Can also run scons project/addons/gdUnit4 to create the symlink manually
  • New PowerShell script to create the symlink on Windows
    • The script prompts for elevated privileges, as Windows requires them to create a symlink.
    • Note: This operation is not performed during the build process, as it requires elevated privileges, and prompting for them during the build would be confusing.

src/sentry_sdk.cpp Show resolved Hide resolved
SConstruct Outdated
os.symlink(src, dst)
return 0

gdunit_symlink = env.Command(

Choose a reason for hiding this comment

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

Can't we env.command(powershell.exe...) from here on Windows as well? I guess Scons will need to run with elevated privileges but there does not seem to be a way around this on windows anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Also, did you consider another approach? Like this: https://stackoverflow.com/questions/58038683/allow-mklink-for-a-non-admin-user

Copy link
Collaborator Author

@limbonaut limbonaut Nov 21, 2024

Choose a reason for hiding this comment

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

@bruno-garcia
Not sure which one of those you mean 😕
Developer mode is not default, so failing a build without it seems unreasonable to me, at least for this.
With the hard link, I get "Access denied".

> mklink /H LinkedMusic Music
Access is denied.

EDIT:
#19 (comment) worked like a charm.

@bruno-garcia
Copy link
Member

Note: This operation is not performed during the build process, as it requires elevated privileges, and prompting for them during the build would be confusing.

How do we make it work in CI in this case?

@limbonaut
Copy link
Collaborator Author

limbonaut commented Nov 21, 2024

Note: This operation is not performed during the build process, as it requires elevated privileges, and prompting for them during the build would be confusing.

How do we make it work in CI in this case?

Easy. We'll see if we have the privileges, if not: cp -R or mv should do the job.

EDIT:
#19 (comment) worked.

@limbonaut
Copy link
Collaborator Author

limbonaut commented Nov 21, 2024

Changes

  • Creating an NTFS junction instead of a symlink on Windows during SCons build
  • Removed the unneeded PowerShell script

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Just a note on the README.md, other than that, LGTM

.gitmodules Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@limbonaut limbonaut force-pushed the feat/gdscript-unit-testing branch from 62d56aa to 234b8ab Compare November 22, 2024 15:41
@limbonaut limbonaut merged commit 17b9db3 into main Nov 22, 2024
3 checks passed
@limbonaut limbonaut deleted the feat/gdscript-unit-testing branch November 22, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GDScript-based unit testing
3 participants