faeldt / redis-mock

Node.js redis client mock
112 stars 157 forks source link

Correcting list and typing #10

Closed lahabana closed 11 years ago

lahabana commented 11 years ago

There was a problem in the list implementation and the results were not stored as strings but as their native types. This wasn't coherent and is now corrected

faeldt commented 11 years ago

1) brpop should block until the end of the timeout: TypeError: Cannot read property 'length' of undefined

Know what causes this?

lahabana commented 11 years ago

I had a look into it. At first I thought it was a node version specific bug but after testing on the same version I didn't get this error (v0.8.22). So I looked deeper and I think the error is not reproducible because it depends on how the tests are ran. The test looks like:

it("should block until the end of the timeout", function(done) {

    var r = redismock.createClient("", "", "");
    var time = false;

    r.brpop("foo", 500, function(err, result) {

        console.log("Waiting for timeout...");
        should.not.exist(result);
        time.should.equal(true);
        done();
    });

    setTimeout(function() {time = true}, 400);
});

So the key that causes problem is "foo". I looked and two other tests redis-mock.server.js and redis-mock.strings.js use this same key. This shouldn't be an issue however.

So I looked a bit more at the set() method:

exports.set = function (mockInstance, key, value, callback) {

    mockInstance.storage[key] = value + "";

    if (callback) {
        process.nextTick(function () {
            callback(null, "OK");
        });
    }
}

And the error in the tests makes sense as for strings we don't use items so elt.list is undefined thus the error.

So there's 2 ways to fix this:

Personally I'm for the second option. What do you think?

lahabana commented 11 years ago

what's your opinion then?

faeldt commented 11 years ago

Thanks for the research and sorry for the late reply.

If I run the following

var redis = require('redis');

var client = redis.createClient();

client.set('test', 'test', function(err, result) {

        client.brpop('test', 500, function(err, result) {

                console.log(err);
                console.log(result);
        });
});

It gets me the following:

[Error: ERR Operation against a key holding the wrong kind of value] undefined

So I guess this is the behavior we want to replicate in terms of brpop etc no?

lahabana commented 11 years ago

Yes.