dial-once/node-cache-manager-redis

Test for storing null value not being run

jeff-kilbride opened this issue · 4 comments

I was working on the zlib stuff a bit tonight and I noticed an error in the current test suite. The 'should store a null value without error' under the set tests is incorrectly contained inside another test clause.

  it('should store a value with a infinite ttl', function (done) {
    redisCache.set('foo', 'bar', {ttl: 0}, function (err) {
      assert.equal(err, null);
      redisCache.ttl('foo', function (err, ttl) {
        assert.equal(err, null);
        assert.equal(ttl, -1);
        done();
      });
    });

    it('should store a null value without error', function (done) {
      redisCache.set('foo2', null, function (err) {
        try {
          assert.equal(err, null);
          redisCache.get('foo2', function (err, value) {
            assert.equal(err, null);
            assert.equal(value, null);
            done();
          });
        } catch (e) {
          done(e);
        }
      });
    });
  });

Notice that it is contained inside the previous it test. Looking at the mocha output, this test is not being run. This is a problem, because when I fixed it, it failed.

Error: value cannot be null
    at Object.self.set (node-cache-manager-redis/index.js:187:17)
    a == null
AssertionError: Error: value cannot be null
    at Object.self.set (index.js:187:17)
    a == null
    at test/lib/redis-store-spec.js:100:16
    at Object.self.set (index.js:187:14)
    at Context.<anonymous> (test/lib/redis-store-spec.js:98:16)

This makes sense, because the default isCacheableValue returns false for null values:

return value !== null && value !== undefined;

If you want to store null values without an error, the default isCacheableValue should be changed to just:

return value !== undefined;

If this is correct, I can make these changes and submit a PR.

Hey @jeff-kilbride, thanks for the report

I think that we should stick to specs and allow null storage - what do you think guys? @dial-once/developers

@PuKoren Ok, just let me know how you want to proceed. I've got the fixes in a separate branch, so I can submit a PR whenever you're ready.

@jeff-kilbride whenever you want, a separate PR would be nice

Fixed by #50