-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Integrate GDScript tests to use doctest #41074
Conversation
Static checks fail on GDScript and test data... 🤔 |
all files should end with a new line i guess
|
Those files are needed to compare generated results from scripts to expected results AFAIK, I haven't delved into this yet. And yeah it would rely on error messages I suppose? The |
Managed to run those tests through doctest now! Current output:
Most fail because I haven't updated the CI fails because Godot needs fixes from #40980, and then likely have to come up with some test contexts godotengine/godot-proposals#1307, so I have to suspend the work for now. I might be able to fix this for current PR locally, but I'd prefer if we review and discuss previous PRs and proposals first, I think I can fix those issues eventually. |
I currently find it problematic to work on this given #41307 on Windows... See also #41616. Created proposal: godotengine/godot-proposals#1429. |
I guess the direction of godotengine/godot-proposals#1429 is that we add test data as a submodule for Godot... I think I could try this out and see the feasibility/practicality of this. |
I'm not quite sure why it fails on Linux/Windows after rebase for now, but this is again testable. I'm not adding scripts via submodule at the moment, godotengine/godot-proposals#1429, should simplify the work on the PR for now, not high priority. Things to do:
|
Test data moved from: https://github.com/godotengine/gdscript-tests Co-authored-by: George Marques <george@gmarqu.es>
Can't run `--test-gdscript` because doctest uses `--test` and prematurely runs doctest related tests instead. Use `--gdscript-test` and `--gdscript-generate-tests` instead for now.
May be hacky but ok for now (running `strip_edges()`)
As seen in godotengine/gdscript-tests@9b2aed6 Added newlines manually... :)
Forgot to mention that the expected output could be potentially configured with a custom reporter: #40890, I see this could be more like |
@Xrayez I've poked into this today and I believe I solved all the issues. I've also submitted #47654 to suppress the output from scripts (which could potentially be used in other tests). Not sure if you want to keep supporting this PR (so I add my fixes on top of your branch) or if I should open a new one. Regarding some extra changes (custom reporting, moving the generate command to test commands) those could be done later as enhancements. We could also review the output format if that makes sense (right now it's just the expected error in the first line, followed by warnings (if there wasn't any parser error), followed by either an error message or the script output). I've also changed to only use the first error message since the following ones are likely cascading from the first and are subject to change (and not really relevant for the tests themselves). |
@vnen feel free to open a new PR to supersede this one. |
Superseded by #47701. |
Test data moved from: https://github.com/godotengine/gdscript-tests
Currently this PR makes it possible to run those script test data against latest
master
branch withthe exactcommand-line tool as in vnen@1db7832.You can still use
--gdscript-generate-tests
to manually generate/update expected output for testing.You can run GDScript-only tests with this command for testing purposes:
You'll have to compile with
scons tests=yes
.