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

test: add test for getMediaKeySystemAccess(); #1603

Open
wants to merge 2 commits into
base: fix/comprehensive-cached-check
Choose a base branch
from

Conversation

Florent-Bouisset
Copy link
Collaborator

add unit test for getMediaKeySystemAccess()

@peaBerberian
Copy link
Collaborator

peaBerberian commented Dec 12, 2024

I think tests for getMediaKeySystemAccess should not be added to __global__/ (but directly inside __tests__/) because __global__ tests basically are integration tests for the ContentDecryptor as a whole, not its inner functions.

The idea of __global__/ was to have a middle ground between:

  • our unit tests, which tested individual functions (not exactly what you have here, but it is still close to root utils)

    The cons for those were that they very often needed to be updated because e.g. the decrypt/ inner logic at the function-level changes very often and they also empirically have been much less useful than integration tests at discovering issues in the RxPlayer: they usually only broke when the thing they do is no more aligned with the code they test.

  • our full-RxPlayer integration tests, which are much more stable (as they just follow our API), but for which specific edge states might be difficult to achieve

So we decided some years ago to have test relying on the module's (in this case the ContentDecryptor) API.
Those API are not part of our semver as they are not exposed to applications, yet because those modules interact with other unrelated modules inside the RxPlayer, they still follow generally a much more stable behavior than those inner functions.
And that's what __global__/ was for: in some sense those are integration tests for the module.


Moreover just to clarify here, I've nothing against unit tests at lower levels, and I think that it's a net good thing to add some, most of all for testing that some code does what we think it does in some edge cases.
But historically we've put much less effort on them when compared with tests testing things at higher abstraction levels because the latter historically have been both more helpful and stable.

@peaBerberian peaBerberian force-pushed the fix/comprehensive-cached-check branch from e5b2cde to 87df748 Compare December 13, 2024 09:54
@Florent-Bouisset Florent-Bouisset force-pushed the test/getMediaKeySystemAccess branch from 4c6ab36 to 0c4e7be Compare December 16, 2024 15:19
}),
}),
getConfiguration: () => {
return baseEmeConfiguration;
Copy link
Collaborator

@peaBerberian peaBerberian Dec 16, 2024

Choose a reason for hiding this comment

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

⚠️ we cannot in many cases just set the asked configuration as the return value of getConfiguration

e.g. distinctiveIdentifier is returned as optional, something that it cannot be:

All values in the configuration may be used in any combination. Members of type MediaKeysRequirement reflect whether the capability is required for any combination. They will not have the value "optional".

Anyway we don't care for this test and this is a nitpick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants