tests: memory leak fixes in tests #5861
Conversation
| @@ -444,13 +444,13 @@ if (Html5.supportsNativeTextTracks()) { | |||
|
|
|||
| el.textTracks = tt; | |||
|
|
|||
| /* eslint-disable no-unused-vars */ | |||
| const htmlTech = new Html5({el}); | |||
There was a problem hiding this comment.
I guess we should have thought about no-unused-vars a bit more here. woops!
| @@ -364,6 +364,8 @@ QUnit.test('menu items should polyfill mode change events', function(assert) { | |||
| assert.equal(changes, 2, 'clicks trigger change events'); | |||
|
|
|||
| player.dispose(); | |||
| trackMenuItem.dispose(); | |||
| player.textTracks().off('change'); | |||
There was a problem hiding this comment.
I almost wonder if we should call off on track lists when we dispose, instead of doing this.
There was a problem hiding this comment.
probably, though, I thought we did that?
There was a problem hiding this comment.
I think we clear them out of Tracks. Let's see...
There was a problem hiding this comment.
Doesn't seem like it: https://github.com/videojs/video.js/blob/master/src/js/tech/tech.js#L367
There was a problem hiding this comment.
Maybe we want to call off on them on player dispose, rather than tech dispose, since the track lists stick around between techs.
There was a problem hiding this comment.
I will remove the test changes related to this if #5867 is ok.
| @@ -21,8 +20,9 @@ TrackBaseline(Track, { | |||
| kind: 'subtitles', | |||
| mode: 'disabled', | |||
| label: 'English', | |||
| language: 'en', | |||
| tech: new TechFaker() | |||
There was a problem hiding this comment.
Since we need a way to cleanup this tech, we cannot just pass a new TechFaker into baseline. Instead I create one in every test inside TrackBaseline and pass it as an options.
Using
DomData.elDatacontained a ton of improperly cleaned up objects in tests this pull request fixes that.