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