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

Add unit tests for StreamPeer and StreamPeerBuffer #95784

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Add unit tests for StreamPeer and StreamPeerBuffer #95784

merged 1 commit into from
Oct 1, 2024

Conversation

pafuent
Copy link
Contributor

@pafuent pafuent commented Aug 19, 2024 •
edited
Loading

This PR aims to help "fix" #43440

Adding unit tests for StreamPeer and StreamPeerBuffer

@pafuent pafuent requested review from a team as code owners August 19, 2024 01:48
@pafuent pafuent mentioned this pull request Aug 19, 2024
Open
99 tasks
@AThousandShips AThousandShips changed the title Fixing StreamPeer::get_string and StreamPeer::get_utf8_string bug (#95783) Fix StreamPeer::get_string and StreamPeer::get_utf8_string bug Aug 19, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Aug 19, 2024
@pafuent
Copy link
Contributor Author

pafuent commented Aug 20, 2024

Because the discussion around if the bug referenced by this PR is a bug or not, I can extract all the other unit test in another PR to get that one move faster and get merged. Thoughts?

@pafuent pafuent changed the title Fix StreamPeer::get_string and StreamPeer::get_utf8_string bug Add unit tests for StreamPeer and StreamPeerBuffer Sep 7, 2024
@pafuent
Copy link
Contributor Author

pafuent commented Sep 7, 2024

I decided to remove #95783 fix and keep this PR as only adding tests

@pafuent
Copy link
Contributor Author

pafuent commented Sep 7, 2024

@timothyqiu Could you please remove breaks compat label?
The PR only add tests now.

@pafuent
Copy link
Contributor Author

pafuent commented Sep 9, 2024

@Calinou Can I get a review now that the PR is only adding Unit Tests?

@AThousandShips
Copy link
Member

You should use Ref here as well

Geometror
Geometror reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

To be honest, I wasn't sure about the separation of the StreamPeer and StreemPeerBuffer tests, but after looking into this a bit longer I now think it makes sense. Maybe adding a StreamPeerDummy/Mock would be the most correct way to test them apart, but I'd leave it up to you if you want to do that.

Either way, needs to be squashed into one commit.

AThousandShips
AThousandShips reviewed Sep 23, 2024
View reviewed changes
tests/core/io/test_stream_peer_buffer.h Outdated Show resolved Hide resolved
tests/core/io/test_stream_peer_buffer.h Outdated Show resolved Hide resolved
tests/core/io/test_stream_peer.h Outdated Show resolved Hide resolved
@pafuent pafuent force-pushed the adding_stream_peer_buffer_tests branch from 6262e71 to adab6da Compare September 23, 2024 19:56
@pafuent
Copy link
Contributor Author

pafuent commented Sep 23, 2024

@Geometror

To be honest, I wasn't sure about the separation of the StreamPeer and StreemPeerBuffer tests, but after looking into this a bit longer I now think it makes sense. Maybe adding a StreamPeerDummy/Mock would be the most correct way to test them apart, but I'd leave it up to you if you want to do that.

In order to add a mock I prefer to wait until a definition on #95224 where I added a Mocking library. If I get that PR merged with the mocking library I will be glad to revisit this tests and use a mock.

@pafuent pafuent mentioned this pull request Sep 30, 2024
Open
AThousandShips
AThousandShips approved these changes Oct 1, 2024
View reviewed changes
tests/core/io/test_stream_peer.h Outdated Show resolved Hide resolved
@pafuent pafuent force-pushed the adding_stream_peer_buffer_tests branch from adab6da to ff10dee Compare October 1, 2024 15:25
@akien-mga akien-mga merged commit d71d954 into godotengine:master Oct 1, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 1, 2024
@jamie-pate jamie-pate mentioned this pull request Nov 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geometror Geometror Geometror left review comments

@AThousandShips AThousandShips AThousandShips approved these changes

@Calinou Calinou Calinou approved these changes

Assignees
No one assigned
Labels
enhancement topic:core topic:tests
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@pafuent @AThousandShips @akien-mga @Calinou @Geometror @timothyqiu