🕸️ Implement cluster commands

[tumile]

🚧 WIP: Cluster API

This changeset will resolve #37 🚀

Commands to implement and test

• cluster_addslots
• cluster_bumpepoch
• cluster_countfailurereports
• cluster_countkeysinslot
• cluster_delslots
• cluster_failover
• cluster_flushslots
• cluster_forget
• cluster_getkeysinslot
• cluster_info
• cluster_keyslot
• cluster_meet
• cluster_myid
• cluster_nodes
• cluster_replicate
• cluster_reset
• cluster_saveconfig
• cluster_setconfigepoch
• cluster_setslot
• cluster_slaves
• cluster_replicas
• cluster_slots
• readonly
• readwrite

Additional requirements

• Update README
• Match coding style and argument names

[tumile]

I didn't implement CLUSTER BUMPEPOCH and CLUSTER SET-CONFIG-EPOCH because per the Redis docs, config epoch management should be performed internally by the cluster. Looking at jedis and go-redis, they left it out as well. Let me know if you think they should be in the API.

[tumile]

The cluster tests need to be run against a cluster enabled redis-server, which will interfere with other tests since in cluster mode some functionalities are disabled. So for now I don't include them in redis_test.ts, but they can be run separately. As I investigate go-redis, they spin up standalone redis-servers for testing with suitable mode. @uki00a What is the best way to move forward with the tests here?

[uki00a]

First of all, thanks for submitting this PR @tumile!

Let me know if you think they should be in the API.

Personally, I don't think it needs to be included in the API.

they spin up standalone redis-servers for testing with suitable mode.

That is exactly what I want to do. (#104)
Evetually, I want to run the cluster tests in the CI 😄

[tumile]

I see. Has there been any plan for #104? This PR can wait till then don't you think?

[uki00a]

@tumile

Has there been any plan for #104?

I added startRedisServer() function to tests/test_util.ts. (#113)
With it, is it possible to include the cluster tests in CI?

[tumile]

I'm thinking about adding a startRedisCluster or something to, well, start multiple servers for the tests based on your work. I'll update as soon as I get it done.

[tumile]

I currently use a timeout at the end of file to clean up the redis cluster after tests in cluster_test.ts. This is a problem with other long running tests like pubsub_test.ts or bzpopmin timeout, the cluster may be cleaned up before the cluster tests have a chance to run. My workaround now is to set the cleanup timeout really high (20s) to make sure every tests have run. This is unsound.

https://github.com/tumile/deno-redis/blob/6942c252a67ecaaf74c3280c7300a0577cdef802/tests/cluster_test.ts#L108-L112

Is there a way to ensure all tests have run before cleanup? I tried putting cleanup in another test, but Deno complains about leaking.

[Terkwood]

https://deno.land/manual/testing

You may have looked at this already, and it's my first time reading it, but I don't see any simple/prepackaged way to accomplish what you're asking. Will probably require a creative hack or some third party test util(if such a thing exists)?

[tumile]

There is an option to run each test file in serial, so test timeouts will not interfere with others. I don't think this is a bad idea, with #114 it's probably good to put all commands tests in a single file and start a redis instance. Tests like cluster and pubsub, which require customized instances, will be run independently.

Still, this doesn't solve using timeout for cleanup, but a test will not suddenly fail if other ones take longer than expected.

some third party test util(if such a thing exists)

I don't think there is a test framework like mocha or ava for Deno yet. Never realized how much we needed them 😂.

[uki00a]

Is there a way to ensure all tests have run before cleanup? I tried putting cleanup in another test, but Deno complains about leaking.

How about the following? 🤔

async function makeClusterTest(): Promise<{
  test: (name: string, fn: () => Promise<void>) => void,
  client: Redis,
  runTests: () => Promise<void>
}> {
  const cleanupCluster = await startRedisCluster();
  const client = await connect({ hostname: "127.0.0.1", port: 7000 });
  const tests = [] as Array<{ name: string, fn: () => Promise<void> }>;

  function test(name: string, fn: () => Promise<void>): void {
    tests.push({ name, fn });
  }

  function runTests(): Promise<void> {
    const promises = [] as Promise<void>[];

    function defineTest(name: string, fn: () => Promise<void>): void {
      Deno.test(name, () => {
        const promise = fn();
        promises.push(promise);
        return promise;
      });
    }

    for (const test of tests) {
      defineTest(test.name, test.fn);
    }

    return Promise.allSettled(promises).finally(() => {
      cleanupCluster();
      client.close();
    });
  }

  return { test, client, runTests };
}

const { test, client, runTests } = await makeClusterTest();

test("[cluster] flushslots", async () => {
  ...
});

...

runTests();

[tumile]

Looks promising, I'll test it in a bit. How about wrapping the makeTest/makeClusterTest in a class? Probably cleaner.

[tumile]

@uki00a can you review my test suite implementation? I think if it's good, we can roll this out to other tests as well!

[uki00a]

Thanks @tumile!
I've left a few comments on this PR.
Please check them out!

[tumile]

I've addressed the comments 😁. Please review again

[uki00a]

LGTM, thanks @tumile!

[uki00a]

Released in v0.11.0!