New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Karma chameleon #1187

Closed
wants to merge 77 commits into
base: master
from

Conversation

Projects
None yet
@gkatsev
Member

gkatsev commented May 1, 2014

This enabled karma testing by default.
Also, it enabled karma testing via saucelabs on travis if the test is initiated by a branch from the main repository, otherwise, it runs the tests in phantomjs using karma. This is because travis ci doesn't export secure credentials on PRs created outside of the main repo.

This is the initial pass of the karma configurations. It should be good to go but it's a bit verbose. I'll be looking into how we can avoid the configuration duplication.

download13 and others added some commits Mar 9, 2014

Prevent mousemove spam
Some platforms produce frequent mousemove events whether or not the mouse has moved. This causes the UI to always be displayed since userActive_ is always `true`. By keeping track of the last mouse position and only firing if the new position is different, we can eleminate the event spam.
Merge branch 'stable'
Conflicts:
  dist/video-js/video-js.css
  dist/video-js/video-js.min.css
  dist/video-js/video.js
Basic UI for Live
Squashed commit of the following:

commit 2c5d4d5
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Tue Apr 1 15:02:37 2014 -0700

    update control text for liveDisplay component

commit 10f21cc
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Tue Apr 1 12:23:20 2014 -0700

    whitespace fix

commit 6a093d6
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Tue Apr 1 12:21:42 2014 -0700

    remove window scope of infinity

commit 2dd8284
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Tue Apr 1 12:12:13 2014 -0700

    update to fix infinity capitalization. only do css logic on valid duration. set any duration of less than zero to window.Infinity

commit f940bef
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Tue Apr 1 10:01:27 2014 -0700

    update to less than zero on player durationChange

commit 554c003
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Mon Mar 31 17:26:13 2014 -0700

    update exports, fix tests

commit 2fb10cb
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Mon Mar 31 13:39:00 2014 -0700

    fix tests, remove update display list in LiveDisplay

commit bc47c5e
Author: Tom Johnson <seniorflexdeveloper@gmail.com>
Date:   Mon Mar 31 13:13:43 2014 -0700

    Basic UI for Live
Making sure that the title element gets placed into the UL element
Included are two sanity check tests for the two areas being checked
to make sure that the menu title items are being correctly placed
in the actual UL element of the menu.

Fixes #1107
Don't force sliders to get evaluated on load. closes #1122
Squashed commit of the following:

commit d5957da
Author: David LaPalomento <dlapalomento@gmail.com>
Date:   Mon Mar 31 16:22:43 2014 -0400

    Peg the volume control to 1.0 on init
    Setup styles so that the volume control initially renders at full volume. This matches browser behavior where volume is available and saves Javascript from having to manually update the volume control on init. After the initial draw, the volume control is updated dynamically the same way it was before.

commit 8bc861f
Author: David LaPalomento <dlapalomento@gmail.com>
Date:   Wed Mar 12 17:16:30 2014 -0400

    Don't force sliders to get evaluated on load
    Since the load and play progress sliders are guaranteed to start from zero, set that through CSS. Calling Slider.prototype.update forces a re-flow because element dimensions are queried and style rules changed. That reflow consistently took around 60ms on my laptop which would mean dropped frames and "jerkiness" on initialization.
getElementsById -> getElementById
small typo fix in code that leads to error
Merge pull request #1130 from stin7/patch-1
getElementsById -> getElementById
Merge branch 'stable'
Conflicts:
	dist/video-js/video-js.css
	dist/video-js/video-js.min.css
	dist/video-js/video.js
Included self-hosting MIME type instructions
Added information on MIME type server configuration to self-hosting instructions in setup.md
@ange007

This comment has been minimized.

Show comment
Hide comment
@ange007

ange007 Apr 19, 2014

Contributor

There is no link.

Contributor

ange007 commented on CHANGELOG.md in 4660573 Apr 19, 2014

There is no link.

heff and others added some commits May 7, 2014

Merge pull request #1149 from bosoniq/master
Included self-hosting MIME type instructions
More explicit MIME type note
Added a more explicit MIME type note in setup. Moved to a footnote and made comment more verbose to avoid confusion.
Made negative LESS calculations a little more readable.
Added line to the changelog for PR merge.
Better error handling. closes #1197
Squashed commit of the following:

commit 81d7859
Author: Steve Heffernan <steve@zencoder.com>
Date:   Mon May 12 12:53:59 2014 -0700

    Removed unneeded comments

commit c7ad732
Author: Steve Heffernan <steve@zencoder.com>
Date:   Fri May 9 14:29:31 2014 -0700

    Addressed comments in #1191
    Now clearing errors on loadstart events.
    Added some default error messages.

commit a742239
Author: Steve Heffernan <steve@zencoder.com>
Date:   Wed May 7 15:38:31 2014 -0700

    Fixed the error display to hide by default

commit 561c3f8
Author: Steve Heffernan <steve@zencoder.com>
Date:   Mon May 5 10:44:47 2014 -0700

    Added support for displaying a message for the error.

commit 2214207
Author: Steve Heffernan <steve@zencoder.com>
Date:   Fri May 2 17:18:22 2014 -0700

    Updated spinner to hide on all errors

commit 95d7e70
Author: Steve Heffernan <steve@zencoder.com>
Date:   Fri May 2 15:37:44 2014 -0700

    Exported ErrorDisplay

commit 11ca9cd
Author: Steve Heffernan <steve@zencoder.com>
Date:   Fri May 2 15:35:46 2014 -0700

    Updated flash tech to support new errors

commit 56cbe66
Author: Steve Heffernan <steve@zencoder.com>
Date:   Fri May 2 13:06:49 2014 -0700

    Started on better error handling and displaying in the UI when an error has occurred.

commit 740014c
Author: Steve Heffernan <steve@zencoder.com>
Date:   Wed Apr 30 16:11:33 2014 -0700

    Added better global log/error/warn functions.
    Added sinon.js for stubs in tests.
    Updated grunt version to satisfy peer dependency warning.
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 14, 2014

Member

I updated to allow running the tests in more browsers via the test task. I also consolidated the karma.conf and the localkarma.conf.

Member

gkatsev commented May 14, 2014

I updated to allow running the tests in more browsers via the test task. I also consolidated the karma.conf and the localkarma.conf.

@gkatsev gkatsev referenced this pull request May 14, 2014

Closed

Karma local sauce #1206

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 14, 2014

Member

So, for watch, the config has which task to run. We could set it to karma:dev which runs karma in firefox, chrome, and safari or just set it to chrome.
However, karma has some built-in watch capabilities that is separate from what grunt-watch provides.
Probably the correct solution is investigate karma's watch and use that (in addition?) to grunt-watch, but for now we should change the task that grunt-watch runs to something like test.

Member

gkatsev commented May 14, 2014

So, for watch, the config has which task to run. We could set it to karma:dev which runs karma in firefox, chrome, and safari or just set it to chrome.
However, karma has some built-in watch capabilities that is separate from what grunt-watch provides.
Probably the correct solution is investigate karma's watch and use that (in addition?) to grunt-watch, but for now we should change the task that grunt-watch runs to something like test.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 14, 2014

Member

Also, I tested that the "travis pr" thing works, still via

TRAVIS_PULL_REQUEST=true grunt test
Member

gkatsev commented May 14, 2014

Also, I tested that the "travis pr" thing works, still via

TRAVIS_PULL_REQUEST=true grunt test
@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap May 14, 2014

Member

Does calling play() again hurt? Silently disregarding API calls seems like a bad idea to me.

Member

dmlap commented on src/js/player.js in d7f840a May 14, 2014

Does calling play() again hurt? Silently disregarding API calls seems like a bad idea to me.

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 14, 2014

Member

Calling play on an error'd video element never fixes playback and triggers the waiting event, which currently shows the spinner. I didn't like that user experience, but that should be handled by hiding play buttons and spinner on an error. I'll kill this part.

Member

heff replied May 14, 2014

Calling play on an error'd video element never fixes playback and triggers the waiting event, which currently shows the spinner. I didn't like that user experience, but that should be handled by hiding play buttons and spinner on an error. I'll kill this part.

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap May 14, 2014

Member

Why not === on line 6, 8, and 11?

Member

dmlap commented on src/js/media-error.js in d7f840a May 14, 2014

Why not === on line 6, 8, and 11?

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 14, 2014

Member

at the time I was thinking there was no threat of anything mistakenly equaling 'object', but it should still be === anyway. Thanks for catching.

Member

heff replied May 14, 2014

at the time I was thinking there was no threat of anything mistakenly equaling 'object', but it should still be === anyway. Thanks for catching.

files: [
'../test/karma-qunit-shim.js',
"../src/js/core.js",

This comment has been minimized.

@heff

heff May 14, 2014

Member

If we can find a way to not duplicate the source file list here that'd be good.

@heff

heff May 14, 2014

Member

If we can find a way to not duplicate the source file list here that'd be good.

This comment has been minimized.

@gkatsev

gkatsev May 15, 2014

Member

Actually, yes, we can collapse this down to a glob.

@gkatsev

gkatsev May 15, 2014

Member

Actually, yes, we can collapse this down to a glob.

This comment has been minimized.

@gkatsev

gkatsev May 16, 2014

Member

Unfortunately, it seems like we may not be able to collapse this into a glob because then the files won't load in the right order.

@gkatsev

gkatsev May 16, 2014

Member

Unfortunately, it seems like we may not be able to collapse this into a glob because then the files won't load in the right order.

Merge branch 'karma-chameleon' of github.com:gkatsev/video.js into gk…
…atsev-karma-chameleon

Conflicts:
	package.json
Show outdated Hide outdated Gruntfile.js
tasksMinified,
tasksMinifiedApi;
grunt.task.run(['jshint', 'less', 'build', 'minify', 'usebanner']);

This comment has been minimized.

@heff

heff May 15, 2014

Member

I've had issues with assuming grunt.task.run would happen synchronously inside a function like this. It seems to be working fine here, but just a warning.

@heff

heff May 15, 2014

Member

I've had issues with assuming grunt.task.run would happen synchronously inside a function like this. It seems to be working fine here, but just a warning.

This comment has been minimized.

@gkatsev

gkatsev May 15, 2014

Member

grunt.task.run queues up the list of tasks to be ran after this function exits. So, basically, this function queues up

  1. jshint
  2. less
  3. build
  4. minify
  5. usebanner
  6. karma:chrome
  7. karma:minified_chrome
  8. karma:minfied_api_chrome

to be run.

@gkatsev

gkatsev May 15, 2014

Member

grunt.task.run queues up the list of tasks to be ran after this function exits. So, basically, this function queues up

  1. jshint
  2. less
  3. build
  4. minify
  5. usebanner
  6. karma:chrome
  7. karma:minified_chrome
  8. karma:minfied_api_chrome

to be run.

This comment has been minimized.

@heff

heff May 15, 2014

Member

Oh interesting. Sorry, I didn't look deep enough to realize you were manipulating the tasks array with this.

@heff

heff May 15, 2014

Member

Oh interesting. Sorry, I didn't look deep enough to realize you were manipulating the tasks array with this.

This comment has been minimized.

@heff

heff May 15, 2014

Member

And now I understand my previous issues with grunt.task.run... :-P. I was assuming the tasks would be run inline.

@heff

heff May 15, 2014

Member

And now I understand my previous issues with grunt.task.run... :-P. I was assuming the tasks would be run inline.

chromecanary: {
browsers: ['ChromeCanary']
},
chrome: {

This comment has been minimized.

@heff

heff May 16, 2014

Member

What does having these individual browser definitions get us?

@heff

heff May 16, 2014

Member

What does having these individual browser definitions get us?

Show outdated Hide outdated Gruntfile.js
});
tasks = tasks.concat(tasksMinified).concat(tasksMinifiedApi);
tasks = tasks.map(function(el) {

This comment has been minimized.

@heff

heff May 16, 2014

Member

el?

@heff

heff May 16, 2014

Member

el?

This comment has been minimized.

@gkatsev

gkatsev May 16, 2014

Member

an element of the list but probably should be renamed to task. Especially since it'll then be consistent with the above maps.

@gkatsev

gkatsev May 16, 2014

Member

an element of the list but probably should be renamed to task. Especially since it'll then be consistent with the above maps.

grunt.task.run(['karma:phantomjs', 'karma:minified_phantomjs', 'karma:minified_api_phantomjs']);
} else if (process.env.TRAVIS) {
grunt.task.run(['karma:saucelabs']);
} else {

This comment has been minimized.

@heff

heff May 16, 2014

Member

This is cool stuff. It could also use some more comments I think. It's a little hard to jump in and know what's going on.

@heff

heff May 16, 2014

Member

This is cool stuff. It could also use some more comments I think. It's a little hard to jump in and know what's going on.

This comment has been minimized.

@gkatsev

gkatsev May 16, 2014

Member

I'll add a comment explaining this.

@gkatsev

gkatsev May 16, 2014

Member

I'll add a comment explaining this.

@heff heff closed this in 3189ec1 May 16, 2014

@gkatsev gkatsev deleted the gkatsev:karma-chameleon branch May 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment