Add test coverage to xk6-redis

[oleiade]

What

This PR adds integration tests covering the xk6-redis API with the goal to improve its stability and test coverage in preparation of merging it to k6's core. It attempts to test the extension in the context of a script interacting with a Redis instance, and focuses on testing the k6/goja/promises/event loop behavior of the Client, rather than interactions with Redis itself (considering we use a pre-established Redis library under the hood). In certain scenarios, we relied on a more extensive testing strategy in order to explicit the promise-based specific behavior of certain operations that might derive from Redis' behavior itself (rejecting a promise under certain conditions rather than returning zero, for instance).

Things that were left out of the scope of this PR: this PR doesn't test Redis in sentinel mode, nor in cluster mode. The feature should be supported out of the box, but I didn't get to address testing it, yet (if ever).

How

This PR is quite massive. Writing tests for an API as big as Redis' involves, well, lots of code. That was to be expected. However, the test file is organized as follows:

• Every Client command has its own TestClient{Command}( test function. They all follow the same convention, and the same approach, and should be somewhat similar. It's probably not worth reviewing ALL of them, but by reviewing a few of them, the pattern should become explicit, and you will likely be able to spot systemic mistakes and cases I might have missed.
• Two table tests TestClientCommandsAgainstUnreachableServer and TestClientCommandInInitContext verify that, based on the fact that every command will ensure a connection is established under the hood, those should fail when ran against an unreachable host, or when executed in the init context.
• Client.Connect, Client.Close and private methods have been tested separately, and follow a bit of a different testing convention, and it might be reviewing those.

Open questions and Feedback needed

Connect and Close methods

Although the underlying library we used to implement the interactions with Redis does not require to explicitly establish a connection, we need to emulate that in the context of k6. The main reason for is that we want to avoid IO to happen in the init context of our users scripts. Besides, as we want our Redis client to reuse our VUs Dialer and TLSConfig, we need to wait for the VU State to be available before any connection to a Redis instance/cluster is attempted. As a result, I tackled this constraint by implementing explicit Connect and Close methods, as we seem to already have in other extensions/modules such as gRPC's.

Our main design question is: as of today, every client's methods call connect under the hood, to make sure that the Redis client has an underlying client's instance, that is properly configured with the VU's Dialer and TLSConfig. I would like to hear your views on what design you'd prefer: the user must explicitly call the .connect method in the init context, or we keep connecting under the hood automatically?.

Testing approach

We've used an integration testing approach. To avoid importing extra-dependencies, we have developed a dedicated and externally controllable redis mock/stub server (in the spirit of httptest). Running this stub server, we run scripts directly into a test runtime, and assert the outcome directly in the script. Once the script is successfully run, we assert that our redis stub server received the commands we expected in the order we anticipated.

I probably have missed some failure scenarios, some edge cases, or dark corners of the script I'm not yet aware of, or have forgotten 🤷🏻 Please, check out some tests, and let me know if you can think of ways to break them, so I can improve the whole suite.

Finally

Thank you for your time and attention! Please let me know what you think 🙇🏻

Edits

26th of July 2022: Removed mentions of miniredis in favor of the stub server. Explicated testing design strategy.

[mstoykov]

I haven't looked at the majority of the test code but would like to ask:

Have we considered not using a full blown redis in golang as this seems to pull a bunch of dependancies (even just for tests) that would then end up in the core k6 repo.

Arguably we don't really care about the "correct" behaviuour of any of the commands once they are called. So I wonder if we can just have some stubs that just check that it gets what seems like an okay redis command.

This arguably will require some amount of redis wire format parsing and returning of somethign that looks at least somewhat like what redis client should expect.

Running a full blown redis and having the integration tests behind a flag might also be preferable if it will mean that we will not suddenly depend on a lua interpreter for some tests :(

[oleiade]

Changelog since the last review round:

• I have dropped the dependency on miniredis, and replaced it with a custom redis stub server, which listens on a TCP port, talks the redis SPEC protocol, is fully controllable, and reports stuff such has command called, command history, connections counts etc... It has zero external dependency and most importantly allows registering custom command handlers, which you can make reply, well, whatever you need your redis stub server to reply.
• I have rewritten most tests to be dumbed down versions of themselves. They now for the most part, verify that given a bunch of redis commands and promises chains, the server receives expected correctly formatted messages, in the expected order. I kept certain test scenarios a bit richer, to explicit that although redis would not return an error in certain scenarios, our methods would sometimes reject promises.
• Tests have been rewritten to use promise chaining more efficiently. "Wow, I know JavaScript promises now"

[mstoykov]

From a fairly quick over the top look - haven't looked at each test case separately through and through - it LGTM! 👏

I would prefer to get the golangci.yaml update now and remove all the nolint that are possible to be removed. Especially file wide ones.

As general note on tests - it might be better to check the error - like just to be certain it doesn''t actually error with something we are not expecting.

It will be nice if we have the example from the Readme as a test 🤔 Or at least something like it - that does something more.

The stub looks a bit big and I did not have time to look it over all that much, sorry - maybe if this stays open longer I might go through it more.

You have a general 👍 from me, if you have something specific you want my opinion on - I can try to look into it more, but otherwise I don't see any reason to block merging this. We can always fix something that turned out to not be as a great down the line.

p.s. I thought about the fact that a lot of the tests almost look at the same and can be made into table ones ... but that likely will not make them any easier to read so ... I am kind of against that idea.

[oleiade]

Alright, I've addressed the feedback I received, most notably:

• the file is now called stub_test.go
• the redisServer is now gone in favor of just StubServer
• the promise err that are expected to fail are now checked
• the connect method is now private and close is gone

[codebien]

LGTM 👏 🎉