Skip to content

Commit

Permalink
Quick fix for Player unit tests
Browse files Browse the repository at this point in the history 
The player unit test setup creates a fake StreamingEngine, which
responds to a request for the current period by returning a value
captured in an anonymous function.

Because of this, any test which loads a manifest other than the
"var manifest" at the top will result in inconsistencies between
the current period known to the fake StreamingEngine and the manifest
loaded by the Player instance.

This makes sure all tests in this file are using the same "var
manifest", even if they overwrite it.

This is not the best fix.  The Player unit tests should be
restructured so that the fake StreamingEngine is always consistent
with the Player instance it belongs to.

Change-Id: Iad3b61006de3dcf902334f5348cd51fe7b5f8db4
  • Loading branch information
@joeyparrish
joeyparrish committed Dec 16, 2017
1 parent cbaefcf commit f4d89cb
Showing 1 changed file with 26 additions and 24 deletions.
50 changes: 26 additions & 24 deletions test/player_unit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ describe('Player', function() {
player.createStreamingEngine = function() {
// This captures the variable |manifest| so this should only be used
// after the manifest has been set.
// Subtle: because this captures var manifest above, there cannot be any
// other location for manifests in these tests.
// TODO: fix this to use the manifest currently loaded by the player.
var period = manifest.periods[0];
streamingEngine.getCurrentPeriod.and.returnValue(period);
return streamingEngine;
Expand Down Expand Up @@ -956,7 +959,7 @@ describe('Player', function() {

describe('filterTracks', function() {
it('retains only video+audio variants if they exist', function(done) {
var manifest = new shaka.test.ManifestGenerator()
manifest = new shaka.test.ManifestGenerator()
.addPeriod(0)
.addVariant(1)
.bandwidth(200)
Expand Down Expand Up @@ -992,7 +995,7 @@ describe('Player', function() {
var variantTracks1 = [
{
id: 2,
active: false,
active: true,
type: 'variant',
bandwidth: 400,
language: 'en',
Expand Down Expand Up @@ -2098,7 +2101,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
var activeVariant = getActiveVariantTrack();
expect(activeVariant.id).toBe(0);

Expand Down Expand Up @@ -2135,7 +2138,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
var activeVariant = getActiveVariantTrack();
expect(activeVariant.id).toBe(0);

Expand Down Expand Up @@ -2166,7 +2169,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
var activeVariant = getActiveVariantTrack();
expect(activeVariant.id).toBe(0);

Expand Down Expand Up @@ -2195,7 +2198,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
abrManager.chooseVariant.calls.reset();

var activeVariant = getActiveVariantTrack();
Expand All @@ -2218,7 +2221,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

onKeyStatus({'abc': 'output-restricted'});
Expand All @@ -2238,7 +2241,7 @@ describe('Player', function() {
.addVideo(2)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

onKeyStatus({'abc': 'internal-error'});
Expand All @@ -2258,7 +2261,7 @@ describe('Player', function() {
.addVideo(3)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

// We have some key statuses, but not for the key IDs we know.
Expand All @@ -2279,7 +2282,7 @@ describe('Player', function() {
.addVideo(3)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

// This simulates, for example, the lack of key status on Chromecast
Expand All @@ -2300,7 +2303,7 @@ describe('Player', function() {
.addVideo(3)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

// A synthetic key status contains a single key status with key '00'.
Expand All @@ -2322,7 +2325,7 @@ describe('Player', function() {
.addVideo(5)
.build();

setupPlayer(manifest)
setupPlayer()
.then(function() {
expect(player.getVariantTracks().length).toBe(3);

Expand Down Expand Up @@ -2353,7 +2356,7 @@ describe('Player', function() {
expect(MediaSource.isTypeSupported('video/unsupported')).toBe(true);
// FakeDrmEngine's getSupportedTypes() returns video/mp4 by default.

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
var tracks = player.getVariantTracks();
expect(tracks.length).toBe(1);
expect(tracks[0].id).toBe(1);
Expand All @@ -2371,7 +2374,7 @@ describe('Player', function() {
.addVideo(3)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(3);

player.configure(
Expand All @@ -2394,7 +2397,7 @@ describe('Player', function() {
.addVideo(3).size(190, 190)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(3);

player.configure(
Expand All @@ -2417,7 +2420,7 @@ describe('Player', function() {
.addVideo(3).size(190, 190)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(3);

player.configure({restrictions: {minWidth: 100, maxWidth: 1000}});
Expand All @@ -2439,7 +2442,7 @@ describe('Player', function() {
.addVideo(3).size(190, 190)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(3);

player.configure({restrictions: {minHeight: 100, maxHeight: 1000}});
Expand All @@ -2462,7 +2465,7 @@ describe('Player', function() {
.addAudio(4)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(2);

player.configure({restrictions: {minHeight: 100, maxHeight: 1000}});
Expand All @@ -2484,7 +2487,7 @@ describe('Player', function() {
.addVideo(3).size(190, 190)
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(player.getVariantTracks().length).toBe(3);

onError.and.callFake(function(e) {
Expand Down Expand Up @@ -2521,7 +2524,7 @@ describe('Player', function() {
.addVideo(5).mime('video/mp4', 'bad')
.build();

setupPlayer(manifest).then(function() {
setupPlayer().then(function() {
expect(abrManager.setVariants).toHaveBeenCalled();
var variants = abrManager.setVariants.calls.argsFor(0)[0];
// We've already chosen codecs, so only 3 tracks should remain.
Expand All @@ -2542,10 +2545,9 @@ describe('Player', function() {
}

/**
* @param {shakaExtern.Manifest} manifest
* @return {!Promise}
*/
function setupPlayer(manifest) {
function setupPlayer() {
var parser = new shaka.test.FakeManifestParser(manifest);
var parserFactory = function() { return parser; };
return player.load('', 0, parserFactory).then(function() {
Expand Down Expand Up @@ -2578,8 +2580,8 @@ describe('Player', function() {
});

it('rejects empty manifests', function(done) {
var emptyManifest = new shaka.test.ManifestGenerator().build();
var emptyParser = new shaka.test.FakeManifestParser(emptyManifest);
manifest = new shaka.test.ManifestGenerator().build();
var emptyParser = new shaka.test.FakeManifestParser(manifest);
var emptyFactory = function() { return emptyParser; };

player.load('', 0, emptyFactory).then(fail).catch(function(error) {
Expand Down

0 comments on commit f4d89cb

Please sign in to comment.