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.

Sign up for GitHub

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

Jump to bottom

unit tests for all public methods in the sprite frame class #57742

Merged
merged 1 commit into from
Aug 4, 2022
Merged

unit tests for all public methods in the sprite frame class #57742

merged 1 commit into from
Aug 4, 2022

Conversation

im-deepfriedwater
Copy link
Contributor

Open to any feedback! Covered many input cases along with expected behaviors when bad input is provided to functions.

@im-deepfriedwater im-deepfriedwater requested a review from a team as a code owner February 7, 2022 08:23
@im-deepfriedwater im-deepfriedwater mentioned this pull request Feb 7, 2022
Open
99 tasks
@im-deepfriedwater
Copy link
Contributor Author

im-deepfriedwater commented Feb 7, 2022 •
edited
Loading

op noticed right after I put this up I accidently removed a test suite entry, adding it back and rebasing! EDIT: done!

@Chaosus Chaosus added this to the 4.0 milestone Feb 7, 2022
@im-deepfriedwater im-deepfriedwater force-pushed the im-deepfriedwater/sprite_frames_tests branch 2 times, most recently from 4a54bed to cda9023 Compare February 7, 2022 19:40
@im-deepfriedwater
Copy link
Contributor Author

ran clang-format locally to fix workflow errors

@im-deepfriedwater
Copy link
Contributor Author

hey sorry I've left this hanging for a while, going back to clean this up and try to get these merged

@akien-mga akien-mga force-pushed the im-deepfriedwater/sprite_frames_tests branch from cda9023 to 667faa6 Compare August 4, 2022 12:27
akien-mga
akien-mga approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for the delay reviewing this.

This looks good 👍

I force pushed an update to fix some style issues (we use snake case for identifiers, and Godot Objects shouldn't be new'ed but memnew()'ed (or here .instantiate()d).
Also fixed the list getters checks which were not checking their results properly.

@akien-mga akien-mga merged commit 9f408ae into godotengine:master Aug 4, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@im-deepfriedwater
Copy link
Contributor Author

no worries! sorry I missed the merged notification LOL, that's awesome! thanks for the feedback, hope I can make some other contributions as well!

@im-deepfriedwater im-deepfriedwater deleted the im-deepfriedwater/sprite_frames_tests branch September 8, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:tests topic:2d
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@im-deepfriedwater @akien-mga @Chaosus