search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
86.5k stars 19.27k forks source link

[TRACKER] Unit tests to add or improve #43440

Open Calinou opened 3 years ago

Calinou commented 3 years ago

Our unit test coverage is currently fairly low. We'd like to increase our unit test coverage; any help is welcome.

Interested in writing new unit tests? See the unit tests documentation and compiling instructions. If you have further questions, join the Godot Contributors Chat.

When opening a pull request, please link back to this issue (#43440) in the PR description so that we can keep track of it more easily.

If you have the appropriate permissions, feel free to edit this issue to add new items or check items for which a PR has been opened.

Classes to test

These classes are currently lacking in test coverage, and are therefore highest-priority for receiving unit tests. Deprecated classes are not listed.

[!NOTE]

When a class is listed with "and" along a given list item, it should be submitted in the same pull request whenever possible. Tests for these classes can be in the same file or a different file depending on the size and complexity of the test suite. If in doubt, follow the file organization used in the original class implementation.

Completed classes

These classes currently have good test coverage. Further improvements may be possible by testing methods that were added after the tests were merged.

- [x] **AABB** https://github.com/godotengine/godot/pull/43727 - [x] **Animation** https://github.com/godotengine/godot/pull/57490 - [x] **Array** https://github.com/godotengine/godot/pull/46227 - [x] **ArrayMesh** - [x] **AudioStreamWAV** https://github.com/godotengine/godot/pull/60736 - [x] **BitMap** - [x] **Camera2D** https://github.com/godotengine/godot/pull/88614 - [x] **Camera3D:** May not be testable in headless mode, but it's worth trying. Test `is_position_behind()` and the `project_`/`unproject_` methods in particular - [x] **CryptoKey** https://github.com/godotengine/godot/pull/89021 - [x] **Curve** https://github.com/godotengine/godot/pull/43516 - [x] **Curve2D** https://github.com/godotengine/godot/pull/71308 - [x] **Curve3D** #76812 - [x] **Dictionary** https://github.com/godotengine/godot/pull/47511 - [x] **FastNoiseLite** - [x] **Font** https://github.com/godotengine/godot/pull/81503 - [x] **GDScript** https://github.com/godotengine/godot/pull/48657 - [x] **Geometry2D** https://github.com/godotengine/godot/pull/59643 - [x] **Geometry3D** https://github.com/godotengine/godot/pull/44974 - [x] **GradientTexture** https://github.com/godotengine/godot/pull/90501 - [x] **HashingContext** https://github.com/godotengine/godot/pull/43459 - [x] **HTTPClient** https://github.com/godotengine/godot/pull/76636 - [x] **Image** https://github.com/godotengine/godot/pull/45737 - [x] **ImageTexture** https://github.com/godotengine/godot/pull/88044 - [x] **InputEvent** https://github.com/godotengine/godot/pull/79444 - [x] **InputEventKey** https://github.com/godotengine/godot/pull/59193 - [x] **JSON** https://github.com/godotengine/godot/pull/43517 - [x] **JSONRPC**: https://github.com/godotengine/godot/pull/89124 - [x] **Math** https://github.com/godotengine/godot/pull/48721 - [x] **Marshalls** https://github.com/godotengine/godot/pull/44797 - [x] **Node**: https://github.com/godotengine/godot/pull/70152 https://github.com/godotengine/godot/pull/71367 - [x] **NodePath** https://github.com/godotengine/godot/pull/43578 - [x] **NoiseTexture** https://github.com/godotengine/godot/pull/70919 - [x] **Object** https://github.com/godotengine/godot/pull/43583 (completed by https://github.com/godotengine/godot/pull/44387) - [x] **OS** https://github.com/godotengine/godot/pull/42069 - [x] **PackedScene** https://github.com/godotengine/godot/pull/79440 https://github.com/godotengine/godot/pull/80423 https://github.com/godotengine/godot/pull/80819 - [x] **Path2D** https://github.com/godotengine/godot/pull/66927 - [x] **PathFollow2D** https://github.com/godotengine/godot/pull/46277 - [x] **Path3D** - [x] **PathFollow3D** https://github.com/godotengine/godot/pull/46341 - [x] **PCKPacker** https://github.com/godotengine/godot/pull/43572 - [x] **Plane** https://github.com/godotengine/godot/pull/44492 - [x] **PrimitiveMesh** (CapsuleMesh, CubeMesh, …) - [x] **Projection** https://github.com/godotengine/godot/pull/77275 - [x] **Quaternion** https://github.com/godotengine/godot/pull/50907 - [x] **RandomNumberGenerator** https://github.com/godotengine/godot/pull/44560 - [x] **Rect2:** Not covered in the legacy math test. https://github.com/godotengine/godot/pull/43514 - [x] **Rect2i:** Not covered in the legacy math test. https://github.com/godotengine/godot/pull/43514 - [x] **RegEx** https://github.com/godotengine/godot/pull/82225 - [x] **RID** https://github.com/godotengine/godot/pull/54325 - [x] **Resource** https://github.com/godotengine/godot/pull/43731 - [x] **Shortcut** https://github.com/godotengine/godot/pull/58712 - [x] **SpriteFrames** https://github.com/godotengine/godot/pull/57742 - [x] **Transform** - [x] **Transform2D** - [x] **Translation** https://github.com/godotengine/godot/pull/48778 - [x] **TranslationServer** https://github.com/godotengine/godot/pull/79331 - [x] **Variant** https://github.com/godotengine/godot/pull/76244 - [x] **Vector** https://github.com/godotengine/godot/pull/48821 - [x] **Vector2/Vector2i** https://github.com/godotengine/godot/pull/47202 - [x] **Vector3/Vector3i** https://github.com/godotengine/godot/pull/48063 - [x] **Vector4/Vector4i** https://github.com/godotengine/godot/pull/64027 - [x] **VisualShader** https://github.com/godotengine/godot/pull/70396

Non-testable classes

These classes can't be unit-tested for technical reasons. Unit tests always run in headless mode, so they can't do things such as rendering scenes and checking the visual result.

- [x] ~~**CubeMap:**~~ Not testable without RenderingServer access. - [x] ~~**Shader:**~~ Not testable without RenderingServer access.
ghost commented 3 years ago

Hi me and a friend would like to take up some of these unit tests for a class project we have. Is there someone we can stay in touch with if we have questions? @Calinou

Calinou commented 3 years ago

@singher Thanks for your interest in contributing! I recommend joining the #godotengine-devel IRC channel on Freenode so you can ask development-related questions if needed.

I also recommend writing a comment here when you're starting to write tests for one class. This way, we can avoid stepping on each other's toes.

ghost commented 3 years ago

@Calinou perfect! Thank you so much for this opportunity. We will try our best to cover as many of those classes to test.

ghost commented 3 years ago

@Calinou Hi, I am working with @singher on this. We just wanted to let you know that we were planning on writing test cases for the following classes: Array, Dictionary, Node, RandomNumberGenerator.

Xrayez commented 3 years ago

we were planning on writing test cases for the following classes: Array, Dictionary, Node, RandomNumberGenerator.

@vinayakmtiwari RandomNumberGenerator initial test suite is written in #43103 and #43343 for new (not yet merged) functionality, so I guess you could take it as a base.

MaxMutantMayer commented 3 years ago

I would like to tackle some of the test cases too and would start with the classes HashingContext and RegEx. Do you prefer separate PRs for each class, or should I aggregate them?

Ps.: First time contributor here!

Xrayez commented 3 years ago

@MaxMutantMayer it's better to create separate PRs for each class, this eases the review process → potentially speeds up the merging process. 😉

Also, I'd focus on one thing at a time.

MaxMutantMayer commented 3 years ago

@Xrayez Alright, makes sense. Thanks for clarification :relaxed:

jonbonazza commented 3 years ago

Noting for posterity that @Calinou found several issues with the JSON facilities not conforming to the spec. He purposely left these unit tests out of the PR to not confuse people, but I created a proposal to address this: https://github.com/godotengine/godot-proposals/issues/1833

Gorgonx7 commented 3 years ago

Hey if you don't mind I'm gonna take a crack at testing geometry 3D, it's also my first time contributing too :D

Xrayez commented 3 years ago

I had to write initial test suite for RandomNumberGenerator in #44089 with test cases for reproducibility, but not all public methods are currently tested.

rmarco3e8 commented 3 years ago

Hello, I'm also looking to start contributing and am writing tests for Path2D. If someone has already been working on that class, let me know. Thanks for the resources dropped earlier. I'll be sure to ask any questions in the Freenode channel.

cabinboy1031 commented 3 years ago

I am going to see about writing unit tests for the Plane class. Though this is my first contribution to anything in Github. If someone more experienced than me ends up wanting to do the unit tests go ahead as mine will probably not be the best and will most likely need to be improved after i am done with it anyways.

ghost commented 3 years ago

Hey @Calinou, I added some unit test cases for the Random Number Generator class in [#44350 ]. Also, I would like to remove my name and @singher from contributors to the Node class

rmarco3e8 commented 3 years ago

Hey, I opened another PR for Object with a few more test cases in #44387.

a-ivanov commented 3 years ago

Hi there! I want to make my first time contribution to the project by writing unit tests for marshalls.h functions. This one has not been covered yet, has it?

Calinou commented 3 years ago

Hi there! I want to make my first time contribution to the project by writing unit tests for marshalls.h functions. This one has not been covered yet, has it?

Nobody has started work on testing Marshalls yet, but I haven't been able to access that class from C++ since it's mostly intended to be used from GDScript. It might not be easy to test.

Xrayez commented 3 years ago

I think it should be possible to test in either case, because core singletons are initialized in the test environment, I don't know exactly what might be a limitation for doing so.

For _Marshalls in core_bind.h you may just need to use _Marshalls::get_singleton()->variant_to_base64()... etc, but again not sure whether bind classes have to be tested currently, I think it may be better to test core stuff, which may be even easier to do if core doesn't rely on ClassDB functionality (those classes which expand GDCLASS macro), and just rely on pure data manipulation, which should be the case for marshalls I presume.

a-ivanov commented 3 years ago

Nobody has started work on testing Marshalls yet, but I haven't been able to access that class from C++ since it's mostly intended to be used from GDScript. It might not be easy to test.

Actually I thought about the strategy suggested by @Xrayez:

  1. start with testing global functions in marshalls.h: encode_uint16, encode_uint32, etc. - first PR,
  2. test _Marshalls class (need additional research on how to approach it, start with what @Xrayez wrote) - another PR.

Anyway, gonna ask you details later here/in Freenode channel.

Thanks.

BrooklynWelsh commented 3 years ago

Hello everyone, interested in making my first contribution with tests for the Node class. I've written a couple simple tests so far, have a question or two for the Freenode channel, but going well so far. Just wanted to comment to make sure it's not being worked on.

Xrayez commented 3 years ago

Since we talk about Node classes, I think it's also possible to simulate _process() calls with node->notification(NOTIFICATION_PROCESS), or even propagate_notification(), for those classes which are supposed to be added to the scene tree (most of them)... Process notification are done in a main loop I presume, and current test environment has no SceneTree instantiated. It may also be possible to instantiate SceneTree manually (and this will likely reveal some hidden bugs).

Those are just some considerations if someone hadn't have much luck with testing Node derived classes yet...

BrooklynWelsh commented 3 years ago

@Xrayez Thanks for mentioning that! I've been meaning to ask more questions in the IRC chat but have been busy the past couple days.

I have indeed been having problems testing the functions which require SceneTree.

You mention instantiating the SceneTree manually, could you tell me what the correct way is of doing that? During my search on how to add a Node to a SceneTree, I tried a few methods of instantiating one manually but all gave me errors, with the stack trace leading to an error with RenderingServer in a file somewhere (it's been a couple days and I can't remember where).

Xrayez commented 3 years ago

Rendering is disabled in the test environment (RenderingServer is not instantiated at all currently) so likely SceneTree depends on rendering functionality in some way (Viewport?), leading to crash. If you look at changes in #40980, you'll see that I've added some checks in different parts of the engine that were dependent on rendering code, so something similar needs to be done to fix this in the SceneTree or other relevant classes.

This kind of a fix would only work for really simple limitations, otherwise we may need to instantiate RenderingServer functionality eventually, yet the idea behind unit testing is that we should only use what's needed... See also godotengine/godot-proposals#1760.

That said, unless you'd like to fix those issues yourself, the only option for now is to use node->propagate_notification(NOTIFICATION_PROCESS) and alike...

a-ivanov commented 3 years ago

Hey, everyone! Gonna add a few more tests for Object class.

Hyperion101010 commented 3 years ago

@Calinou I would like to work on Curve2D test, I am a new contributor with experience writting C++ code.

Calinou commented 3 years ago

@Hyperion101010 Sure, go ahead :slightly_smiling_face:

a-ivanov commented 3 years ago

Do you think it makes sense to try to write unit tests for Mono-related classes like CSharpLanguage, CSharpScript, etc.?

Calinou commented 3 years ago

Do you think it makes sense to try to write unit tests for Mono-related classes like CSharpLanguage, CSharpScript, etc.?

Sure, as long as these tests work when Mono is disabled (or aren't compiled when Mono is disabled).

Xrayez commented 3 years ago

You can look at GDNative tests for reference (which is a module): https://github.com/godotengine/godot/tree/master/modules/gdnative/tests

Sure, as long as these tests work when Mono is disabled (or aren't compiled when Mono is disabled).

I think this shouldn't be a problem since it's for development purposes, but I don't recall whether those module-specific tests are compiled or not when disabled.

OverloadedOrama commented 3 years ago

Hello. I'd like to try to write tests for the Image class. I've already cloned the repo and started writing some tests for image creation, file saving and loading and drawing pixels (with set_pixel()) and everything seems to be working fine so far. My main issue is that I am very new to C++ and writing unit tests in general, so I might make mistakes, and I'm not entirely sure what kind of tests each class needs. Should I add as many as I possibly can think of and then open a PR?

Xrayez commented 3 years ago

@OverloadedOrama you should strive good coverage when writing unit tests. It means that you can focus on testing every public method available in the Image class. You can also go the "use case" way (find a typical, real-world usage of the class, but which would be simple enough to implement).

I'd say the Godot's source code has a lot of examples to learn from.

Calinou commented 3 years ago

@OverloadedOrama @Xrayez I tried to test Image but got a crash due to pthreads whenever I loaded a PNG or JPEG image from a buffer…

OverloadedOrama commented 3 years ago

@OverloadedOrama @Xrayez I tried to test Image but got a crash due to pthreads whenever I loaded a PNG or JPEG image from a buffer…

@Calinou @Xrayez I gave loading images from buffers a try as well, and they seem to be running fine in my local Windows 10 machine. All successes, no crashes. The rest of the tests seem also to be running fine, and I'll try to add more before opening a PR.

sps1112 commented 3 years ago

Hey everyone, i would like to write tests for the Variant class. The class seems to already have some tests. What other tests should it have? Should i write tests for every combination of data-types? This is also my first time contributing to an organisation but i have some experience writing C++ code.

Calinou commented 3 years ago

What other tests should it have? Should i write tests for every combination of data-types?

As I said in the first post, I think it's important to test all the operator overloads on as many Variant types as we can.

sps1112 commented 3 years ago

@Calinou Sure. I will try to check as many combinations as possible.

angad-k commented 3 years ago

I'd like to try my hand at writing some of the unit tests... I'd like to write tests for PathFollow2D but would require some pointers on where to start from and what all to check?

Calinou commented 3 years ago

I'd like to try my hand at writing some of the unit tests... I'd like to write tests for PathFollow2D but would require some pointers on where to start from and what all to check?

I think the PathFollow2D test should:

angad-k commented 3 years ago

@Calinou thanks a lot for explaining! I'll start working on this from tomorrow and get back to you if I face any problem.

angad-k commented 3 years ago

@Calinou next I'd like to work on PathFollow3D. I guess the flow would overall be similar to PathFollow2D except for the addition of one more dimension, correct?

Calinou commented 3 years ago

@angad-k Indeed, it should be pretty similar.

angad-k commented 3 years ago

Okay. Will send a PR in a day or two...

JoseTMonagas commented 3 years ago

Hi, I want to contribute, this would be my first contribution in Godot and in open source projects in general so I may stumble around a bit, I want to work on the unit tests for Vector, I'm guessing that means Vector2 and Vector3 in core/math, right? Anything I should keep in mind?

Calinou commented 3 years ago

Hi, I want to contribute, this would be my first contribution in Godot and in open source projects in general so I may stumble around a bit, I want to work on the unit tests for Vector, I'm guessing that means Vector2 and Vector3 in core/math, right? Anything I should keep in mind?

There was a small test suite for Vector2 and Vector3 in the legacy test_math.cpp file (this file isn't run in the current test suite). However, as you can see, it's not quite complete and it's unorganized. Therefore, I recommend starting from scratch at this point.

In OP, I also referred to the Vector datatype (an array-like structure), but this is not the same as Vector2 and Vector3. Nonetheless, tests for Vector, Vector2 and Vector3 are all welcome :slightly_smiling_face:

JoseTMonagas commented 3 years ago

Great, I started working on it, at least for Vector2, and also as mentioned by @Xrayez here:

it's better to create separate PRs for each class, this eases the review process → potentially speeds up the merging process.

As Vector2 and Vector3 are in different files, should I make different PRs for them? The same would be for the Vector datatype which I'm assuming is the one located in core/templates/vector.h, right?

Xrayez commented 3 years ago

@JoseTMonagas I think the rule of thumb is when you see a source file, it can have a corresponding test suite, so yeah makes sense to create separate PRs for Vector2/Vector3.

resul4e commented 3 years ago

@Calinou Could you maybe check Geometry2D because I thought there were no tests yet for this class, but it seems that @mbrlabs has already added tests in #44274.

mbrlabs commented 3 years ago

@resul4e yeah that was me, but i didn't implement tests for all methods in there. Some are kind of hard/impossible to test, but there are still some you can test. It probably wouldn't hurt to test methods which are not exposed to GDScript as well.

resul4e commented 3 years ago

@mbrlabs Ah okay good to know. However, as this is my first unit test, I'd like to start with something simpler.

resul4e commented 3 years ago

If it is all right, I would like to take a crack at testing the Bitmap class.