redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

Allow to run the test suite without Toxiproxy #89

Closed voxik closed 1 year ago

voxik commented 1 year ago

Trying to package redis-client for Fedora, there is no Toxiproxy available in Fedora and there is more reasons why it is not really feasible to have it "somehow" available just for the tests. While it seems that currently most of the test cases are directed towards Toxiproxy, they could IMHO run directly against Redis. E.g. this test fails without Toxyproxy available:

$ ruby -Ilib:test -e 'Dir.glob "./test/redis_client/middlewares_test.rb", &method(:require)' -- -n /test_failing_call_instrumentation/
starting redis... started with pid=1798
Waiting for redis (port 16380)... ready.
Running test suite with driver: RedisClient::RubyConnection
Run options: -n /test_failing_call_instrumentation/ --seed 33075

# Running:

E

Finished in 0.003621s, 276.1378 runs/s, 0.0000 assertions/s.

  1) Error:
RedisClient::MiddlewaresTest#test_failing_call_instrumentation:
RedisClient::CannotConnectError: Connection refused - connect(2) for 127.0.0.1:16379
    /usr/share/ruby/socket.rb:1217:in `__connect_nonblock'
    /usr/share/ruby/socket.rb:1217:in `connect_nonblock'
    /usr/share/ruby/socket.rb:60:in `connect_internal'
    /usr/share/ruby/socket.rb:141:in `connect'
    /usr/share/ruby/socket.rb:645:in `block in tcp'
    /usr/share/ruby/socket.rb:231:in `each'
    /usr/share/ruby/socket.rb:231:in `foreach'
    /usr/share/ruby/socket.rb:635:in `tcp'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client/ruby_connection.rb:49:in `initialize'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:664:in `new'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:664:in `block in connect'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client/middlewares.rb:12:in `connect'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/test/redis_client/middlewares_test.rb:106:in `connect'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:663:in `connect'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:657:in `raw_connection'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:624:in `ensure_connected'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/lib/redis_client.rb:208:in `call'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/test/support/client_test_helper.rb:38:in `setup'
    /builddir/build/BUILD/redis-client-0.12.1/usr/share/gems/gems/redis-client-0.12.1/test/redis_client/middlewares_test.rb:15:in `setup'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

But just simple change of ports such as:

$ sed -i '/REDIS.*79/ s/79/80/' test/support/servers.rb

Let the test case succeed:

$ ruby -Ilib:test -e 'Dir.glob "./test/redis_client/middlewares_test.rb", &method(:require)' -- -n /test_failing_call_instrumentation/
starting redis... started with pid=1819
Waiting for redis (port 16380)... ready.
Running test suite with driver: RedisClient::RubyConnection
Run options: -n /test_failing_call_instrumentation/ --seed 19990

# Running:

.

Finished in 0.004641s, 215.4873 runs/s, 646.4620 assertions/s.

1 runs, 3 assertions, 0 failures, 0 errors, 0 skips

So I wonder, would it be possible to run most of the parts of the test suite without Toxiproxy and use Toxiproxy only when necessary?

byroot commented 1 year ago

would it be possible to run most of the parts of the test suite without Toxiproxy and use Toxiproxy only when necessary?

On paper yes, but it means connecting to a different port when you do need it, which makes the test suite much more convoluted.

So I'd rather not bother with this complexity.

There's also the question of the value of getting a green build if you skip what I consider to be very important tests. At this point why even bother running the test suite?

I'll close this as I don't plan to make any change here. If you feel strongly about it and can come up with a PR that doesn't make the test suite particularly more complex, I'll seriously consider it.

voxik commented 1 year ago

would it be possible to run most of the parts of the test suite without Toxiproxy and use Toxiproxy only when necessary?

On paper yes, but it means connecting to a different port when you do need it, which makes the test suite much more convoluted.

So I'd rather not bother with this complexity.

That is understandable.

There's also the question of the value of getting a green build if you skip what I consider to be very important tests. At this point why even bother running the test suite?

Well, for downstream distribution, I think it is always better to run at least some tests then no test at all. No tests is the worst scenario. And currently, there seems to be ~330 test cases and ~20 failing due to missing Toxiproxy. I still believe that the test result is quite representative and for the rest, I will trust upstream that the library does what it is supposed to do. I think that 99.9% of users who directly downloads this library just "trusts", so I still believe that we do much better in Fedora.

I'll close this as I don't plan to make any change here. If you feel strongly about it and can come up with a PR that doesn't make the test suite particularly more complex, I'll seriously consider it.

No worries. I'll deal with it somehow. ATM, I'll do something like:

sed -i '/Toxiproxy\[/i\
      skip' test/redis_client/connection_test.rb

considering to write some Toxiproxy mock just to avoid the need of changing the files.

Of course, the ideal state would be to have Toxiproxy in Fedora, but that would be too big detour unfortunately. I can just hope that community will find Toxiproxy valuable enough to bring it into Fedora and make this discussion moot.