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

[TRACKER] Unit tests to add or improve #43440

Open
73 of 99 tasks
Calinou opened this issue Nov 10, 2020 · 198 comments
Open
73 of 99 tasks

[TRACKER] Unit tests to add or improve #43440

Calinou opened this issue Nov 10, 2020 · 198 comments

Comments

@Calinou
Copy link
Member

Calinou commented Nov 10, 2020

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.

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.

  • CubeMap: Not testable without RenderingServer access.
  • Shader: Not testable without RenderingServer access.
@ghost
Copy link

ghost commented Nov 10, 2020

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
Copy link
Member Author

Calinou commented Nov 10, 2020

@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
Copy link

ghost commented Nov 10, 2020

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

@ghost
Copy link

ghost commented Nov 11, 2020

@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
Copy link
Contributor

Xrayez commented Nov 11, 2020

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.

@maxmutant
Copy link
Contributor

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
Copy link
Contributor

Xrayez commented Nov 11, 2020

@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.

@maxmutant
Copy link
Contributor

@Xrayez Alright, makes sense. Thanks for clarification ☺️

@jonbonazza
Copy link
Contributor

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: godotengine/godot-proposals#1833

@Gorgonx7
Copy link
Contributor

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
Copy link
Contributor

Xrayez commented Dec 8, 2020

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
Copy link

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
Copy link
Contributor

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
Copy link

ghost commented Dec 13, 2020

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
Copy link

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

@a-ivanov
Copy link
Contributor

a-ivanov commented Dec 24, 2020

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
Copy link
Member Author

Calinou commented Dec 24, 2020

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
Copy link
Contributor

Xrayez commented Dec 25, 2020

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

Xrayez commented Jan 7, 2021

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...

pafuent added a commit to pafuent/godot that referenced this issue Sep 21, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 21, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 22, 2024
@pafuent
Copy link
Contributor

pafuent commented Sep 23, 2024

@Calinou Can I get some reviews on Unit Tests PRs?
#95590
#95784
#95931
#97262
#97311

pafuent added a commit to pafuent/godot that referenced this issue Sep 23, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 23, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 25, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 25, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 25, 2024
Gurka2 added a commit to Gurka2/godot that referenced this issue Sep 27, 2024
The tests test the class getters and setters and the function get aabb.
This is what was asked in the issue godotengine#43440.
pafuent added a commit to pafuent/godot that referenced this issue Sep 28, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 29, 2024
This PR aims to help "fix" godotengine#43440

Also fixing a small typo on `SceneMultiplayer` docs.
pafuent added a commit to pafuent/godot that referenced this issue Sep 29, 2024
This PR aims to help "fix" godotengine#43440

Also fixing a small typo on `SceneMultiplayer` docs.
pafuent added a commit to pafuent/godot that referenced this issue Sep 30, 2024
This PR aims to help "fix" godotengine#43440

Also fixing a small typo on `SceneMultiplayer` docs.
pafuent added a commit to pafuent/godot that referenced this issue Sep 30, 2024
pafuent added a commit to pafuent/godot that referenced this issue Sep 30, 2024
pafuent added a commit to pafuent/godot that referenced this issue Oct 1, 2024
pafuent added a commit to pafuent/godot that referenced this issue Oct 1, 2024
pafuent added a commit to pafuent/godot that referenced this issue Oct 1, 2024
@adamscott adamscott reopened this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

No branches or pull requests