Add Redis integration tests #225
Conversation
nfriedly
left a comment
nfriedly
left a comment
There was a problem hiding this comment.
Hey, thanks, this is awesome!
I have a couple of thoughts below, but nothing too major.
test/redis-integration-test.ts
Outdated
| // If SCRIPT LOAD, send to all master nodes | ||
| if (command[0] === 'SCRIPT' && command[1] === 'LOAD') { | ||
| const nodes = client.nodes('master') | ||
| await Promise.all( | ||
| nodes.map(async (node) => | ||
| node.call(command[0], ...command.slice(1)), | ||
| ), | ||
| ) | ||
| // Return the result from one of them (they should be identical) | ||
| const result = await client.call(command[0], ...command.slice(1)) | ||
| return result as RedisReply | ||
| } |
There was a problem hiding this comment.
Hum, the change I made was to avoid needing something like this, but I suppose that was for node-redis. Is there something we could do to make the same thing easier with ioredis?
Otherwise we should probably add this to the documentation.
There was a problem hiding this comment.
Also, could we get the result from the Promise.all return value instead of doing an extra client.call at the end?
There was a problem hiding this comment.
You're absolutely right. The manual broadcasting with Promise.all was specifically needed because ioredis (unlike node-redis) doesn't automatically route SCRIPT LOAD commands to the correct node when using raw commands, causing NOSCRIPT errors in cluster mode. [Check my first commit] (Workflow failed)
I’m thinking of a better solution 🤔
There was a problem hiding this comment.
Wdyt about this? c0a3621
Hum.. I'm not sure. I think I like the earlier solution better. I'm concerned that fallback is going to end up being the most common case if we let it happen silently; I'd prefer that we do something to ensure the script is loaded onto each server. Maybe do both in parallel?
Also, this is getting farther away from the original intent of this PR - would you mind making a new branch and PR with c0a3621, then removing it from this one?
| "test:unit": "jest test/store-test.ts", | ||
| "test:integration": "jest test/redis-integration-test.ts", | ||
| "test:lib": "jest", | ||
| "test": "run-s lint test:*", | ||
| "test": "run-s lint test:lib", |
There was a problem hiding this comment.
I think this could be simplified by dropping test:lib:
| "test:unit": "jest test/store-test.ts", | |
| "test:integration": "jest test/redis-integration-test.ts", | |
| "test:lib": "jest", | |
| "test": "run-s lint test:*", | |
| "test": "run-s lint test:lib", | |
| "test:unit": "jest test/store-test.ts", | |
| "test:integration": "jest test/redis-integration-test.ts", | |
| "test": "run-s lint test:*", |
There was a problem hiding this comment.
Or, alternatively, if we want npm test to work without a local redis server, make it this:
| "test:unit": "jest test/store-test.ts", | |
| "test:integration": "jest test/redis-integration-test.ts", | |
| "test:lib": "jest", | |
| "test": "run-s lint test:*", | |
| "test": "run-s lint test:lib", | |
| "test:unit": "jest test/store-test.ts", | |
| "test:integration": "jest test/redis-integration-test.ts", | |
| "test": "run-s lint test:unit", |
…nnection checks and streamlining sendCommandCluster implementation
| @@ -0,0 +1,13 @@ | |||
| version: '3' | |||
There was a problem hiding this comment.
Is the Compose file required if we are already mentioning services in the CI file?
There was a problem hiding this comment.
We actually don’t need a compose file, but it is required (or recommended) for quickly deploying a Redis cluster on local.
Add Docker Compose configuration and Redis integration tests
docker-compose.ymlfile to define Redis and Redis Cluster services.This enhances the testing environment and ensures compatibility with Redis functionalities.