Skip to content

Add unit tests for AudioStreamInteractive #105457

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

Closed
wants to merge 3 commits into from

Conversation

michelleboisvert
Copy link

Contributions to Issue #43440
Basic unit tests to verify the constructor, some getters, and some setters of the AudioStreamInteractive, AudioStreamPlaybackInteractive, AudioStreamSynchronized, and AudioStreamPlaylist class.

@@ -502,6 +502,9 @@ else:
# Disable assert() for production targets (only used in thirdparty code).
env.Append(CPPDEFINES=["NDEBUG"])

# adding the line below to remove the error in the control.h file (claimed it could not find core/math)
env.Append(CPPPATH=['#core'])
Copy link
Member

@AThousandShips AThousandShips Apr 16, 2025

Choose a reason for hiding this comment

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

This should be solved differently, this sounds like an issue either in your compilation setup or something related to your tests, this is not the way to fix it

I suspect that you tried to build from inside test/ which is not correct

But regardless of this the fix should be a separate PR if it is needed, this PR shouldn't contain a buildystem fix

Copy link
Author

Choose a reason for hiding this comment

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

I see, on the writing unit test site it said that the tests would fall under tests/ so I got confused, but I can fix that issue. Thank you for pointing me to a helpful file, and I see the formatting is a little different than those in the tests/ folder. Is the one for jsonrpc the expected unit tests?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean for formatting? And what file? Did you mean to reply to my other comment?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I meant to reply to your other comment apologies.

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.

Thank you for your contribution

However see above for the issue with making changes to the buildsystem

Also these tests should be in the module, not in tests/, see modules/jsonrpc for an example of how to set up such a test

@michelleboisvert michelleboisvert requested a review from a team as a code owner April 16, 2025 19:37
@AThousandShips AThousandShips removed this from the 4.x milestone Apr 17, 2025
@Calinou
Copy link
Member

Calinou commented Apr 17, 2025

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