feat(config): Gulp Server Watch for Mocha

[mleanos]

Adds a Gulp task that watches all server files (including server tests),
and upon any changes it will perform the Gulp test:server task.

Also, includes an optional argument for the task that can specify
that if the changed file is a test, then it will only run that test
file.

Example usage: gulp test:server:watch --onlyChanged

[mleanos]

This is a pretty handy feature that I've been using with a project based off meanjs. It really speeds development & encourages proper TDD considerations.

Normal test execution:

When the --onlyChanged option is set & the changed file is a test:

Another:

[lirantal]

Sounds like a great addition. @ilanbiala is the gulp guy.

[mleanos]

@ilanbiala There were a few things here that I was unsure of; quick review?

In particular, I don't have much experience with Glob. I didn't see a real straight forward way of getting an individual test file from the testAssets.tests.server array of glob patterns. So this is the best way I saw fit. I'll point out a couple things in line notes.

[mleanos]

@ilanbiala What do you think of this section overall?

Here, I'm iterating through the glob patterns because, since it's an array, technically a project could have multiple locations/patterns for tests.

The tests param should probably be renamed to something like patterns, or locations

[ilanbiala]

@mleanos this shouldn't all be one gulp task in my opinion. Ideally gulp tasks are composed, just like sass, less, watch, etc. which all do one thing and are used in other places to make general utility tasks.

I think it would be better to specify this functionality in the general watch task and just pare down the functionality a bit. I see the potential time value of just running the affected file tests, but when other things depend on others, you may still have issues, so I would just run all the tests anyway.

[mleanos]

@ilanbiala Have you taken this for a test drive? I suggest that you do, so you understand the benefits a bit more.

As for it being included in the other Watch task, this is a separate task that runs without any other dependencies; it's not linked to any other Gulp task.

The way that I use this, is to have it running in a terminal as I'm developing.. anytime I save a server file, the task invokes the existing tests:server task. If everything passes, I continue developing. With this flow, you can see that it should be an isolated task on it's own; there's no need to couple it with the server task, for instance.

And the --onlyChanged is just an option.. it's up to the developer if they want to run all tests, or just a changed test file.

[mleanos]

@ilanbiala After looking at the watch task again, I see what you mean. I think this new watch configuration can be added there. We would still use the new test:server:watch task, in order to run the server tests initially, and set the NODE_ENV to test

gulp.task('test:server:watch', function (done) {
  runSequence('test:server', 'watch', done);
});

This is what it would look like added to the existing watch task

if (process.env.NODE_ENV === 'test') {
    // Add Server Test file rules
    gulp.watch([testAssets.tests.server, defaultAssets.server.allJS], ['test:server']).on('change', function (file) {
      var runOnlyChangedTestFile = argv.onlyChanged ? true : false;

      // check if we should only run a changed test file
      if (runOnlyChangedTestFile) {
        var changedTestFiles = [];

        // iterate through server test glob patterns
        _.forEach(testAssets.tests.server, function (tests) {
          // determine if the changed (watched) file is a server test
          _.forEach(glob.sync(tests), function (f) {
            var filePath = path.resolve(f);

            if (filePath === path.resolve(file.path)) {
              changedTestFiles.push(f);
            }
          });
        });

        // set task argument for tracking changed test files
        argv.changedTestFiles = changedTestFiles;
      }

      plugins.livereload.changed();
    });
  }

[ilanbiala]

Still making the watch a little too powerful in my opinion. Also, I don't think adding argv parsing to gulp is a great idea because it seems unnatural in gulp. Is there a better way you can think of? What would creating a separate task for watch changed and not watch changed look like?

[mleanos]

@ilanbiala I don't understand what you mean by "too powerful".. What's the point of Gulp, if not to help manage mundane development tasks? Think of the development workflow that you have to follow to save a file, and then run the tests, and keep repeating that process.

And what is unnatural about adding argv parsing to a Gulp task? I'd like to understand what you mean, but I'm just not following.

[ilanbiala]

Instead of argv parsing for different tasks, Gulp seems to prefer just creating a different task with a different name.

Tasks that are alone too powerful do too much in the function and don't compose the functionality from other tasks that should do one specific task well.

[mleanos]

@ilanbiala I removed the new Gulp task, and moved the watch into the main Gulp watch task.

The optional argument that I'm using argv to track, is not specifying that a different task should be run. It's determining whether or not all server tests should be run, or just the modified file that triggered the watch.on('change') event. In other words, the argument is only affecting the tests to be ran with Mocha here https://github.com/meanjs/mean/pull/1083/files#diff-b9e12334e9eafd8341a6107dd98510c9R222

Having a separate task that runs just one server test file, and then having one that runs all server tests, would be duplicate code. I think the use of argv is the clean way of handling a requirement such as this.

As for your concern over the watch definition being "too powerful", I think the purpose of the watch callback is for tasks that need special handling. I believe this is such a case.

How would you approach this differently, given these requirements?

1. Start a Gulp task that initially runs all Server Tests
2. Now watch for any changed Server files
3. Upon the above watch being triggered, run all Server Tests; with an option that the user specifies to just run a single Server Test, if the changed file that triggers the watch.on('change') event is a Server Test file.

I'm open to suggestions. However, this feels like the most straight-forward, and clean, way of satisfying the above requirements.

[ilanbiala]

@lirantal @codydaig any suggestions/feedback?

[lirantal]

nothing special from me

[codydaig]

@ilanbiala I haven't followed this too closely. But the concept of being able to watch test files and either run a single test from a passed in arg or an entire suite of tests is awesome.

[mleanos]

@ilanbiala Any more feedback on this?

I've been using this implementation for development, and it's immensely useful. It makes TDD so much faster. I think others will see it as a very useful tool for productivity.

[ilanbiala]

I have finals until next week, so I can't really spend any time on other stuff until then. Ping me then.

[mleanos]

SGTM. Thanks.

[mleanos]

@ilanbiala Let's get this merged in, so we can keep going on enhancing the Gulp configuration.

[ilanbiala]

~

[ilanbiala]

never mind, just comma change, so this is fine

[ilanbiala]

Last couple fixes, and then I'll merge.

[mleanos]

@ilanbiala I made the change to the yargs dependency.

I left the use of lodash's .forEach, as it seems to be more favorable & efficient than the native Array.forEach.

[ilanbiala]

LGTM. @lirantal @codydaig any comments before I merge?

[mleanos]

@ilanbiala I noticed, after the fact, that you also pointed out the dependency of supertest. The diff makes it look like I added it, but it was already there. If we want to make a sweeping change to use ~ on all dev dependencies, then that can be a dedicated PR.

[ilanbiala]

@mleanos yep, I commented on that before, so it's fine.

[codydaig]

@mleanos This needs a blurb in the documentation, but that's a separate PR to a separate repo.

LGTM

[mleanos]

@ilanbiala Can we get this merged?

[ilanbiala]

Sorry about the delay, just merged it in.

[mleanos]

No worries. I jhst have some other work I wanna do with Gulp.

Thanks!