move test suite to mocha or tap

[bcoe]

we have fancy pants badges now \o/

I moved tests around a bit and got them running on travis/coveralls (see #776), in the issue we discussed replacing the hand rolled artisanal test suite with a fancy new tool.

The main requirement that I'd like to carry over form the refactoring work, is that we run the suite against hiredis and javascript parsers -- I think it's great to know that the whole test suite is running in both modes -- we should be able to pull this off with either testing framework.

I use mocha on a lot of my projects, but I'd like to put a vote in for trying out node-tap:

https://github.com/isaacs/node-tap

@isaacs has been doing a ton of work on node-tap, and I think its syntax might be a bit closer to the existing test suite?

@erinspice how would you like to divide up work on this?

[raydog]

I'm a big fan of mocha, so I'm a bit biased, but I do feel mocha would be closer to the original test suite. Each test runs, and the assert library is used for validation and to throw exceptions on failure. Node-tap, AFAICT mixes the runner with the assertion library, meaning that to correctly use that lib to it's maximum potential, each assertion would have to updated. With mocha, only the runner bits would change, meaning we can still use the same assertion lib. We would only have to convert every:

tests.MULTI_2 = function () {
  // ...
  next();
};

into

it("multi() with array args", function (done) {
  // ...
  done();
});

Just my 2 cents.

[erinishimoticha]

I am also a big Mocha fan. I would also not be opposed to moving to a more flexible assertion library. I really, really like Chai's Expect.

[erinishimoticha]

Here's a quick example I put together that uses mocha. It starts redis-server before and stops it after the tests run. We could also add various Redis config files to to test node_redis with Redis running in different modes.

[bcoe]

@erinspice this is awesome, could we add this slight tweak:

describe('redis', function () {
  ['javascript', 'hiredis'].forEach(function (parser) {

    it('IPV4', function (done) {
      var ipv4Client = redis.createClient( PORT, "127.0.0.1", { family : "IPv4", parser: parser } );

      ipv4Client.once("ready", function start_tests() {
          console.log("Connected to " + ipv4Client.address + ", Redis server version " + ipv4Client.server_info.redis_version + "\n");
          console.log("Using reply parser " + ipv4Client.reply_parser.name);

          ipv4Client.quit();
          return done();
      });

      ipv4Client.on('end', function () {

      });

      // Exit immediately on connection failure, which triggers "exit", below, which fails the test
      ipv4Client.on("error", function (err) {
          console.error("client: " + err.stack);
          throw err;
      });
    });

  });
});

This way we run the whole test suite on hiredis and javascript.

[bcoe]

@erinspice we could probably also drop this line:

redis.debug_mode = process.argv[2] ? JSON.parse(process.argv[2]) : false;

And switch to redis.debug_mode = process.env.DEBUG ? JSON.parse(process.env.DEBUG) : false;

And then we don't need to play around with process.argv for anything, which I think runs afoul of Mocha.

I'm really happy with the approach you're taking (it's cool how you pulled the assertions into their own helper), also I have 1,000,000 open tickets on yargs I promised to work on this weekend; do you want to run ahead with the refactor, and I can help test and give a thorough code review of the finished product?

[erinishimoticha]

Agreed. I've added that to the mocha tests. I don't think I'll change that line in the old tests, since they don't run with mocha. Hopefully we'll be able to completely delete them soon.

I've added the DEBUG switch and made those two tests run on each combination of javascript, hiredis, ipv4, and ipv6.

[erinishimoticha]

@bcoe How do we add coverage reporting to the mocha tests?

[bcoe]

@erinspice nyc mocha, rather than nyc ./test/run.s, once you've got everything moved over -- coverage should stay at 86%, which is a good test that the port went well.

[erinishimoticha]

Whoa, nice, even nyc grunt run-tests works.

[bcoe]

@erinspice Isaac built that part of nyc, it's amazingly clever -- looks for the node bin and replaces it with a new node-bin that adds coverage tracking.

As a representative from npm, can I vote for using package.json's scripts stanza rather than grunt for kicking off our mocha suite? once you delete the weird .sh file I added, I think you should be able to simply run mocha -- seems like the tests were written in a pretty darn portable way, even if rolling their own test runner.

[erinishimoticha]

Sure, we can use scripts.

[erinishimoticha]

@bcoe I wrote a Node script to start Redis, run the tests and shut Redis down, and it broke nyc. Is there a trick to it? About to convert it to bash and see if that fixes it.

[bcoe]

@erinspice I'd have to see, I can't see why it would cause an issue. However, my personal preference would be to avoid starting a Redis server as part of the tests, I think it's a reasonable expectation that a developer will have Redis on their machine -- and this could interfere with Travis which already provisions a Redis process.

[bcoe]

@erinspice I could have my opinion swayed, some of my coworkers love bootstrapping a daemon at the start of tests -- I usually tend to like having something up and running on my machine already.

[erinishimoticha]

Here's a version without Grunt, still bootstrapped. I really like that this doesn't interfere with any Redis processes a user may have had running previously.

With that branch, the nyc ouput looks like this:

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
----------|----------|----------|----------|----------|----------------|
All files |      100 |      100 |      100 |      100 |                |
----------|----------|----------|----------|----------|----------------|

With Grunt, it looks normal.

[brycebaril]

See https://github.com/NodeRedis/node_redis/tree/refactor242 for how I was converting tests to tape a while ago -- I had to fix a lot of the tests to get them working right. Some of the work there may be helpful in fixing some of the more errant, broken, or racy tests.

While I'm not particularly a fan of Mocha, I'm definitely a fan of getting the tests fixed up and am very happy to see that torch picked up.

[erinishimoticha]

Thanks, @brycebaril! We've done even more work and moved the branch over here to node_redis.

[BridgeAR]

I like that you want to implement mocha. While still being in the planing phase might I suggest using promises instead of async for the tests (they'll need less code and therefor should be better to read) and use npm as the build tool?

[blainsmith]

Nice work with the mocha tests and removing grunt. One less thing to deal with. I've been a big fan of using npm as a build tool.

[blainsmith]

Should we close this since #793 was merged?

[bcoe]

\o/