Remove extraneous methods from PlaylistLoader #1286
Conversation
src/playlist-loader.js
Outdated
| * | ||
| * @return {Boolean} true if on lowest rendition | ||
| */ | ||
| export const isLowestEnabledRendition = (master, media) => { |
There was a problem hiding this comment.
I feel like this would make more sense being in the playlist.js file now that its not tied to the playlist-loader instance
There was a problem hiding this comment.
Good call. Looks much nicer there 👍
test/playlist-loader.test.js
Outdated
| @@ -791,6 +794,43 @@ QUnit.test('uses last segment duration for refresh delay', function(assert) { | |||
| 'used half targetDuration when update is false'); | |||
| }); | |||
|
|
|||
| QUnit.test('isLowestEnabledRendition detects if we are on lowest rendition', | |||
There was a problem hiding this comment.
I think putting these tests in playlist.test.js would be more appropriate and then you can test on just plain objects with the info thats needed instead of all this extra creating a loader, responding to request, etc.
There was a problem hiding this comment.
master-playlist-controller.test.js also already has integration tests for isLowestEnabledRendition, so even more reason to convert these to unit tests
test/playlist.test.js
Outdated
| @@ -4,6 +4,64 @@ import QUnit from 'qunit'; | |||
| import xhrFactory from '../src/xhr'; | |||
| import { useFakeEnvironment } from './test-helpers'; | |||
|
|
|||
| QUnit.module('Playlist Status'); | |||
There was a problem hiding this comment.
There is already an entire module of tests for blacklisting and enabled states near the bottom of this file...
https://github.com/videojs/videojs-contrib-hls/blob/master/test/playlist.test.js#L791
Requirements Checklist