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

Improve player testing setup #350

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Improve player testing setup #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d977d88
chore: improve running from the command line
rolandkakonyi Dec 7, 2023
2e6fc90
chore: reduce possible race-conditions when running tests and calling…
rolandkakonyi Dec 7, 2023
d3643a6
chore: Improve code style
rolandkakonyi Dec 13, 2023
e9ee5c1
Merge branch 'development' into improve-player-testing-setup
rolandkakonyi Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions integration_test/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,8 @@ yarn bootstrap
To run the tests, run the following command from the repository root:

```sh
yarn integration-test test:ios # Run tests on iOS
yarn integration-test test:android # Run tests on Android
```

Hint: You can provide a specific iOS simulator by name when using `--simulator` flag. `xcrun simctl list devices available` provides you with a list of available devices in your environment.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now detected automatically


```sh
yarn example ios --simulator="iPhone 14 Pro"
yarn integration-test test:ios # Run tests on iOS simulator
yarn integration-test test:android # Run tests on Android emulator
```

To set the license key to be used for the tests, you can set the key `"licenseKey"` in `integration_test/app.json`.
Expand Down
8 changes: 6 additions & 2 deletions integration_test/ios/IntegrationTest.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,8 @@
CLANG_ENABLE_MODULES = YES;
CURRENT_PROJECT_VERSION = 1;
ENABLE_BITCODE = NO;
"EXCLUDED_ARCHS[sdk=appletvsimulator*]" = i386;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = arm64;
INFOPLIST_FILE = IntegrationTest/Info.plist;
LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
Expand Down Expand Up @@ -312,6 +314,8 @@
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
CLANG_ENABLE_MODULES = YES;
CURRENT_PROJECT_VERSION = 1;
"EXCLUDED_ARCHS[sdk=appletvsimulator*]" = i386;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = arm64;
INFOPLIST_FILE = IntegrationTest/Info.plist;
LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
Expand Down Expand Up @@ -367,7 +371,7 @@
ENABLE_STRICT_OBJC_MSGSEND = YES;
ENABLE_TESTABILITY = YES;
"EXCLUDED_ARCHS[sdk=appletvsimulator*]" = i386;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = arm64;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = i386;
GCC_C_LANGUAGE_STANDARD = gnu99;
GCC_DYNAMIC_NO_PIC = NO;
GCC_NO_COMMON_BLOCKS = YES;
Expand Down Expand Up @@ -447,7 +451,7 @@
ENABLE_NS_ASSERTIONS = NO;
ENABLE_STRICT_OBJC_MSGSEND = YES;
"EXCLUDED_ARCHS[sdk=appletvsimulator*]" = i386;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = arm64;
"EXCLUDED_ARCHS[sdk=iphonesimulator*]" = i386;
GCC_C_LANGUAGE_STANDARD = gnu99;
GCC_NO_COMMON_BLOCKS = YES;
GCC_PREPROCESSOR_DEFINITIONS = (
Expand Down
10 changes: 10 additions & 0 deletions integration_test/ios/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ post_install do |installer|
)
__apply_Xcode_12_5_M1_post_install_workaround(installer)
fix_deployment_target(installer)
fix_simulator_run(installer)
end

def fix_deployment_target(installer)
Expand All @@ -90,3 +91,12 @@ def fix_deployment_target(installer)
end
end
end

def fix_simulator_run(installer)
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64"
config.build_settings["EXCLUDED_ARCHS[sdk=appletvsimulator*]"] = "i386"
end
end
end
2 changes: 1 addition & 1 deletion integration_test/ios/Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,6 @@ SPEC CHECKSUMS:
Yoga: f67f5769ce78049c5fe798bc735f04535c7bc1ac
YogaKit: f782866e155069a2cca2517aafea43200b01fd5a

PODFILE CHECKSUM: aa4d941d9c016574cd5330873d8b6d6a8d7ea421
PODFILE CHECKSUM: a697b54562cb6063433ce9a20f9875a08a45b453

COCOAPODS: 1.14.2
8 changes: 6 additions & 2 deletions integration_test/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
"android": "react-native run-android",
"ios": "react-native run-ios",
"start": "react-native start",
"test:android": "yarn cavy run-android",
"test:ios": "yarn cavy run-ios",
"stop-test:android": "adb shell am force-stop com.bitmovin.player.reactnative.integrationtest",
"stop-test:ios": "xcrun simctl list devices available -e -j | jq --raw-output '.devices.[].[] | select( .state == \"Booted\" and (.deviceTypeIdentifier | contains(\"iPhone\"))).udid' | xargs -I UDID xcrun simctl terminate UDID com.bitmovin.player.reactnative.IntegrationTest 2> /dev/null; exit 0",
Comment on lines +9 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These will stop already running instance of the integration test app to prevent it executing tests while rebuilding.

"start-test:android": "yarn cavy run-android",
"start-test:ios": "yarn cavy run-ios --simulator \"$(xcrun simctl list devices available -e -j | jq --raw-output -s '[.[].devices.[].[] | select( .deviceTypeIdentifier | contains(\"iPhone\") )|.name] | first')\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto-detect iOS simulator to run on

"test:android": "yarn stop-test:android && yarn start-test:android",
"test:ios": "yarn stop-test:ios && yarn start-test:ios",
"pods": "yarn pods-install || yarn pods-update",
"pods-install": "yarn pod-install",
"pods-update": "pod update --silent"
Expand Down
35 changes: 22 additions & 13 deletions integration_test/playertesting/PlayerTestWorld.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default class PlayerTestWorld {
private player_: Player | undefined;
private isFinished_: boolean = false;
private eventListeners: { [key: string]: (event: Event) => void } = {};
private isPlayerInitialized: boolean = false;

static get shared(): PlayerTestWorld {
if (PlayerTestWorld.shared_ === undefined) {
Expand All @@ -42,6 +43,7 @@ export default class PlayerTestWorld {

private set player(player: Player | undefined) {
this.player_ = player;
this.isPlayerInitialized = false;
this.reRender();
}

Expand Down Expand Up @@ -72,10 +74,6 @@ export default class PlayerTestWorld {
player.initialize();
this.player = player;

// Trick to wait for the player to be initialized
// otherwise initial events might be missed
await player.isPlaying();

Comment on lines -75 to -78
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this as it caused missing initial events from the player.
Now this trick is applied on the first interaction with the player.

await fn().finally(() => {
player.destroy();
this.isFinished_ = true;
Expand All @@ -85,7 +83,7 @@ export default class PlayerTestWorld {
};

callPlayer = async <T>(fn: (player: Player) => Promise<T>): Promise<T> => {
return await fn(this.ensurePlayer());
return await fn(await this.ensurePlayer());
};

expectEvent = async <T extends Event>(
Expand All @@ -110,7 +108,7 @@ export default class PlayerTestWorld {
return await this.expectEventCalling<E>(
expectationConvertible,
timeoutSeconds,
() => fn(this.ensurePlayer())
async () => fn(await this.ensurePlayer())
);
};

Expand All @@ -122,7 +120,7 @@ export default class PlayerTestWorld {
return await this.expectEventsCalling(
expectationsConvertible,
timeoutSeconds,
() => fn(this.ensurePlayer())
async () => fn(await this.ensurePlayer())
);
};

Expand All @@ -143,7 +141,8 @@ export default class PlayerTestWorld {
time: number,
timeoutSeconds: number
): Promise<TimeChangedEvent> => {
const currentTime = await this.ensurePlayer().getCurrentTime();
const player = await this.ensurePlayer();
const currentTime = await player.getCurrentTime();
const targetTime = currentTime + time;
return await this.playUntil(targetTime, timeoutSeconds);
};
Expand All @@ -164,13 +163,14 @@ export default class PlayerTestWorld {
);
};

private ensurePlayer = (): Player => {
private ensurePlayer = async (): Promise<Player> => {
if (this.player !== undefined) {
if (this.player.isDestroyed) {
throw new Error(
'Player was destroyed. Did you forget to call "startPlayerTest" again?'
);
}
await this.ensurePlayerInitialized();
return this.player;
}
throw new Error("It seems you forgot to call 'startPlayerTest' first.");
Expand All @@ -186,7 +186,7 @@ export default class PlayerTestWorld {
private expectEventCalling = async <T extends Event>(
expectationConvertible: SingleEventExpectation | EventType,
timeoutSeconds: number,
afterListenerAttached: () => void = () => {}
afterListenerAttached: () => Promise<void> = async () => Promise.resolve()
): Promise<T> => {
let actualExpectation: SingleEventExpectation;
if (expectationConvertible instanceof SingleEventExpectation) {
Expand All @@ -211,7 +211,7 @@ export default class PlayerTestWorld {
reject(new Error(`Expectation was not met: ${actualExpectation}`));
}
}, timeoutSeconds * 1000);
afterListenerAttached();
await afterListenerAttached();
return future.then((event) => {
clearTimeout(timeoutHandle);
removeListener();
Expand All @@ -222,7 +222,7 @@ export default class PlayerTestWorld {
private expectEventsCalling = async (
expectationsConvertible: MultipleEventsExpectation | EventType[],
timeoutSeconds: number,
afterListenerAttached: () => void = () => {}
afterListenerAttached: () => Promise<void> = async () => Promise.resolve()
): Promise<Event[]> => {
let actualExpectation: MultipleEventsExpectation;
if (expectationsConvertible instanceof MultipleEventsExpectation) {
Expand Down Expand Up @@ -259,7 +259,7 @@ export default class PlayerTestWorld {
reject(new Error(`Expectation was not met: ${actualExpectation}`));
}
}, timeoutSeconds * 1000);
afterListenerAttached();
await afterListenerAttached();
return future.then((events) => {
clearTimeout(timeoutHandle);
removeListener();
Expand All @@ -276,4 +276,13 @@ export default class PlayerTestWorld {
delete this.eventListeners[key];
};
};

private ensurePlayerInitialized = async (): Promise<void> => {
if (!this.isPlayerInitialized) {
// Trick to make sure the player is initialized,
// otherwise method calls might have no effect.
await new Promise((resolve) => setTimeout(resolve, 150));
Copy link
Contributor

Choose a reason for hiding this comment

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

How come the change to the fixed wait time? Why not use the same trick as before with player.isPlaying()?

Unrelated to this PR but it would be very nice if player.initialize() function was blocking, then we could remove all of 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.

This was trial and error, player.isPlaying() didn't seem to work reliably on Android emulator.

Agree, player.initialize() being async would possibly solve this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that's unfortunate. Out of interest what happened? Did it await for ever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the opposite, it was not waiting long enough.

I will double-check it with using the latest changes from development

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it didn't help. Initial events are missed somehow.
I will keep the current solution for now!

this.isPlayerInitialized = true;
}
};
}