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 some tests for uid:// encoding and decoding #100970

Conversation

anknetau
Copy link
Contributor

I have added some basic uid encoding tests. See also #100969 for some details on the corner cases of our current implementation, documented in the test cases here.

@AThousandShips
Copy link
Member

Can be rebased on top of #100976 and tweaked to have the same test suite (I think it deserves its own one), with the uid://a case removed as it is already covered, feel free to rework any tests as you see fit

@anknetau anknetau force-pushed the ank/godot-add-test-cases-for-uid-strings branch from 4eb41a4 to abb2796 Compare January 7, 2025 06:25
@anknetau
Copy link
Contributor Author

anknetau commented Jan 7, 2025

@AThousandShips I have updated the tests and rebased on top of your commits now - let me know what you think!

@anknetau anknetau force-pushed the ank/godot-add-test-cases-for-uid-strings branch from abb2796 to 11ae44b Compare January 7, 2025 06:30
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

Unsure if the rest for leading a should be included, it might be considered an implementation detail, and it just shows that irregular input is handled gracefully

But it can be removed if we choose to sanitize UID decoding in the future

@anknetau
Copy link
Contributor Author

LGTM

Unsure if the rest for leading a should be included, it might be considered an implementation detail, and it just shows that irregular input is handled gracefully

But it can be removed if we choose to sanitize UID decoding in the future

Thanks! I don't seem to have merge permissions - do we need someone else to press that button?

@AThousandShips
Copy link
Member

The production team handles merges, you shouldn't do that on your own (and there should be at least one more approval as well from relevant teams)

@AThousandShips AThousandShips requested a review from a team January 10, 2025 11:53
@anknetau anknetau force-pushed the ank/godot-add-test-cases-for-uid-strings branch from 11ae44b to e5e0fb1 Compare January 10, 2025 12:39
@anknetau anknetau force-pushed the ank/godot-add-test-cases-for-uid-strings branch from e5e0fb1 to 2b5dd99 Compare January 10, 2025 12:41
@akien-mga akien-mga changed the title Add some tests for uid:// encoding and decoding Add some tests for uid:// encoding and decoding Jan 10, 2025
@akien-mga akien-mga merged commit 35080c6 into godotengine:master Jan 10, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

5 participants