-
-
Notifications
You must be signed in to change notification settings - Fork 575
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
Conversation
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 |
4d720f1
to
7eca5fb
Compare
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! |
As for the |
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 |
My latest push renames the "demo" directory to "project" as requested at the meeting! |
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: (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. |
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 :-) |
There was a problem hiding this 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.
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 |
In order to prevent regressing or reverting old fixes, this PR adds some automated tests to the CI that involve actually running a GDExtension.
Some highlights:
This is a DRAFT because there's a couple I'd still like to figure out:It's downloading Godot v4.1-dev1 to run the tests - it'd be better if it grabbed the latest artifacts from 'master'. There's a GitHub action that should allow us to do this, but I haven't gotten it working yet. See It's trying to download a very old artifact, when much newer ones exist dawidd6/action-download-artifact#240I want to add tests for the issue from PR Fix crash using Ref<T> as parameter #1045. This would make these new tests immediately useful for a problem that we're actively trying to tackle right now.Please let me know what you think!