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
76 of 99 tasks
Calinou opened this issue Nov 10, 2020 · 206 comments
Open
76 of 99 tasks

[TRACKER] Unit tests to add or improve #43440

Calinou opened this issue Nov 10, 2020 · 206 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...

@CampioniMan
Copy link

CampioniMan commented Nov 10, 2024

Hello, I am going to create some unit tests for WebSocketPeer!
I am a new contributor with experience writting C++ code.

pafuent added a commit to pafuent/godot that referenced this issue Nov 12, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 12, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 12, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 13, 2024
@NeuralTide
Copy link

I want to work on my first contribution. I will start working on adding tests for Parallax2D.

ChrisBase pushed a commit to ChrisBase/godot that referenced this issue Nov 15, 2024
ChrisBase pushed a commit to ChrisBase/godot that referenced this issue Nov 15, 2024
ChrisBase pushed a commit to ChrisBase/godot that referenced this issue Nov 15, 2024
This PR aims to help "fix" godotengine#43440

Also fixing a small typo on `SceneMultiplayer` docs.
@carsonetb
Copy link
Contributor

I would like to do tests for OccluderInstance3D for my first contribution. Thanks!

carsonetb added a commit to carsonetb/godot that referenced this issue Nov 17, 2024
carsonetb added a commit to carsonetb/godot that referenced this issue Nov 18, 2024
@JustinBraben
Copy link

I wish to work on my first contribution. I can work on FontFile.

@qwertychomp
Copy link
Contributor

I want to make tests for Occluder3D and all the occluder shapes (BoxOccluder3D, SphereOccuder3D, etc).

pafuent added a commit to pafuent/godot that referenced this issue Nov 20, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 20, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 20, 2024
@JustinBraben
Copy link

Closed #99406 since #99131 was merged in. May I work on FontVariation instead?

pafuent added a commit to pafuent/godot that referenced this issue Nov 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 22, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 23, 2024
pafuent added a commit to pafuent/godot that referenced this issue Nov 23, 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