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 automated tests that run a GDExtension (rather than just building it) #1101

Merged
merged 1 commit into from
May 25, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Apr 29, 2023

In order to prevent regressing or reverting old fixes, this PR adds some automated tests to the CI that involve actually running a GDExtension.

Selection_029

Some highlights:

  • It converts the project in 'test/' to be just an integration test NOT also an example for users. If we want to have a true example project, personally, I think it should be in its own repo, and it should be pared down to just showing the things we really want beginners to know about. At the moment, it has a little bit of that, but it's also just doing things to make sure they work. Let's split them! (This will also require changing the 2nd paragraph on this docs page, that recommends this as an example: https://docs.godotengine.org/en/latest/tutorials/scripting/gdextension/gdextension_cpp_example.html)
  • It adds a simple test harness for asserting things. This isn't very complete but can be improved over time. I don't want to get carried away with this in the first PR.
  • It turns everything that can be made assert-able in the original demo project, into assertions. A handful of little things couldn't be converted (so for those we're really just making sure we're not crashing :-)), but I think anything is better than nothing for now. Like the previous point, we can improve this over time.
  • It adds some tests for the bug fixed by Fix crash using Ref<T> as parameter #1045. I had actually intended to add a test for the regression caused by that PR, but I didn't consider that that would be a failing test. :-) So, while we work on fixing that regression, the tests here will make sure we don't bring back the bugs in Crash when passing Image to a function  #1021 and Crash using Ref as parameter #1024. Here is a patch adding a test that does show the regression, which we can add after it's fixed.

This is a DRAFT because there's a couple I'd still like to figure out:

Please let me know what you think!

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Apr 29, 2023
@dsnopek dsnopek added this to the 4.1 milestone Apr 29, 2023
@dsnopek dsnopek requested review from a team as code owners April 29, 2023 17:18
@dsnopek dsnopek marked this pull request as draft April 29, 2023 17:20
@dsnopek
Copy link
Contributor Author

dsnopek commented Apr 29, 2023

I got it downloading Godot from the latest artifacts on the 'master' branch!!

I had to patch action-download-artifact: dawidd6/action-download-artifact#241

@dsnopek dsnopek force-pushed the better-tests branch 2 times, most recently from 4d720f1 to 7eca5fb Compare May 1, 2023 18:41
@dsnopek
Copy link
Contributor Author

dsnopek commented May 1, 2023

Alright, I added some tests related to #1045. They actually test the issue that #1045 fixed, since those tests will pass - it doesn't test the regression caused by #1045 because that would fail

Attached to this comment is a patch adding a test that does show the regression, but I guess that'll have to merged with a fix, so that it passes :-)

At this point, there's nothing left that I want to add here - taking out of "draft" status!

test-pr1045-regression.patch

@dsnopek dsnopek marked this pull request as ready for review May 1, 2023 18:46
@dsnopek dsnopek mentioned this pull request May 1, 2023
@saki7
Copy link
Contributor

saki7 commented May 3, 2023

As for the Ref<T> issue the current approach looks solid. I completely agree that we can gradually improve the tests; this PR would be a great first step.

@dsnopek
Copy link
Contributor Author

dsnopek commented May 13, 2023

Discussed at the GDExtension meeting: looks good, except rename the "demo" folder to "project"

As far as an actual demo: we dicussed cleaning up https://github.com/paddy-exe/GDExtensionSummator and promoting it to be an official example in the godotengine organization

@dsnopek
Copy link
Contributor Author

dsnopek commented May 14, 2023

My latest push renames the "demo" directory to "project" as requested at the meeting!

@dsnopek
Copy link
Contributor Author

dsnopek commented May 16, 2023

The last test on this PR didn't pull the most recent artifact from Godot, but one from the day before, which has me a little worried:

Selection_039

(The test ran on 2023-05-14 but the artifact is from 2023-05-13.)

I'm going to do some additional tests to make sure there isn't some issue with the GitHub Action. We really need this to grab the latest artifact for situations where we need to coordinate merges between Godot and godot-cpp.

@dsnopek
Copy link
Contributor Author

dsnopek commented May 17, 2023

Alright, I added some code to the GitHub action that should ensure that we're always getting the latest artifact:

dsnopek/action-download-artifact@1322f74

It seems to be working in my testing, and also in the latest CI build on this PR!

So, I think this PR is (once again) ready :-)

Copy link
Collaborator

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Discussed in GDExtension meeting, we're all happy with these changes, should be merged.

@dsnopek dsnopek merged commit 2e45bd8 into godotengine:master May 25, 2023
11 checks passed
@dsnopek
Copy link
Contributor Author

dsnopek commented May 25, 2023

Blergh. This just had a run on PR #1113 where it pulled an old expired artifact. :-/ I ran this a whole bunch of times on my fork and it was working consistently there! I may need to dig back into this one...

Here's the output from that run: https://github.com/godotengine/godot-cpp/actions/runs/5080843000/jobs/9128373154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants