Start down path of new build system:

[cthrax]

• Move tests to karma test runner and mocha framework, also adding chai assertions
• Add gulp tasks for testing, watching, concatting and compressing
• Update travis file to use new build system
• Remove jstestDriver stuff
• Reorganize a bit so that it's easier to see what's going on
• Create a dist folder to hold the output of builds (should probably look at having a bower branch/repository where these are held, instead of master)

I'm pretty sure we don't want to push this to master, but that's where this was diffed against.

[cthrax]

dygraph-dev was pretty fragile due to the ordering of the files. The build system now creates a dygraph-combined.dev.js that puts everything in the right order, maintains that order in one place and includes the dygraph-options file that is only for dev.

[danvk]

This is fantastic! I'm on vacation at the moment, but I'll plan to take a look at this sometime in the few days.

[danvk]

Also cc @kberg in case he'd like to take a look. gulp, mocha and karma are the future, shell scripts and jstd are the past.

[cthrax]

@danvk @kberg any movement on this?

[danvk]

Sorry for the slow responses! I'll plan to take a look at it this week.

[danvk]

Apologies for the long radio silence. I'm looking at this now.

[cthrax]

No worries, I understand things get in the way.

[danvk]

First of all: a huge thanks for doing this! I would like to merge this once we get a few issues sorted out. It will make me very happy to replace the home-grown dygraphs build system (with lots of shell scripts) with something more modern & standard.

Again, sorry the review took so long. The good news is that I haven't been reviewing any other dygraphs code, either (only one PR has been merged since you sent yours out), so the merge conflicts shouldn't be difficult to resolve.

My biggest bit of feedback is that I'd like to maintain file revision history (and keep git blame and its github equivalent) working to the extent that we can. That probably means formatting the tests like this (i.e. w/o indentation inside the describe() {...} block) and keeping them where they currently are auto_tests/tests rather than specs/unit. For files which are moved and edited, we may need to use two commits.

I love that we're getting code coverage! It would be nice to get it on a per-file basis, though, so that we could act on it and publish it on coveralls.io. I've run into issues with code coverage & source maps in other projects. I solved it in one using this script, which takes an LCOV code coverage file and applies a source map to it. Something similar would work for this setup, though this isn't necessarily a problem that needs to be solved in this PR.

I'm curious to hear your thoughts on gulp vs. grunt. I have no strong preference either way.

More specific questions/nits:

• What's the karma-chai-plugins dep that's being pulled in from your github?
• The /dist directory doesn't belong in source control (at least not on master—it can live in release branches, just like dygraph-combined.js currently does).
• What's the distinction being made between /src/dygraph and /src/lib?
• Any reason to put the main code in /src/dygraph/dygraph.js and not just /src/dygraph.js?
• Is /specs a standard directory for tests? I've never seen it before.
• We use a 2-space tab everywhere in dygraphs, not a 4-space.
• Why do the generated files go in dist/scratch, rather than just dist?
• Why does Travis-CI need bower install now? Can’t we grab everything via NPM? (jQuery is available via NPM.)
• Should gulp watch also run create-dev? (That's what the tests pages we use for iteration source.)
• Testing is definitely slower with this setup (test execution goes from ~3s→10s). I'm curious how much of that is starting up the karma server. With gulp watch-test, does this happen every time a file changes? Is it possible to start it once and keep it running?
• gulp creates dygraph-autoloader.js, but nothing that I can see references it. Should the /tests files be pulling this in?

If you're up for doing one more round of changes on this, I think it's basically good to go. We should be able to get it in in less than two months :)

[cthrax]

I can answer a few of these right now, the rest is going to have to come at a later date when I can actually work on it.

My biggest bit of feedback is that I'd like to maintain file revision history (and keep git blame and its github equivalent) working to the extent that we can....
I'll see what I can do here. It was a lot easier to move the files to make sure that I converted everything over to the new test format by just moving them (there are a LOT of tests).

It would be nice to get it on a per-file basis, though, so that we could act on it and publish it on coveralls.io
I'm reading this as the files in src/tree rather than the combined file, is that correct? If so, I would have to look at that. I know that I can use the same coverage tool (istanbul I think) to get per file coverage, I'm not sure how to get that to the sourcemapped files though. We might be able to solve that more elegantly by making the separate files requirejs or some other AMD modules and require things in the correct order that way, rather than depending on tools to get the files in the correct order.

What's the karma-chai-plugins dep that's being pulled in from your github?
This is a dependency that pulls in some plugins for working with chaijs and sinon. Used for some asserts and some mocking. It's forked from https://www.npmjs.com/package/karma-chai-plugins so that dirty-chai can be pulled in at the same time as the other chai plugins. This plugin, and most karma plugins, basically allows karma to load some required files into the tests pages before the tests run, so that the tests can depend on those files/libraries without every test file having to pull in those files

The /dist directory doesn't belong in source control (at least not on master—it can live in release branches, just like dygraph-combined.js currently does).
I'm not advocating either way, but this is certainly a point of contention for every bower package maintainer I have interacted with. It boils down to bower requiring the built files stored in github (different branches are obviously fine). The projects that I've worked on have all stored a "binary" folder in master for ease of distribution and lock-stepping versions with "binaries".

What's the distinction being made between /src/dygraph and /src/lib?
/src/dygraph is the actual dygraph code, /src/lib is the dependencies that are not able to be resolved by bower, but that dygraphs depends on. This is a convention I've picked up for helping to make a distinction between the primary value add software and the libraries that are just tools to prevent re-inventing the wheel. I've seen these libraries be updated independently as well as replaced and so it's easier to pull them out external to the primary code. You can take or leave the convention.

Any reason to put the main code in /src/dygraph/dygraph.js and not just /src/dygraph.js?
See the above answer about libraries, but basically just keeps the distinction of what is core competency.

Is /specs a standard directory for tests? I've never seen it before.
It's fairly standard, I think it came about as a ruby-ism initially, but it stands for specification and I tend to use that model when I'm using the BDD style unit tests. ("describe", "it" model that was pulled in from chai)

We use a 2-space tab everywhere in dygraphs, not a 4-space.
Ok, I didn't see that in the .jshintrc, so I think I just used my editor default.

Why do the generated files go in dist/scratch, rather than just dist?
It was a way to create the two distributions a little more easily. (GWT and bower) since they both have different file structure requirements.

Why does Travis-CI need bower install now? Can’t we grab everything via NPM? (jQuery is available via NPM.)
I put the dependencies in bower so that the bower package was a little more complete and rather than leave the dep in both places, removed it from package.json. This keeps client side deps separate from dev deps. No other reason than that.

Should gulp watch also run create-dev? (That's what the tests pages we use for iteration source.)
Maybe, maybe not. create-dev creates an unminified version of the full lib, but the 'concat' task creates sourcemaps, so it's just as easy to debug (at least in chrome) so I was just using that.

Testing is definitely slower with this setup (test execution goes from ~3s→10s). I'm curious how much of that is starting up the karma server. With gulp watch-test, does this happen every time a file changes? Is it possible to start it once and keep it running?
I think it's a combination of starting up the karma server every time AND running coverage every time. We could create a separate karma.config (or some other neat tricks to prevent duplication) to have a dev-test conf that doesn't run coverage.

Also, you can specify ".only" on any "describe" or "it" to run just that test for development, which would reduce run time significantly.

gulp creates dygraph-autoloader.js, but nothing that I can see references it. Should the /tests files be pulling this in?
I didn't find a use for the autoloader because the tests are just using the properly combined dygraph.js. It seemed like the autoloader was something you would use if you were trying to running everything in-place instead of running from a built version of the lib. I made sure to leave it in there, since the original build system had it, but I'm not sure where it fits in with having gulp in place.

[cthrax]

Oh, and gulp vs grunt, I prefer gulp for its pipeline management versus config management that grunt does. Basically in gulp you setup a bunch of pipelines and manage input/output. Grunt you configure a bunch of plugins ahead of time and then run a bunch of tasks individually.

[danvk]

It would be nice to get it on a per-file basis, though, so that we could act on it and publish it on coveralls.io

I'm reading this as the files in src/tree rather than the combined file, is that correct? If so, I would have to look at that. I know that I can use the same coverage tool (istanbul I think) to get per file coverage, I'm not sure how to get that to the sourcemapped files though. We might be able to solve that more elegantly by making the separate files requirejs or some other AMD modules and require things in the correct order that way, rather than depending on tools to get the files in the correct order.

The repo I referenced runs tests using the concatted (but not minified) source. Then it uses the script to map (coverage on concatted, source map)→coverage on sourcemapped files.

I'm all in favor of dygraphs using some kind of module system to track dependencies, but I don't think we have to hold off on getting coverage data until then.

The /dist directory doesn't belong in source control (at least not on master—it can live in release branches, just like dygraph-combined.js currently does).

I'm not advocating either way, but this is certainly a point of contention for every bower package maintainer I have interacted with. It boils down to bower requiring the built files stored in github (different branches are obviously fine). The projects that I've worked on have all stored a "binary" folder in master for ease of distribution and lock-stepping versions with "binaries".

My preference is to only commit the /dist directory to release branches. I don't like cluttering the version history with "update combined"-type commits.

What's the distinction being made between /src/dygraph and /src/lib?

/src/dygraph is the actual dygraph code, /src/lib is the dependencies that are not able to be resolved by bower, but that dygraphs depends on. This is a convention I've picked up for helping to make a distinction between the primary value add software and the libraries that are just tools to prevent re-inventing the wheel. I've seen these libraries be updated independently as well as replaced and so it's easier to pull them out external to the primary code. You can take or leave the convention.

I'm fine with this so long as we drop the dygraph- prefix on the files, too.

Alternatively, we could get rid of /src/lib entirely if I found replacements for console.js and dashed-canvas.js on NPM. I'd prefer to source third-party libraries via NPM rather than committing them to the dygraphs repo.

Is /specs a standard directory for tests? I've never seen it before.

It's fairly standard, I think it came about as a ruby-ism initially, but it stands for specification and I tend to use that model when I'm using the BDD style unit tests. ("describe", "it" model that was pulled in from chai)

As discussed above, I'd prefer to keep specs/unit as auto_tests/tests, to avoid losing history.

Why do the generated files go in dist/scratch, rather than just dist?

It was a way to create the two distributions a little more easily. (GWT and bower) since they both have different file structure requirements.

My preference would be to keep the primary files (dygraph-combined.js and friends) in the top-level dist directory, ala jQuery and many other libraries.

Why does Travis-CI need bower install now? Can’t we grab everything via NPM? (jQuery is available via NPM.)

I put the dependencies in bower so that the bower package was a little more complete and rather than leave the dep in both places, removed it from package.json. This keeps client side deps separate from dev deps. No other reason than that.

I don't follow this. dygraphs doesn't depend on jQuery. It doesn't depend on anything!

bower.json is in this repo only so that I can publish on bower. I'd prefer to pull in any dependencies via NPM.

Should gulp watch also run create-dev? (That's what the tests pages we use for iteration source.)

Maybe, maybe not. create-dev creates an unminified version of the full lib, but the 'concat' task creates sourcemaps, so it's just as easy to debug (at least in chrome) so I was just using that.

I'm fine either way, I'd just like to have it so that running gulp watch and reloading, e.g. tests/demo.html in the browser will track my changes (as it does now, minus the gulp watch).

[danvk]

(also, let me know if you'd prefer me to take it over from here—I'm happy to do so.)

[danvk]

I'm playing around with a variation on this. One thing I noticed: I don't think the DEBUG stripping is working. Whenever I try to use dygraph-combined.min.js, I just get the error "Must include options reference JS for testing". Did you see this?

[danvk]

Looks like it had to be:

  uglify: {
    compress: {
      global_defs: { DEBUG: false }
    },
  }

[cthrax]

Sorry for going silent, I got slammed at work with an Alpha release that took all of my mental cycles. This is the first weekend I've had back.

I had not seen that error, I'll spend some time today, seeing if I can do a two-stage commit to maintain the history you're looking for.

[danvk]

Hey there -- I've actually been hacking around on getting this going today, too. Unless you feel strongly, I think it's going to be easiest if I take it from here.

(BTW, did you script conversion of the tests? that would be a very helpful script to have!)

[cthrax]

Alright, I'll let you take it. I did not script that conversion, there were too many inconsistencies between tests, so I just did it by hand.

[danvk]

Wow! I'm impressed (& appreciative!) of the time you must have put into that manual migration.

I'm drilling down into a few remaining Karma failures in my branch. Do you know if there's a simple way to pass a --grep option to the karma runner? Or to open a single test in a real browser? That was always a nice feature of the old dygraphs test system.

[danvk]

I've sent out #565 which supersedes this. Please take a look if you're so inclined!

And a huge thanks for setting this all up. You've helped modernize dygraphs in a big way.

[cthrax]

With karma you can run a reduced set (including one) of tests by placing a ".only" in any describe or it. All tests can be run in a real browser by passing the command line option for the browser or setting it in the karma.conf. I typically will have two different run configurations in my webstorm ide to run in phantomjs or chrome. But you could do it with a parameterized gulp take too.