search

videojs / videojs-contrib-ads

A Tool for Building Video.js Ad Plugins
http://videojs.github.io/videojs-contrib-ads/
Other
384 stars 257 forks source link

restore sources instead of once source #412

Closed mysuf closed 6 years ago

mysuf commented 6 years ago

I modified that test to form that better reflects current state.

incompl commented 6 years ago

Hmm, it looks like the test needs to mock currentSources instead of currentSrc, does that fix it? I'm not sure why the test has two places that it sets updatedSrc so maybe it could use cleanup.

mysuf commented 6 years ago

1 test failed on something else on one of nodes. Dunno why..

mysuf commented 6 years ago

It cannot reach updatedSrc in tech_.src. I think that it should contain only getter return 'ad.webm';

gkatsev commented 6 years ago

Seems fine to me, the only thing I'd be slightly concerned about, though, maybe it's not worth being concerned over it, is that if techs or source handlers are added during the ad, we may end up selecting a different source than what was originally selected.

mysuf commented 6 years ago

Yep, it is not absolutely bulletproof. But adding some hooks during ad playback sounds error prone to me at all..

incompl commented 6 years ago

I reran the build job that failed and it passed so I think it was a fluke.

I think selecting a different rendition after ads doesn't seem unreasonable, it may even be better in some cases.

LGTM

incompl commented 6 years ago

I am currently testing stuff for a release of https://github.com/videojs/videojs-contrib-ads/pull/407, in order to not restart testing I'll do this as a second release after. Sorry for the delay.