Add support for test environment

[treppo]

Why not add a test environment?

[joliss]

I generally assume that tests will run in the development environment. When developing (in development mode), you might automatically kick off a test runner after every rebuild, or maybe you keep the test suite open in a window, QUnit/Mocha style.

[treppo]

I added the test mode, because I need a distinct test mode:

I write my scripts and my unit–tests in Coffeescript and with the ES6 module syntax and I use Broccoli to transpile those to Javascript. On this Javascript I execute my tests via jasmine-node in the console. No moving or filtering of HTML or stylesheets is necessary. My development build uses a different workflow and so does my production build.

For a functional/end-to-end testing in the browser I would then use the development environment, but on a unit–test level, it makes sense to have another environment.

What do you think?

[joliss]

Interesting. Can you tell me more about how your development build is different? And if you were to try and move the tests into the development build, would it look terrible?

[joliss]

(I'm willing to accept this PR even with a fairly weak use case - I figure it doesn't hurt to add it.)

[treppo]

Sure, my development build includes SASS stylesheets and HTML files, but no unit tests, while for my test environment it is the other way around. My unit tests need no HTML test runner, as they are run by node via jasmine-node.

Of course some build steps are shared between the test and the development environment, and also with the production environment, but they diverge in a few steps.

Maybe my Brocfile will make things clearer:

module.exports = function (broccoli) {
  var filterCoffeeScript = require('broccoli-coffee');
  var filterES6Modules = require('broccoli-es6-module-filter');
  var pickFiles = require('broccoli-static-compiler');
  var env = require('broccoli-env').getEnv();

  var compileCoffeeScript = function(treeName) {
    var tree = pickFiles(broccoli.makeTree(treeName), {
      srcDir: '/',
      destDir: treeName
    });

    var filteredTree = filterCoffeeScript(tree, {
      bare: true
    })

    return filteredTree
  }

  var scriptTrees = [compileCoffeeScript('src')];
  var result = [];

  if (env === 'test') {
    scriptTrees.push(compileCoffeeScript('test'));
  } else {
    result.push(broccoli.makeTree('public'));
    result.push(broccoli.makeTree('style')); // no SASS filtering yet
  }

  var transpiledScriptTree = filterES6Modules(new broccoli.MergedTree(scriptTrees), {
    moduleType: 'cjs'
  });
  result.push(transpiledScriptTree);

  return result
}

[recipher]

I'd not put any limits on the BROCCOLI_ENV variable at all. Is there a good reason?

We have at least 4 named server environments, each of which has different configuration, for example, the host name for all server requests is different for each environment. I'm planning to use broccoli-replace to set those environment variables at build time.

At the moment, I'm limited to a single environment called "production" unless I use my own brocolli-env fork, or I just look at BROCCOLI_ENV directly.

[treppo]

I agree, it probably would be best to avoid the restrictions altogether. @joliss, what do you think? Was there a concrete reason for it?

[joliss]

The reason was to prevent typos (dev versus development, etc.). I'm not sure it's actually helping. I'm out of bandwidth at the moment, will revisit this at some point.

If we don't do any checking at all, we might just use var env = process.env.BROCCOLI_ENV || 'development' directly in our Brocfiles, instead of using this package. This package has been borderline too-small-to-be-useful in my view.

[recipher]

That's pretty much what I'm going to do (as soon as I need to deploy
somewhere other than production). A typo is easily corrected.

You could wrap that (single line of) code inside broccoli core, to save
having this separate package.

if (broccoli.env === 'test') {
// ...
}

I'd say that knowing the environment is core to broccoli, no need for the
extra package.

On 19 March 2014 23:25, Jo Liss notifications@github.com wrote:

The reason was to prevent typos (dev versus development, etc.). I'm not
sure it's actually helping. I'm out of bandwidth at the moment, will
revisit this at some point.

If we don't do any checking at all, we might just use var env =
process.env.BROCCOLI_ENV || 'development' directly in our Brocfiles,
instead of using this package. This package has been borderline
too-small-to-be-useful in my view.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-38119891
.

[joliss]

if (broccoli.env === 'test') {

You probably don't want to have this as a global variable. That's kindof the problem.

It's not clear to me yet that Broccoli core actually needs to know about the env, so I didn't add API surface (like broccoli.env) for it, and just left it to Brocfiles to add their own notion of environment.

[recipher]

That makes sense. It doesn't really matter if broccoli-env exists or not,
since it's not really doing too much.

On 22 March 2014 13:54, Jo Liss notifications@github.com wrote:

if (broccoli.env === 'test') {

You probably don't want to have this as a global variable. That's kindof
the problem. It's not clear to me yet that Broccoli core actually needs to
know about the env, so I didn't add API surface (like broccoli.env) for
it, and just left it to Brocfiles to add their own notion of environment.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-38352085
.

[treppo]

I just had an idea, while working on a project using Broccoli and Grunt to run the build task:
I think it would make sense for this plug–in to not only have a getEnv() method, but also a setEnv() method, that hides the specific BROCCOLI_ENV variable. When you are not working in a shell, this would make it more pleasant to work with:

// instead of
process.env['BROCCOLI_ENV'] = 'production';

// use this
broccoli.setEnv('production');

// or even something like this, which is only allowing predefined environments
broccoli.setEnv().toProduction();
broccoli.getEnv().isProduction(); // => true

I believe that the change would make this plug–in worthwhile.

[treppo]

Sample implementation:

exports.getEnv = getEnv;
exports.setEnv = setEnv;

function getEnv() {
  var env = process.env.BROCCOLI_ENV || 'development';
  if (env !== 'production' && env !== 'development' && env !== 'test') {
    throw new Error('Environment set to "' + env + '", but only production, development or test are supported at the moment');
  }

  return {
    isProduction: function() { return env === 'production'; },
    isDevelopment: function() { return env === 'development'; },
    isTest: function() { return env === 'test'; }
  };
}

function setEnv() {
  return {
    toProduction: function() { process.env.BROCCOLI_ENV = 'production'; },
    toDevelopment: function() { process.env.BROCCOLI_ENV = 'development'; },
    toTest: function() { process.env.BROCCOLI_ENV = 'test'; }
  };
}

[jakecraige]

I agree with @recipher and @treppo

It's pretty limiting to enforce that we can only have a production or development environment. I have a need to have a staging environment and I can't without forking. I'm sure other people have other sorts of environments they may need.

Do we have a plan for how we are going to handle this @joliss ?

[recipher]

Just retrieve the variable directly. Don't use the plugin.—
Johnny H
07971 880871

On Thu, Apr 24, 2014 at 12:07 AM, Jake Craige notifications@github.com
wrote:

I agree with @recipher and @treppo
It's pretty limiting to enforce that we can only have a production or development environment. I have a need to have a staging environment and I can't without forking. I'm sure other people have other sorts of environments they may need.

Do we have a plan for how we are going to handle this?

Reply to this email directly or view it on GitHub:
#1 (comment)