Feat: Native SDK Core Unit Tests (Part I, II, Events) #333
Conversation
parag-pv
commented
Oct 3, 2024
parag-pv
commented
- Added unit tests for 3 event methods under accessibility module.
- 6 tests in total. Each method has subscribe and unsubscribe.
|
|
|
MFOS standalone sanity report - CORE,MANAGE,DISCOVERY: |
|
MFOS standalone sanity report - CORE,MANAGE,DISCOVERY: |
ksentak
changed the title
|
MFOS standalone sanity report - CORE,MANAGE,DISCOVERY: |
| if (closedCaptions.styles.has_value()) { | ||
| const auto& styles = closedCaptions.styles.value(); | ||
|
|
||
| if (styles.backgroundColor.has_value()) |
There was a problem hiding this comment.
What if styles.background doesn't have a value? Will this pass or fail? Is there a way to enforce that if our example has "styles.backgroundColor" then the result should and fail if that's not the case? This applies to all other elements as well.
There was a problem hiding this comment.
You're correct that the current implementation does not explicitly enforce that styles.backgroundColor and other fields must exist in both the actual result and the expected values. If the actual result is missing a field while the expected values contain it, the test will simply skip the EXPECT_EQ comparison, which could potentially lead to false positives if the field was expected but not present.
We could maybe do something like this:
if (expectedValues["styles"].contains("backgroundColor")) {
ASSERT_TRUE(styles.backgroundColor.has_value()) << "Expected backgroundColor, but it is missing";
EXPECT_EQ(styles.backgroundColor.value(), expectedValues["styles"]["backgroundColor"]);
}
| { | ||
| // Hardcoded expected values | ||
| Firebolt::Device::AudioProfiles expectedValues; | ||
| expectedValues.stereo = true; |
There was a problem hiding this comment.
Why are we hard-coding the values instead of getting them from the OpenRPC here? I'm assuming it has something to do with the name but if those values ever change this will fail.
|
🎉 This PR is included in version 1.5.0-next.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
