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

Add unit test for VisualShader #70396

Merged
merged 1 commit into from
Dec 26, 2022

Conversation

jainl28patel
Copy link
Contributor

part of #43440
Adds test for VisualShader class test the following

  • Parameter with getter and setter
  • Node Creation, deletion, editing, replacing and removing
  • Varyings
  • Invalid Entries for above three

Tests are compiled and run successfully on my device

@jainl28patel jainl28patel requested a review from a team as a code owner December 21, 2022 11:07
@akien-mga akien-mga requested a review from a team December 21, 2022 11:19
@akien-mga akien-mga added this to the 4.0 milestone Dec 21, 2022
tests/scene/test_visual_shader.h Outdated Show resolved Hide resolved
tests/scene/test_visual_shader.h Outdated Show resolved Hide resolved
@Chaosus
Copy link
Member

Chaosus commented Dec 21, 2022

You need to squash the commits as required by our pipeline (see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#updating-your-branch)

@jainl28patel jainl28patel force-pushed the VisualShader-unit-test branch from cf87467 to 10fe08c Compare December 21, 2022 13:42
@jainl28patel jainl28patel force-pushed the VisualShader-unit-test branch 2 times, most recently from 2ecc7c8 to e85ee7e Compare December 21, 2022 17:53
@jainl28patel jainl28patel requested a review from Maran23 December 22, 2022 14:02
@Maran23
Copy link
Contributor

Maran23 commented Dec 23, 2022

I wonder if we can also test the generate_code method?

@jainl28patel
Copy link
Contributor Author

I wonder if we can also test the generate_code method?

Yeah sure. Actually I didn't tested it because it wasn't the part of VisualShader. I will make changes by today/tomorrow 👍 .
If there are any other changes please let me know.
Also is there a way to run the Static checks on my system before I make commit so that i have cleaner commits.

@jainl28patel
Copy link
Contributor Author

I saw the code and I feel that generate_code method should not be checked as a part of VisualShader class as it belongs to a different class.
Is it necessary to test it in VisualShader ?

@jainl28patel jainl28patel force-pushed the VisualShader-unit-test branch from e85ee7e to e9634d3 Compare December 24, 2022 07:53
@jainl28patel jainl28patel requested review from Chaosus and Maran23 and removed request for Maran23 and Chaosus December 24, 2022 07:57
@jainl28patel
Copy link
Contributor Author

@Chaosus please review if any other changes are needed.

Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

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

Ok, thanks for contribution, I think it's fine now.

@jainl28patel
Copy link
Contributor Author

jainl28patel commented Dec 26, 2022

Ok, thanks for contribution, I think it's fine now.

Thankyou very much for guiding as it was my 1st experience and was amazing 🙏 .
Need one last help. Why is Linux build and MacOS failing ? It didn't failed last 2 times and i only changed the for{} loop.

@Chaosus
Copy link
Member

Chaosus commented Dec 26, 2022

Why is Linux build and MacOS failing ? It didn't failed last 2 times and i only changed the for{} loop.

I don't know, but my guess, you should try to declare the variables outside of SUBCASE labels (at the beginning of CASE).

@jainl28patel
Copy link
Contributor Author

Why is Linux build and MacOS failing ? It didn't failed last 2 times and i only changed the for{} loop.

I don't know, but my guess, you should try to declare the variables outside of SUBCASE labels (at the beginning of CASE).

I was doing that before but didn't gave any ERRORS. Can it happen because I am deleting the object pointer every time in the for loop using memdelete(vsn.ptr()) ?

@Chaosus
Copy link
Member

Chaosus commented Dec 26, 2022

I was doing that before but didn't gave any ERRORS. Can it happen because I am deleting the object pointer every time in the for loop using memdelete(vsn.ptr()) ?

Ah yes, you should not call memdelete on Ref counted objects - they would automatically free the memory when goes out of scope.

tests/scene/test_visual_shader.h Outdated Show resolved Hide resolved
tests/scene/test_visual_shader.h Outdated Show resolved Hide resolved
tests/scene/test_visual_shader.h Outdated Show resolved Hide resolved
@jainl28patel jainl28patel force-pushed the VisualShader-unit-test branch from f1f7b22 to 453ab2a Compare December 26, 2022 10:52
@jainl28patel jainl28patel force-pushed the VisualShader-unit-test branch from 453ab2a to e4f4fb5 Compare December 26, 2022 11:23
@Chaosus Chaosus merged commit b6e0603 into godotengine:master Dec 26, 2022
@Chaosus
Copy link
Member

Chaosus commented Dec 26, 2022

Thanks!

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.

4 participants