VideoJS 5.0 Updates (DO NOT MERGE) #116

misteroneill · 2015-09-08T21:20:54Z

This updates the plugin for use with VideoJS 5.0 changes.

There is one functional change that should not affect consumers of the plugin whatsoever: there is no longer any use of setImmediate (non-standard) or requestAnimationFrame; rather, this is simplified to use setTimeout universally. This not only makes code easier to reason about - no need to think about the minor differences in 3 separate APIs - but it makes testing using Sinon's fake timers more straight-forward.

Testing Changes

The bulk of the changes in this PR are related to testing; this seemed like a good opportunity for that work. There were significant sources of fragility in the tests previously and this sweeps the entire test suite with changes like:

Reduced reliance on globals

Each test should have whatever scaffolding it needs attached to this, imported from other sources, or hidden in a closure. Leaking globals is a good way to cause hard-to-debug issues.

Eliminated duplicated code

Particularly in setup/teardown processes, there was a ton of repetition. This should now be minimized via sharedModuleHooks.

Switched to QUnit 2.0-style tests

The only QUnit global that should be used now is QUnit.
QUnit.module should be passed beforeEach/afterEach hooks instead of the deprecated setup/teardown.
All assertion functions can be referenced on the assert object passed into each test (defined by QUnit.test).

Reduced assertion fragility

There was a lot of use of QUnit.assert.equal, which could potentially lead to bugs. QUnit.assert.strictEqual is used now, for the same reason === is preferred to ==.

Used Sinon extensively

Fake timers were already in use in a few places, but this makes that universal (particularly in the Flash tests). All custom/manual function spying is replaced by uses of sinon.spy - no more manual counting or pushing onto arrays!

gkatsev · 2015-09-08T21:23:27Z

.gitignore

@@ -1,2 +1,3 @@
+/dist/


do we have an npmignore? Because otherwise, npm would ignore the dist folder as well.

Good point. Added an empty .npmignore file.

Also, when this is ready to go, we should try publishing it to npm under a next or beta tag.

misteroneill · 2015-09-15T22:26:37Z

This can probably be reviewed, but should not be merged until we are sure it should be in master since it's not 4.x compatible!

misteroneill · 2015-09-16T15:05:52Z

Also, worth noting: two tests are currently broken in events.html, but this is a result of a bug in VideoJS.

dmlap · 2015-09-17T14:00:06Z

package.json

    "qunitjs": "^1.17.1",
-    "video.js": "^4.12.4"
+    "sinon": "~1.16.1",


Nitpick but isn't ^ the best-practice version specifier these days?

I thought it was ~.

I've run into a lot of issues in the past with ^ bringing in breaking changes (despite semver saying minor versions should be backward-compatible); sometimes multiple dependency layers deep.

That said, I'm fine with changing these. It ought to be a company/project-wide standard, though.

With sinon, I would even just get rid of the prefix altogether. They broke too many times on minor and patch releases to trust them.
lodash I think is fine to use ^.

Interesting. I've never had a problem with Sinon at all.

dmlap · 2015-09-17T17:53:00Z

This was really tough to review. I know the timeout stuff was a pain to convert but it would have been a lot easier if we could have split the upgrade + refactor.

misteroneill · 2015-09-17T18:04:45Z

Yeah, that's fair.

There were so many test failures related to subtle differences in things that are now async that were not previously - as well as the order and structure of events between video.js and contrib-ads - that it would've been a big thing regardless.

misteroneill · 2015-09-21T20:51:55Z

This is good for review once again.

nickygerritsen · 2015-09-24T12:26:45Z

I'm using this PR for implementing ads in 5.0, but I have a suggestion:

Could you add a main section to package.json. Then I can use a nice import 'videojs-contrib-ads' instead of import '../node_modules/videojs-contrib-ads/dist/videojs.ads.js'.

Maybe this only makes sense after releasing?

dmlap · 2015-09-24T17:45:45Z

Would be nice to get @nickygerritsen's suggestions in here too but LGTM either way.

nickygerritsen · 2015-09-25T20:06:54Z

dist/videojs.ads.global.js

+    player.ads = {
+      state: 'content-set',
+
+      // Call this when an ad response has been recieved and there are


Although not changed in this PR, recieved does not seem as a valid word to me ;). This is already wrong in the src file of course.

The same typo is there 15 lines lower.

You're right - "i" before "e" except after "c"! :)

- This removes the variable use of setImmediate/clearImmediate and RAF; replacing it with universal use of setTimeout. - Tests were not great because they did a lot of manual overriding of setTimeout and friends. They will now use Sinon's fake timers feature instead. - Updates tests to use QUnit 2.0 style, for future compatibility. Related to this, there is a general shift here away from relying on any global state (outside of helper functions) in tests as that makes them less atomic.

Also added a `hasClass` function to make it easier to test that.

Not sure why this one test needs async handling, but it appears to. The only way to make it not async is to trigger the 'play' event on the player's `tech_` object.

This also renames the obscure `timeout` in the plugin to the more meaningful `adTimeoutTimeout`.

misteroneill · 2015-09-28T20:05:17Z

Closing in favor of #121

misteroneill changed the title ~~VideoJS 5.0 Updates (NOT READY)~~ VideoJS 5.0 Updates Sep 8, 2015

gkatsev reviewed Sep 8, 2015
View reviewed changes

misteroneill force-pushed the vjs-5-upgrade branch from 75e8d2f to 3707f90 Compare September 14, 2015 16:24

dmlap reviewed Sep 17, 2015
View reviewed changes

gkatsev mentioned this pull request Sep 22, 2015

Ads contrib for 5.0? videojs/video.js#2626

Closed

nickygerritsen reviewed Sep 25, 2015
View reviewed changes

misteroneill changed the title ~~VideoJS 5.0 Updates~~ VideoJS 5.0 Updates (DO NOT MERGE) Sep 28, 2015

misteroneill added 18 commits September 28, 2015 13:56

Use getComponent going forward

22c6b38

'play' should be triggered on tech

8fea5b1

Add empty .npmignore

7266ad5

Use strict assertions

8371114

Update vjs and trigger on player instead of tech

c6b91f4

Also added a `hasClass` function to make it easier to test that.

Fix async issue w/ test

71f8f0f

Not sure why this one test needs async handling, but it appears to. The only way to make it not async is to trigger the 'play' event on the player's `tech_` object.

Binding not necessary on these events

aea67a4

Re-evaluate tick amounts

d9be8fa

Formatting/whitespace

5c6c381

Restore lost test

e074ca0

Flesh out timer-detection tests

4425d5b

This also renames the obscure `timeout` in the plugin to the more meaningful `adTimeoutTimeout`.

Restore missing test

f9f349c

Make test synchronous

bc2367b

Bump video.js rc version

dd28ea0

Add 'main' to package.json

c5c975e

Fix comment typo

c0e10f0

Bump to latest video.js rc

13b2bac

misteroneill force-pushed the vjs-5-upgrade branch from de9e6bd to 13b2bac Compare September 28, 2015 17:57

Add migrating-to-3.0 doc

38899d1

misteroneill closed this Sep 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VideoJS 5.0 Updates (DO NOT MERGE) #116

VideoJS 5.0 Updates (DO NOT MERGE) #116

misteroneill commented Sep 8, 2015

gkatsev Sep 8, 2015

misteroneill Sep 9, 2015

gkatsev Sep 9, 2015

misteroneill commented Sep 15, 2015

misteroneill commented Sep 16, 2015

dmlap Sep 17, 2015

misteroneill Sep 17, 2015

gkatsev Sep 17, 2015

misteroneill Sep 17, 2015

dmlap commented Sep 17, 2015

misteroneill commented Sep 17, 2015

misteroneill commented Sep 21, 2015

nickygerritsen commented Sep 24, 2015

dmlap commented Sep 24, 2015

nickygerritsen Sep 25, 2015

misteroneill Sep 25, 2015

misteroneill commented Sep 28, 2015

VideoJS 5.0 Updates (DO NOT MERGE) #116

VideoJS 5.0 Updates (DO NOT MERGE) #116

Conversation

misteroneill commented Sep 8, 2015

Testing Changes

Reduced reliance on globals

Eliminated duplicated code

Switched to QUnit 2.0-style tests

Reduced assertion fragility

Used Sinon extensively

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill commented Sep 15, 2015

misteroneill commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlap commented Sep 17, 2015

misteroneill commented Sep 17, 2015

misteroneill commented Sep 21, 2015

nickygerritsen commented Sep 24, 2015

dmlap commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill commented Sep 28, 2015