Sped up tests by relying on watchify and splitting up tests #2254

heff · 2015-06-10T19:59:44Z

The main thing that's happening here is setting up parallel watch tasks so that we can use watchify's smart caching of browserify source files. We end with 3 watch tasks that run at the same time. We can't use grunt-contrib-watch for all of them, because it just starts browserify from scratch each time, instead of using watchify to cache the files, which is why our suite is so slow right now.

grunt browserify:watch - rebuild build/temp/video.js with each change for sandbox manual tests
grunt karma:watch - rerun tests with relevant changes for automated tests
grunt watch - everything else that doesn't need to be smart. Sass and jshint.

The first two are different tasks because that automated tests only pull in the source files they need, not the full video.js. If we wanted to share the two we would need to rewrite the test suite to work off the fully compiled video.js, but I don't think that's the way we want to go.

Also temporarily killed API tests. I'm combining them with other tests so we only have one suite.

Would be interested to get @gkatsev's feedback since the parallel watch tasks idea came from him.
And also @dmlap's since this is reverting some of the work he did to compile the library first before running tests. Let me know if any of this isn't clear.

mmcc · 2015-06-10T20:07:01Z

grunt.js

+          'grunt browserify:watch',
+          'grunt karma:watch'
+        ].join('&')
+      }


Is there a benefit of doing it this way over something like grunt-concurrent?

Cooool. Didn't know about that. I'll try it. Originally had a non-grunt wait command in there too, but with this it's not needed.

Heh, my bad, should have mentioned that yesterday. What we've got here makes sense, but while we're still in a Grunt world we should probably use grunt-concurrent. Also helps avoid any weirdness around zombie processes that this could potentially introduce.

It's been switched to grunt-concurrent now. Was able to back out the keep alive change for connect as well.

mmcc · 2015-06-10T23:00:22Z

lgtm

dmlap · 2015-06-12T04:53:58Z

I'm all for speeding up development builds. My main goal in the build work I did last month was to ensure it remained easy to open up the automated tests in your regular browser so you can debug issues properly. I'd be happy tracing back concatenated+babelified files to their sources as long as I don't have to attempt to fix a test issue solely through a stack trace on the command line. I love running automated tests through the command line but it's a terrible way to fix them when they break.

heff · 2015-07-14T00:54:47Z

Ok, I think I have a solution for tests here. Not perfect but better. Could use a review, please!

Run grunt dev, it will...

Build
Start a server
Concurrently watch multiple file sets
- jsbin and sass watching
- browsersify:watch, building the lib for sandbox and dist while using watchify caching for speed

Run grunt test-ui, it will watch and build test files for the Qunit UI, located at localhost:9999/test/. It requires grunt dev to be running the server already.

Run grunt test-cli it will run karma:watch, watching and auto running test files on the command line when they change.

Also...

Fixed line number reporting (coverage only runs on Travis). In the Qunit UI, you have to use the try/catch option to see the source line numbers in the console. Qunit doesn't handle sourcemaps.
Consolidated Travis test differences into karma.conf.js
Removed iPad and Android from Sauce testing for now
Updated Travis to use their new containers for better speed, and also added npm caching

heff · 2015-07-14T01:01:20Z

I should also note, I tried combining ALL the watch tasks into one concurrent command, but it was a mess of log output. I think it probably makes more sense just to open a new terminal for other tasks. I welcome any ideas for improving from here.

Also there's something wrong with our tests right now that causes them to run infinitely when there's an error in a commonly used file, like component. I can't figure it out, but it made the super-concurrent setup all the more complicated.

mmcc · 2015-07-14T20:25:37Z

.travis.yml

@@ -22,3 +23,7 @@ env:
  - secure: GIbhjUJapvC70nIZVlhVyK+3KAD2TVKpiY/q412OO7V2izbvcM1tvU3LBoMZbROzrt5TT84tCoJDvHnrpL0OvxPwrzL5CUU7h4UTxhTOyQkEinbYAnWlW9wdrvtdczsEvANkFPqBZ53B3hVHZHMLOG8QRWaTBicF68vSHEJFqb4=


This isn't your fault, but while you're editing this Travis yml file, please fix this indentation.

mmcc · 2015-07-14T20:34:38Z

lgtm

gkatsev · 2015-07-14T20:44:05Z

.travis.yml

@@ -8,6 +8,7 @@ before_install:
 - export CHROME_BIN=chromium-browser
 - export DISPLAY=:99.0
 - sh -e /etc/init.d/xvfb start
+script: grunt test && grunt coveralls


Why not just keep the default and have npm test either get passed wither to run coveralls as an option or figure it out itself?

Can you show me what you mean? Previously we had a conditional in the grunt file and it was creating a long chain to follow to figure out tests. This at least seemed like an improvement.

gkatsev · 2015-07-14T21:36:29Z

For the most part, looking good. I'd like to actually try it out locally before we pull it in, though.

heff · 2015-07-14T21:57:13Z

@gkatsev I've added in your requests. Let me know when you've had a chance to try it out. I'd like to get this pulled in soon.

gkatsev · 2015-07-14T22:18:50Z

I'll get to it really soon.

gkatsev · 2015-07-15T20:14:37Z

grunt.js

+      // Needed so watchify can cache intelligently
+      watchAll: [
+        'watch',
+        'browserify:watch',


we should move browserify:watch and browser:tests to the first two items in this list.
Seems like as it currently is, browserify:tests ends up running after karma:watch runs so, it takes some time extra before you can refresh your browser window.

The order doesn't seem to have a significant impact on when they're run (or rather when they finish), from what I've seen. But I'll try it. I think we still want watch first though, because if you're developing the skin you don't want to wait for the rest, and also the tests depend on the skin being available.

Also, this is why I separated them out into test-ui and test-cli, so you can choose which you're using. I like to let the cli version run and beep at me when there's test errors, so I'd personally prioritize karma:watch over the others.

- Temporarily killed API tests. Combining them with other tests. - Using browserify:watch for build/temp/video.js (sandbox testing) - Using karma:watch for automated tests Using individual watch tasks allows watchify to use smart caching in both instances. - Switched to grunt-concurrent for watch tasks - Switched to travis containers, sudo: false - Added caching of npm modules in Travis - Consolidated travis testing - Cleaned up grunt file - Fixed travis.yml spacing - Added the watchAll task for trying it out - Moved travis test script logic to package.json - Moved coverage reporting to Travis only closes videojs#2254

heff · 2015-07-15T22:42:53Z

@gkatsev does this mean anything to you? https://travis-ci.org/videojs/video.js/builds/71163633#L618

gkatsev · 2015-07-15T22:47:46Z

Not sure. I can take a look tomorrow.

heff added the enhancement label Jun 10, 2015

mmcc reviewed Jun 10, 2015
View reviewed changes

heff mentioned this pull request Jul 10, 2015

Unify 'loose' option for all babelify transforms #2337

Closed

heff force-pushed the speedup-tests branch 3 times, most recently from 9938d9b to 37bb1da Compare July 13, 2015 23:48

mmcc reviewed Jul 14, 2015
View reviewed changes

mmcc mentioned this pull request Jul 14, 2015

updated travis.yml file #2354

Closed

gkatsev reviewed Jul 14, 2015
View reviewed changes

gkatsev reviewed Jul 15, 2015
View reviewed changes

heff force-pushed the speedup-tests branch from 51c4518 to 3e35935 Compare July 15, 2015 22:21

heff merged commit 3e35935 into videojs:master Jul 15, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sped up tests by relying on watchify and splitting up tests #2254

Sped up tests by relying on watchify and splitting up tests #2254

heff commented Jun 10, 2015

mmcc Jun 10, 2015

heff Jun 10, 2015

mmcc Jun 10, 2015

heff Jun 10, 2015

mmcc commented Jun 10, 2015

dmlap commented Jun 12, 2015

heff commented Jul 14, 2015

heff commented Jul 14, 2015

mmcc Jul 14, 2015

heff Jul 14, 2015

mmcc commented Jul 14, 2015

gkatsev Jul 14, 2015

heff Jul 14, 2015

gkatsev commented Jul 14, 2015

heff commented Jul 14, 2015

gkatsev commented Jul 14, 2015

gkatsev Jul 15, 2015

heff Jul 15, 2015

heff Jul 15, 2015

heff commented Jul 15, 2015

gkatsev commented Jul 15, 2015

		@@ -22,3 +23,7 @@ env:
		- secure: GIbhjUJapvC70nIZVlhVyK+3KAD2TVKpiY/q412OO7V2izbvcM1tvU3LBoMZbROzrt5TT84tCoJDvHnrpL0OvxPwrzL5CUU7h4UTxhTOyQkEinbYAnWlW9wdrvtdczsEvANkFPqBZ53B3hVHZHMLOG8QRWaTBicF68vSHEJFqb4=

Sped up tests by relying on watchify and splitting up tests #2254

Sped up tests by relying on watchify and splitting up tests #2254

Conversation

heff commented Jun 10, 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

mmcc commented Jun 10, 2015

dmlap commented Jun 12, 2015

heff commented Jul 14, 2015

heff commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmcc commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Jul 14, 2015

heff commented Jul 14, 2015

gkatsev commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Jul 15, 2015

gkatsev commented Jul 15, 2015