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

Added doctest unit test framework #40148

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

RevoluPowered
Copy link
Contributor

@RevoluPowered RevoluPowered commented Jul 6, 2020

This uses doctest to implement unit testing for the core of godot.

UnitTesting Prototype

This includes the following:

  • a very quick test runner of a single test by running the godot executable.
  • the third party library with license file (MIT) and appropriate documentation in the third party folder.
  • modifications to SCSub file to make tests part of main executable as linking tests statically can be hard (need a second opinion on this approach too)

Benefits

Current test runner can be executed by running ./bin/godot.x11.tools.64 --test string, or by specific test name

Demo with ./godot --test argument

image

Demo with backward compatibility with other core tests --test math
image

Articles of supporting info:

@RevoluPowered
Copy link
Contributor Author

#30743 - updated doctest for 4.0 fixed conflicts

@akien-mga
Copy link
Member

Current test runner can be executed by running ./bin/godot.x11.tools.64 --test doctest

How would future tests be handled? Would it be --test doctest as an entry point to run all tests, or can we keep using --test string instead, as it's not particularly relevant what framework is used when calling the test?

Once this is merged, I think we should port all other tests to doctest and document the workflow.

@akien-mga
Copy link
Member

Some style issues: https://travis-ci.org/github/godotengine/godot/jobs/705243011

Also make sure to use braces around single-line if or for blocks.

You lost some of @aaronfranke's changes in test_string while rebasing, so it fails building on Clang: https://travis-ci.org/github/godotengine/godot/jobs/705243015
(Be sure to keep the MinGW-specific hack too.)

@akien-mga akien-mga mentioned this pull request Jul 6, 2020
@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Jul 6, 2020

@akien-mga I have applied the requested changes, but made a slight improvement to the mingw specific section.

I used PRId64 (inttypes.h) to correct this issue as it handles the platform specific issue here for us, but I need to test this on mingw, so will do this in a bit and if it works would it be okay to keep this?

The fix is based on https://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c this article, just so we don't have to duplicate code or do extra casting.

Also just applied the fix you mentioned regarding the {} parenthesis

@RevoluPowered
Copy link
Contributor Author

Current test runner can be executed by running ./bin/godot.x11.tools.64 --test doctest

How would future tests be handled? Would it be --test doctest as an entry point to run all tests, or can we keep using --test string instead, as it's not particularly relevant what framework is used when calling the test?

Once this is merged, I think we should port all other tests to doctest and document the workflow.

I will write a quick fix for this now, to make it so

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Jul 6, 2020

@akien-mga OK all points raised should now be sorted, I expanded the run functionality to make it more usable for regression testing too, it's super simple lets you run whatever specific test you would like too, also leaves legacy compatibility in.

demo gif
UnitTesting Prototype

@aaronfranke can you possibly take a look at the fix I applied here, I think it could be simpler for godot to use this instead, it should remove the need for the preprocessor.

1cd932d

reference: https://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c

main/tests/test_string.h Outdated Show resolved Hide resolved
thirdparty/README.md Outdated Show resolved Hide resolved
main/SCsub Outdated Show resolved Hide resolved
main/SCsub Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
@RevoluPowered RevoluPowered force-pushed the unit-test-revamp branch 2 times, most recently from 4396b43 to e6c3c8e Compare July 16, 2020 14:07
@RevoluPowered RevoluPowered requested a review from vnen as a code owner July 16, 2020 14:32
@RevoluPowered
Copy link
Contributor Author

Nice work, I'll try to test it today.

@vnen @Faless You might want to take a look, and see if you like the new format to write some tests for GDScript and networking respectively.

I had to remove the legacy tests unfortunately so we need to port them

@akien-mga
Copy link
Member

I had to remove the legacy tests unfortunately so we need to port them

That's fine, I don't think there was much to salvage in most legacy tests in the first place.

It's better to start cleanly with the new framework, and then port and modernize the tests which were up-to-date enough with the Godot API (like hashmaps and AStar tests).

@RevoluPowered
Copy link
Contributor Author

That's fine, I don't think there was much to salvage in most legacy tests in the first place.

It's better to start cleanly with the new framework, and then port and modernize the tests which were up-to-date enough with the Godot API (like hashmaps and AStar tests).

Cool, so I left the real files in main/tests/ but they do nothing at the moment to clarify that I didn't remove the actual files, just the legacy test runner, so whoever ports them its simpler, I will try to help with this effort and get them across but need to do this after merge, basis, fbx and math will probably be the ones I port after merge

@RevoluPowered
Copy link
Contributor Author

Possibly we need some system for measuring the code coverage later on too.

@akien-mga
Copy link
Member

Needs a rebase after #40556.

@RevoluPowered
Copy link
Contributor Author

Needs a rebase after #40556.

Done, I ensured no extra whitespace.

@RevoluPowered RevoluPowered force-pushed the unit-test-revamp branch 2 times, most recently from 9c503ca to 5099faa Compare July 24, 2020 11:02
@@ -51,6 +51,11 @@ jobs:
run: |
scons -j2 verbose=yes warnings=all werror=yes platform=osx tools=yes target=release_debug

# execute unit tests for the editor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# execute unit tests for the editor
# Execute unit tests for the editor

Consistency! :)

Implements exit codes into the engine so tests can return their statuses.
Ideally we don't do this, and we use FIXUP logic to 'begin' and 'end' the engine execution for tests specifically.

Since realistically we're initialising the engine here we don't want to do that, since String should not require an engine startup to test a single header.

This lowers the complexity of running the unit tests and even for
physics should be possible to implement such a fix.
@RevoluPowered
Copy link
Contributor Author

For Devs:
To use the unit testing PR you can include the header from thirdparty/doctest/doctest.h it's single header only.

In order to make sure your test is built, once you write your test include the header for it in main/tests/test_main.cpp this ensures it is built as part of the same static linkage to godot's test harness.

To run tests you can use ./godot --test after running this will mean that you no longer can use godot's arguments, you must now use doctest arguments.

The basics:

TEST_CASE("some regression test") {
    CHECK(something_worked());
}

Reference guide
https://github.com/onqtam/doctest

Things which wont work

// by design errors a check should be a SINGULAR check.
CHECK(something && something_else);
CHECK(something || something_else);

@RevoluPowered RevoluPowered mentioned this pull request Jul 24, 2020
10 tasks
@akien-mga akien-mga merged commit 3f1fc5a into godotengine:master Jul 24, 2020
@akien-mga
Copy link
Member

Thanks a ton!

@aaronfranke
Copy link
Member

--test needs to be added to the --help page. Currently --help | grep test returns nothing.

@Calinou
Copy link
Member

Calinou commented Aug 5, 2020

@aaronfranke As of 6831da6, I can see it at the end of the text returned by --help:

--test [--help]                  Run unit tests. Use --test --help for more information.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 22, 2020

Yes, this was done in #40832, the old tests are inaccessible but are still there, but some of those are resurrected now: #41355. See preliminary documentation about tests and those additional test tools in godotengine/godot-docs#4017.

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.

5 participants