ngbp / warlock

A sophisticated meta build system written in JavaScript. Created by @joshdmiller
http://www.getwarlock.com
MIT License
19 stars 5 forks source link

Warlock Karma Spell #2

Closed Merott closed 10 years ago

Merott commented 10 years ago

Hello again,

Hopefully I won't start to become an annoyance by asking too much from a build system that's still in its early stages of development. I'm sure you have "Karma Spell" written somewhere near the top of your Warlock to-do list, but in any case, I took a stab at it.

So, I made an attempt to build a Karma spell, and it appears to me that at least one challenge is how to ensure correct loading order of js files, while still providing some default globs. For an AngularJS app, the final desired order would be something like below:

"globs": {
   "jasmine": [
      "<%= paths.build_js %>/angular.js",
      "<%= vendor/angular-mocks/angular-mocks.js",
      "<%= paths.build_js %>/**/*.js",
      "<%= paths.source_app %>/**/*.spec.js"
   ]
},

For example, if by default, we wanted to have <%= paths.build_js %>/**/*.js and <%= paths.source_app %>/**/*.spec.js in the list of globs, and let the user add the rest, how could that be achieved? Do you have any solutions/suggestions/workarounds?

And for the sake of completeness, here is the plugin:

karma = require 'gulp-karma'

module.exports = (warlock) ->
  runKarma = (options) ->
    options.action = "run"
    options.configFile ?= warlock.file.joinPath __dirname, "../config/karma-unit.js"
    karma(options)

  warlock.flow 'karma-to-run',
    source: [ '<%= globs.jasmine %>' ]
    source_options:
      base: "<%= paths.source_app %>"
    tasks: [ 'webapp-build' ]
    depends: [ 'flow::scripts-to-build' ]

  .add(50, 'karma-run', runKarma)

I'm sure somewhere in your docs or in the code I read something about load ordering not being supported yet. It's either that I can't remember where it was, or it's finally bed time (I'm in the UK, by the way, so it's just past midnight).

I hate to ask about ETAs on open source projects, where clearly the maintainer has to give up his free time for. But, may I, just this one time, ask if you have a Karma Spell in the pipeline?

joshdmiller commented 10 years ago

You have a long way to go before becoming an annoyance; I am finding this early feedback incredibly valuable.

Coincidentally, I spent a few hours Sunday morning working on a Karma plugin; I think I'll post a "to do" list on the wiki so the community can divide and conquer. I started in a similar place as you, but ran into the same challenge you did - as well as a couple extras I hadn't anticipated

I eventually dropped the gulp-karma plugin for a couple of reasons. The primary one being that it doesn't make sense to read in the files twice (once by gulp-karma and again by Karma itself). Inherently, it just doesn't feel like a "flow" to me.

This is an important point to consider before continuing. Karma isn't like the vast majority of spells a project will use; it doesn't read in something, do something with it, and then put it someplace else. What we actually want to do is get some file names, properly order them, figure out the right configuration, and then launch a child process or spawn a server to run some unit tests. This is completely different. I mention this both to keep in mind that this isn't a typical flow and to make sure you're aware this is a complicated spell for you to have started with. :-)

Anyway, I just simplified into just a task. And then I thought about the config file. In order to have everything configurable - and inheritable - I didn't like the idea of using a config file by default. If the user wants to change one setting, they have to rewrite all of them from the config file. Warlock already has a mechanism for customizing this very kind of configuration with support for inheritance.

So I then had something like this:

karma = require "karma"
gs = require "glob-stream"

module.exports = ( warlock ) ->
  warlock.task.add 'karma-single-run', ( callback ) ->
    options = warlock.config ( "tasks.karma.single-run" )
    globs = warlock.util.mout.array.flatten warlock.config.process [
      "<%= paths.build_js %>/**/*.js"
      "<%= globs.source.specs %>"
    ]

    warlock.streams.highland( gs.create( globs ) )
    .toArray ( files ) ->
      options.files = files.map ( file ) -> file.path
      karma.server.start options, ( code ) ->
        warlock.verbose.log "Karma exited with code #{code}."
        callback code

And the package.json:

{
  "warlock": {
    "globs": {
      "source": {
        "test": [ "<%= paths.source_app %>/**/*.spec.js" ]
      }
    },
    "tasks": {
      "karma": {
        "single-run": {
          "frameworks": [ "jasmine" ],
          "preprocessors": [
            "karma-jasmine",
            "karma-firefox-launcher",
            "karma-chrome-launcher",
            "karma-phantomjs-launcher"
          ],
          "reporters": "dots",
          "browsers": [ "Firefox" ],
          "singleRun": true,
          "autoWatch": false
        }
      }
    }
  }
}

Note: I think users should be able to specify a config file that overwrites the settings, but that's an improvement for another day. Right now, if you add the tasks.karma.single-run.configFile key, Karma will load and use the configuration file, but the config settings provided by Warlock will overwrite them due to the order Karma processes them. This is an easy enough fix, but doesn't currently exist.

This works, except for the problem you mentioned: file ordering. I solved this in the webapp spell through a little stream magic. It would be silly to duplicate that here, but I don't think we have to. Fundamentally, we have four kinds of resources: app files, test files, vendor files, and test plugins/mocks/etc. I think it's safe to assume they should be processed in the same order regardless of the individual application: vendor, app, plugins, tests. If I export the sorting algorithm used in the webapp spell, the first two are already in the desired order. If we use two additional arrays, we can handle the rest of them. We can use globs.source.test for the tests, as I did above, and globs.vendor.test for the plugins. Using a tiny amount of stream magic, the task can be rewritten like so:

karma = require "karma"
globStream = require "glob-stream"
webappSpell = require "warlock-spell-webapp"
File = require "vinyl"

module.exports = ( warlock ) ->
  gs = ( globs ) ->
    globs = warlock.util.mout.array.flatten warlock.config.process globs
    warlock.streams.highland( globStream.create( globs ) ).map (f) -> new File f

  warlock.task.add 'karma-single-run', [ "flow::scripts-to-build" ], ( callback ) ->
    options = warlock.config ( "tasks.karma.single-run" )
    buildGlobs = [ "<%= paths.build_js %>/**/*.js" ]
    testGlobs = [
      "<%= globs.vendor.test %>"
      "<%= globs.source.test %>"
    ]

    buildStream = gs( buildGlobs ).collect()
      .invoke( 'sort', [ webappSpell.sortVendorFirst warlock.config "globs.vendor.js" ] )
      .flatten()
      .toArray ( buildFiles ) ->
        testStream = gs( testGlobs ).toArray ( testFiles ) ->
          options.files = buildFiles.concat( testFiles )
            .filter( ( file ) -> file and file?.path )
            .map( ( file ) -> file.path )

          karma.server.start options, ( code ) ->
            warlock.verbose.log "Karma exited with code #{code}."
            callback code

Then there's the problem of framework-specific addons like angular-mocks.js that would go in globs.vendor.test. We can't embed these into the system as defaults because Warlock supports more than just AngularJS. The most logical place is for the AngularJS spell to require it; unfortunately, the angular-mocks.js file has to come from Bower, so we can't require it there. A "coming soon" feature is Bower integration to solve for things like this; in the meantime, one must add it to her own warlock.json:

{
  "globs": {
    "vendor": {
      "test": [
        "vendor/angular-mocks/angular-mocks.js"
      ]
    }
  }
}

This isn't ideal, but it's the same procedure one must follow to include AngularJS in the first place. And again, this will soon be solved through Bower integration.

What's missing right now is that this task will not run during webapp-build because there's no dependency on it. While meta tasks can be declared using Flows, there's no way to do that with tasks - yet. That will need to be a change to core.

Anyway, this works. It needs a few bells and whistles, but it's essentially fully functional. I'll try to push this up later today so y'all can play with it and maybe find some ways to improve it. Again, because running Karma is not really stream-based, it feels very weird. If you take a look at the gulp-karma code, you'll probably get the same impression. Unlike most tasks, this feels more in Grunt's wheelhouse. But we'll get there over time.

Sidebar: For what it's worth, we could accomplish the above with two flows (build files and test files) that merge before calling karma. I may see if this makes the code more comprehensible; if so, I'll post an update here.

Merott commented 10 years ago

Hi Josh,

Thanks for the detailed explanations. I noticed Karma not being a flow as such, so I knew I picked the wrong challenge, but at least it had me play around with Warlock quite a bit.

About having a default Karma config file, I think the options parameter passed to gulp-karma would overwrite any options set in the config file, at least that's what I assumed, but never verified. In any case, I agree that having the options in package.json (and warlock.json) is cleaner, and also acts as a reference for available options.

I was thinking, is it really a bad idea to have angular-mocks.js added to the test globs pattern by the Karma spell? Afterall, The globs won't match any files that don't exist, and if someone has angular-mocks in his vendor directory, it would be safe to assume they're using AngularJS?

Lastly, it seems that you already have the loading order issue sorted out, but I think I'd at least mention what I was thinking just a little earlier - just thinking out loud here. Warlock already makes use of priority numbers to order tasks, why not do the same for globs? Assume that the Karma spell defines the test glob as below:

"test": [
      "<%= paths.build_js %>/angular.js::10",
      "<%= vendor/angular-mocks/angular-mocks.js::11",
      "<%= paths.source_app %>/**/*.spec.js"
]

Globs could then be sorted, having those with priority numbers appear first, in order of priority, followed by everything else in their original order. Is that even possible, and maintainable?

joshdmiller commented 10 years ago

You're right about the config file being overwritten - I mentioned that in the "Note" above. The solution is simple - the task can load in the config file if provided and merge it manually, deleting the key. This has the same effect as Karma natively, but with the order switched.

You're also right about Warlock ignoring angular-mocks.js if it doesn't exist, but what happens when we add additional spells for non-Angular frameworks like Ember? We've now put the onus on the Karma spell to support the many frameworks and to keep up with any changes in how they handle testing; I think the onus should be on the framework-specific spells. My vision is that installing warlock-spell-angular will install the necessary Bower components and add the necessary files to the globs.vendor arrays, including the framework itself and its required test files. This makes it truly automated. Bower support is on the "to do" list, but I'm thinking something like this in a spell's package.json:

{
  "warlock": {
    "libraries": {
      "angular": "*",
      "jquery": "1.4.2"
    }
  }
}

Your idea about glob ordering is very interesting. I'll have to think about that one...

Merott commented 10 years ago

Understood your point about framework-specific spells managing their own globs, and agreed.

Regarding bower integration, I'm not sure how I'd feel about letting the build system manage my app dependencies. I think I'd prefer to manually maintain bower.json and libraries used in my app, and have the build system take care of only build-related tasks. Obviously a personal preference, and like all preferences, subject to change! Having the choice however, would be nice.

Thanks for the Karma plugin code. I'm looking forward to the spell.

joshdmiller commented 10 years ago

Okay, here's the flow-based version. Let me know which you find easier to comprehend:

module.exports = ( warlock ) ->
  # FIXME: This should be pulled in entirely from webappSpell
  sortFilesByVendor = ( options, stream ) ->
    stream.collect()
      .invoke( 'sort', [ webappSpell.sortVendorFirst warlock.config "globs.vendor.js" ] )
      .flatten()

  startKarmaServer = warlock.streams.highland.wrapCallback karma.server.start

  warlock.flow 'karma-get-vendor',
    source: [
      '<%= globs.vendor.test %>'
      '<%= globs.source.test %>'
    ]
    source_options:
      read: false
    merge: "flow::karma-single-run::20"

  warlock.flow 'karma-single-run',
    source: [ '<%= paths.build_js %>/**/*.js' ]
    source_options:
      read: false
    depends: [ 'flow::scripts-to-build' ]
    tasks: [ 'webapp-build' ]

  .add( 10, 'karma-sort-vendor', sortFilesByVendor, { raw: true } )
  .add( 50, 'karma.single-run', ( options, stream ) ->
    stream.collect().map( ( files ) ->
      options.files = files
        .filter( ( file ) -> file and file?.path )
        .map( ( file ) -> file.path )

      startKarmaServer options
    ).flatten()
  , { raw: true } )
  .add( 51, 'karma.reporter', ( options ) ->
    warlock.streams.map ( code ) ->
      warlock.verbose.log "Karma exited with code #{code}."
      code
  )

The only reason for a the separate karma-get-vendor flow is to ensure the correct file ordering because the sources from karma-single-run need to be re-ordered after read. I find this all very clunky. As @Merrott indicated, I think we need a more permanent fix to file ordering.

Note: You can't run either karma spell I've posted because they require changes to spell-webapp that I haven't pushed, and this one requires a couple of bug fixes to core.

Merott commented 10 years ago

Although both ways of doing it appear somewhat complicated I find the flow-based version slightly easier to follow.

joshdmiller commented 10 years ago

I think we can make the flow version a little simpler:

module.exports = ( warlock ) ->
  sortFilesByVendor = ( options, stream ) ->
    stream.invoke 'sort', [
      webappSpell.sortVendorFirst warlock.config "globs.vendor.js"
    ]

  runKarma = ( options ) ->
    warlock.streams.map ( files ) ->
      options.files = files
        .filter( ( file ) -> file and file?.path )
        .map( ( file ) -> file.path )
      warlock.streams.wrapCallback( karma.server.start )( options )

  warlock.flow 'karma-get-vendor',
    source: [
      '<%= globs.vendor.test %>'
      '<%= globs.source.test %>'
    ]
    source_options:
      read: false
    merge: "flow::karma-single-run::20"

  warlock.flow 'karma-single-run',
    source: [ '<%= paths.build_js %>/**/*.js' ]
    source_options:
      read: false
    depends: [ 'flow::scripts-to-build' ]
    tasks: [ 'webapp-build' ]

  .add( 10, 'karma-sort-vendor', sortFilesByVendor, { raw: true, collect: true } )
  .add( 50, 'karma.single-run', runKarma, { collect: true } )
  .add( 51, 'karma.reporter', ( options ) ->
    warlock.streams.map ( code ) ->
      warlock.verbose.log "Karma exited with code #{code}."
      code
  )

The "stream magic" with the raw option was to get all the files passed through the stream all at once; if we were to offer that as an API option ({ collect: true }) and put that logic behind the scenes, the flow gets a little easier to follow, I think. collect essentially makes the stream get called once with an array of all values passed through the stream, rather than being called once for each file.

Does that make it any better?

joshdmiller commented 10 years ago

Boom! I just pushed warlock-spell-karma and published it to NPM. I also updated example-angularjs to use it. To show off the feature, there is also a karma.conf.js file in the root directory that will be loaded, overwriting spell defaults - but I consider that feature "experimental".