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

Player testing: Add Player Testing framework #332

Merged
merged 22 commits into from
Dec 13, 2023

Conversation

rolandkakonyi
Copy link
Contributor

@rolandkakonyi rolandkakonyi commented Nov 24, 2023

Description

To enable writing Player Tests, we need to introduce a framework that enables writing such tests without much boilerplate.

Changes

I introduced a set of APIs in ./playertesting to enable writing player tests. See available APIs here.
The test host app was extended with index.test.js as the entrypoint for the tests.
There is a single test right now in exampleSpec.ts.

More tests will be added in current sprint!

Tests can be run via the following commands:

  • Run on iOS:

    yarn integration-test test:ios
  • Run on Android:

    yarn integration-test test:android

(Instructions are shown and logged to the console when simply running the app)

Checklist

  • 🗒 CHANGELOG entry

@rolandkakonyi rolandkakonyi self-assigned this Nov 24, 2023
@rolandkakonyi rolandkakonyi changed the title Add Player Testing framework Player testing: Add Player Testing framework Nov 24, 2023
@rolandkakonyi rolandkakonyi marked this pull request as ready for review November 27, 2023 09:54
@@ -1,4 +1,5 @@
{
"name": "IntegrationTest",
"displayName": "IntegrationTest"
"displayName": "IntegrationTest",
"licenseKey": "ENTER_LICENSE_KEY"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this new key here, to enable setting the license key easily for tests.
This can be later used for CI as well.

Copy link
Contributor

@matamegger matamegger left a comment

Choose a reason for hiding this comment

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

Pretty neat, I have some suggestions, but generally well done 💪

Comment on lines 2 to 5
const uniqueId = new Date(Math.ceil(Math.random() * 1e13))
.valueOf()
.toString(36);
return uniqueId;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look like proper UUID generation 😄
I guess though we should have no problems in the testing with that. Still we might consider using an dependency for handling this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 150bc5e

Comment on lines 152 to 155
export class S extends EventSequenceExpectation {}
export class B extends EventBagExpectation {}
export class R extends RepeatedEventExpectation {}
export class A extends AnyEventExpectation {}
Copy link
Contributor

Choose a reason for hiding this comment

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

While I do like such a concept in the case of people who handle the framework on a daily basis, I think having such alias makes it quite hard for contributors to grasp what is happening, and may slow them down when writing own tests.

I am fine though with an alias that's more descriptive e.g. S -> EventSequence (dropping the `Expectationz)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in fe5f4ea


export class S extends EventSequenceExpectation {}
export class B extends EventBagExpectation {}
export class R extends RepeatedEventExpectation {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export class R extends RepeatedEventExpectation {}
export function RepeatedEvent(
singleExpectationConvertible: SingleEventExpectation | EventType,
count: number
): EventSequenceExpectation {
return new RepeatedEventExpectation(singleExpectationConvertible, count);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in fe5f4ea

Copy link
Contributor Author

@rolandkakonyi rolandkakonyi Nov 28, 2023

Choose a reason for hiding this comment

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

added variadic parameters for easier usage in a4c62d7 and 9cf03b2

Comment on lines 1 to 17
import { TestScope } from 'cavy';
import { loadSourceConfig, playFor, startPlayerTest } from '../playertesting';
import { SourceType } from 'bitmovin-player-react-native';

export default (spec: TestScope) => {
spec.describe('cavy', () => {
spec.it('works', async () => {
await new Promise((resolve) => setTimeout(resolve, 1000));
spec.describe('player', () => {
spec.it('loads source and plays for 5 seconds', async () => {
await startPlayerTest({}, async () => {
await loadSourceConfig({
url: 'https://bitmovin-a.akamaihd.net/content/MI201109210084_1/m3u8s/f08e80da-bf1d-4e3d-8899-f0f6155f6efa.m3u8',
type: SourceType.HLS,
});
await playFor(5);
});
});
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, the test is not a good example to showcase. Here is a suggestion.. if applied please also update the markdown file with one of these examples.

Suggested change
import { TestScope } from 'cavy';
import { loadSourceConfig, playFor, startPlayerTest } from '../playertesting';
import { SourceType } from 'bitmovin-player-react-native';
export default (spec: TestScope) => {
spec.describe('cavy', () => {
spec.it('works', async () => {
await new Promise((resolve) => setTimeout(resolve, 1000));
spec.describe('player', () => {
spec.it('loads source and plays for 5 seconds', async () => {
await startPlayerTest({}, async () => {
await loadSourceConfig({
url: 'https://bitmovin-a.akamaihd.net/content/MI201109210084_1/m3u8s/f08e80da-bf1d-4e3d-8899-f0f6155f6efa.m3u8',
type: SourceType.HLS,
});
await playFor(5);
});
});
});
};
import { TestScope } from 'cavy';
import {
callPlayerAndExpectEvents,
EventSequence,
EventType,
expectEvents,
loadSourceConfig,
RepeatedEvent,
startPlayerTest,
} from '../playertesting';
import { SourceType } from 'bitmovin-player-react-native';
export default (spec: TestScope) => {
spec.describe('calling play when a source is loaded', () => {
spec.it('emits a Play and Playing event', async () => {
await startPlayerTest({}, async () => {
await loadSourceConfig({
url: 'https://bitmovin-a.akamaihd.net/content/MI201109210084_1/m3u8s/f08e80da-bf1d-4e3d-8899-f0f6155f6efa.m3u8',
type: SourceType.HLS,
});
await callPlayerAndExpectEvents((player) => {
player.play();
}, EventSequence(EventType.Play, EventType.Playing));
});
});
});
spec.describe('playing a source', () => {
spec.it('emits TimeChanged events', async () => {
await startPlayerTest({}, async () => {
await loadSourceConfig({
url: 'https://bitmovin-a.akamaihd.net/content/MI201109210084_1/m3u8s/f08e80da-bf1d-4e3d-8899-f0f6155f6efa.m3u8',
type: SourceType.HLS,
});
await callPlayerAndExpectEvents((player) => {
player.play();
}, EventSequence(EventType.Play, EventType.Playing));
await expectEvents(RepeatedEvent(EventType.TimeChanged, 5));
});
});
});
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added your changes in 23323d3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also updated CONTRIBUTING.md here 3742d68

await new Promise((resolve) => setTimeout(resolve, 1000));
spec.describe('player', () => {
spec.it('loads source and plays for 5 seconds', async () => {
await startPlayerTest({}, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunate that we can not get rid of those await keywords in front of (basically) every line of the test.
At least we discussed that there is no simple/nice way to do this to our knowledge.

Copy link
Contributor

@matamegger matamegger left a comment

Choose a reason for hiding this comment

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

Super 👏

Co-authored-by: Matthias Tamegger <matamegger@users.noreply.github.com>
@rolandkakonyi
Copy link
Contributor Author

rolandkakonyi commented Nov 29, 2023

@matamegger I renamed PlayerWorld to PlayerTestWorld in 0d97cc6 as we discussed before.

Base automatically changed from player-testing/cavy-setup to development December 13, 2023 07:37
@rolandkakonyi rolandkakonyi merged commit 7ed5dbc into development Dec 13, 2023
8 checks passed
@rolandkakonyi rolandkakonyi deleted the player-testing/add-player-testing-framework branch December 13, 2023 07:39
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