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

Unit Test Framework #30743

Conversation

RevoluPowered
Copy link
Contributor

@RevoluPowered RevoluPowered commented Jul 21, 2019

This uses doctest and doesn't use Catch2, this was because I was advised that Catch2 would severely decrease build performance.

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

  • Simple testing library
  • We can remove boilerplate for test cases to run test x y z.
  • Most tests will be simplified into header only tests.
  • Assert could be ported into gdscript and so could test cases.
  • product testing with godot would become very easy.
  • less code per test (because you don't need to copy paste a test harness)

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

please note: doctest specific arguments not handled yet, but are not required to make this work #26213

Demo with --test doctest argument

image

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

thirdparty/README.md Outdated Show resolved Hide resolved
@RevoluPowered
Copy link
Contributor Author

Rebased and removed requested items, also edited readme.md to use proper capitalization.

@starry-abyss
Copy link
Contributor

This uses doctest and doesn't use Catch2, this was because I was advised that Catch2 would severely decrease build performance.

I believe doctest is not very popular, it's developed by just one guy, and relies on undefined behaviour.
IMHO, if we separate test and compile targets, then we can use whatever library we like without impacting compile times (i.e. user only compiles, and CI both runs tests and compilation).

@RevoluPowered
Copy link
Contributor Author

@starry-abyss I was advised to not separate them and I believe that unit tests should be executable from the main executable as intended by the godot developers. This is the default behavior of the engine.

Please advise how doctest is unpopular, hacky and only developed by one guy? with examples.

Furthermore: it has 31 contributors so I don't understand how 'one guy' is equal to 31 contributors.

Static analysis rates the code quality higher than godot. It isn't relying on undefined behavior and this requires further explanation.

@RevoluPowered
Copy link
Contributor Author

After this is all working I will re-base my commits to only include the finished code.

@Anutrix
Copy link
Contributor

Anutrix commented Jul 22, 2019

Travis errors are because noexcept specifier. You suppress it for MSVC here https://github.com/godotengine/godot/blob/7c788945a20a315d9f5e8c955f70d2bbee09f18a/thirdparty/doctest/doctest.h#L3224 but I couldn't find it for gcc. Maybe add it for gcc too.

@RevoluPowered

This comment has been minimized.

@RevoluPowered

This comment has been minimized.

@akien-mga
Copy link
Member

It's ok for libraries to use C++11. If you rebase on current master the codebase builds with C++11 support, so it should work fine.

@RevoluPowered
Copy link
Contributor Author

@akien-mga okay, if that's now allowed I will do a rebase and restore the latest version of the library since C++11 is now enabled and usable.

@RevoluPowered RevoluPowered force-pushed the feature/unit-test-framework branch 4 times, most recently from a043f98 to c001c75 Compare July 23, 2019 18:48
@RevoluPowered
Copy link
Contributor Author

Conflict because of new string method #25090 test case 35.

I will port this test soon.

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Jul 24, 2019

Next patch adds backward compatibility for existing tests written in pure C++ rather than with doctest.

new arguments are as follows:
runs the existing tests which are not ported yet to TEST_CASE like before.
./bin/godot.x11.tools.64 --test physics_2d
this runs the doctest based tests:
./bin/godot.x11.tools.64 --test doctest

Already planning on supporting the doctest arguments properly but this will take some time to do.

@RevoluPowered
Copy link
Contributor Author

@akien-mga please can you review this?

@hpvb
Copy link
Member

hpvb commented Jul 30, 2019

This looks good to me, it will simplify test writing a bit so maybe we will get more. And the header is tiny.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 5, 2019

Already planning on supporting the doctest arguments properly but this will take some time to do.

See #26213 to be implemented first perhaps which needs review/test (no pun intended!), also see my little test endeavours in #32387.

@akien-mga akien-mga added this to the 4.0 milestone Nov 7, 2019
Also rewritten string tests in this library
@follower
Copy link
Contributor

follower commented May 4, 2020

FWIW, in case anyone ends up here via a search for Catch2 & Godot or GDNative...

I recently integrated Catch2 into a GDNative module for someone who was trying to "run Godot code outside Godot" but had run into issues.

The example repository is here:

@aaronfranke
Copy link
Member

@RevoluPowered Is this still desired? If so, due to the size of this feature, you should open a proposal which explains example use cases and how this approach will solve the problem.

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented May 26, 2020

@RevoluPowered Is this still desired? If so, due to the size of this feature, you should open a proposal which explains example use cases and how this approach will solve the problem.

We discussed at length with reduz and akien and it was to be merged for 4.0 (at godot con), we are looking to see if another route is practical for 4.0, doctest is still better than no test framework for core, so I was looking for a way around the shared libraries not working for test discovery.

I understand the need for a proposal but this was pre-proposal godot and approved at godotcon

@aaronfranke
Copy link
Member

Yes, but if formalized as a proposal, it also allows you/reduz/akien to organize your thoughts in one place, and it gives everyone a chance to pitch in to the discussion. Having a proposal is more than just deciding whether or not it should be approved. For this topic, there are many approaches and therefore it's worth discussing openly with the community to collect ideas. Writing a proposal also forces you to think in the context of best practices for engine contributors.

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented May 26, 2020

If I get free time I will open a proposal, I'll need to abandon this PR in favor of prioritizing FBX, too much work to go through another iteration right now, sorry got no free time.

@fire
Copy link
Member

fire commented May 26, 2020

<RevoluPowered> Can someone unabandon #30743, feel like I'm beating a dead horse? it's important it's just I need to focus on FBX. Semi-demoralised though since we discussed it at length at godotcon...

@neikeq
Copy link
Contributor

neikeq commented Jul 5, 2020

What needs to be done to salvage this?

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Jul 5, 2020

@neikeq in order to make this PR work we need to update doctest and rebase on 4.0, older version would literally not build on OSX so this is now fixed in the latest doctest. I will update the PR, and see if it works, hopefully its 'okay' really I got blocked by the static libs not working cross core/modules which was the main contention point really (it wasn't practical) however this could work if the latest version fixes the issue.

@akien-mga
Copy link
Member

Superseded by #40148.

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.