Handle Dialyzer warnings (#81)

* Correcting spec for eredis:q and eredis:qp
  A gen_tcp:send / ssl:send can return errors like
    {'error','enotconn'}
    {'error', {'tls_alert', {'certificate_expired', _}}}
  which propagates to the user.
* Update spec for return_value()
  A multibulk can contain a null value, like '$-1\r\n' which is an 'undefined' in the return_value().
* Correcting spec for eredis_parser:parse/2
  The function will return {error, unknown_response} when an unknown type is found while parsing.

* Check eredis:stop(C) with pattern matching in tests
* Correcting specs in eredis_test_utils
* Remove a redundant testcase in eredis_parser_tests

* Run dialyzer on the test profile during `make dialyzer`
  This makes sure we run dialyzer on testcode during development and in CI.

Makefile

@@ -72,7 +72,7 @@ xref:
 	@rebar3 xref
 
 dialyzer:
-	@rebar3 dialyzer
+	@rebar3 as test dialyzer
 
 elvis:
 	@elvis rock

include/eredis.hrl

@@ -19,7 +19,7 @@
 -type options() :: [option()].
 -type server_args() :: options().               % for backwards compatibility
 
--type return_value() :: undefined | binary() | [binary() | nonempty_list()].
+-type return_value() :: undefined | binary() | [binary() | nonempty_list() | undefined].
 
 -type pipeline() :: [iolist()].
 

src/eredis.erl

@@ -142,7 +142,7 @@ stop(Client) ->
     eredis_client:stop(Client).
 
 -spec q(Client::client(), Command::[any()]) ->
-          {ok, return_value()} | {error, Reason::binary() | no_connection}.
+          {ok, return_value()} | {error, Reason::any() | no_connection}.
 %% @doc: Executes the given command in the specified connection. The
 %% command must be a valid Redis command and may contain arbitrary
 %% data which will be converted to binaries. The returned values will
@@ -151,15 +151,14 @@ q(Client, Command) ->
     call(Client, Command, ?TIMEOUT).
 
 -spec q(Client::client(), Command::[any()], Timeout::timeout()) ->
-          {ok, return_value()} | {error, Reason::binary() | no_connection}.
+          {ok, return_value()} | {error, Reason::any() | no_connection}.
 %% @doc Like q/2 with a custom timeout.
 q(Client, Command, Timeout) ->
     call(Client, Command, Timeout).
 
 
 -spec qp(Client::client(), Pipeline::pipeline()) ->
-          [{ok, return_value()} | {error, Reason::binary()}] |
-          {error, no_connection}.
+          [{ok, return_value()} | {error, Reason::any() | no_connection}].
 %% @doc: Executes the given pipeline (list of commands) in the
 %% specified connection. The commands must be valid Redis commands and
 %% may contain arbitrary data which will be converted to binaries. The
@@ -168,8 +167,7 @@ qp(Client, Pipeline) ->
     pipeline(Client, Pipeline, ?TIMEOUT).
 
 -spec qp(Client::client(), Pipeline::pipeline(), Timeout::timeout()) ->
-          [{ok, return_value()} | {error, Reason::binary()}] |
-          {error, no_connection}.
+          [{ok, return_value()} | {error, Reason::any() | no_connection}].
 %% @doc Like qp/2 with a custom timeout.
 qp(Client, Pipeline, Timeout) ->
     pipeline(Client, Pipeline, Timeout).

src/eredis_parser.erl

@@ -41,6 +41,7 @@ init() ->
           {ok, return_value(), Rest::binary(), NewState::#pstate{}} |
           {error, ErrString::binary(), NewState::#pstate{}} |
           {error, ErrString::binary(), Rest::binary(), NewState::#pstate{}} |
+          {error, unknown_response} |
           {continue, NewState::#pstate{}}.
 
 %% @doc: Parses the (possibly partial) response from Redis. Returns

test/eredis_parser_tests.erl

@@ -156,11 +156,6 @@ bulk_nil_and_rest_test() ->
     B = <<"$-1\r\n$3\r\nfoo\r\n">>,
     ?assertEqual({ok, undefined, <<"$3\r\nfoo\r\n">>, init()}, parse(init(), B)).
 
-%% parse_bulk function tests
-parse_bulk_test() ->
-    B = <<"$3\r\nbar\r\n">>,
-    ?_assertEqual({ok, <<"bar">>, <<>>}, parse(init(), B)).
-
 parse_bulk_too_much_data_in_continuation_test() ->
     B1 = <<"$1\r\n">>,
     B2 = <<"1\r\n$1\r\n2\r\n$1\r\n3\r\n">>,

test/eredis_sentinel_SUITE.erl

@@ -66,7 +66,7 @@ t_connect_with_explicit_options(Config) when is_list(Config) ->
 t_stop(Config) when is_list(Config) ->
     process_flag(trap_exit, true),
     C = c_sentinel(),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _, _} -> died
              after 1000 -> still_alive end,
     process_flag(trap_exit, false),
@@ -97,7 +97,7 @@ t_connection_failure_during_start_reconnect(Config) when is_list(Config) ->
     IsDead = receive {'EXIT', C, _} -> died
              after 400 -> still_alive end,
     ?assertEqual(still_alive, IsDead),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead2 = receive {'EXIT', _Pid, _Reason} -> died
               after 1000 -> still_alive end,
     process_flag(trap_exit, false),
@@ -115,7 +115,7 @@ t_reconnect_success_on_sentinel_process_exit(Config) when is_list(Config) ->
     eredis:q(C1, ["CLIENT", "KILL", "TYPE", "NORMAL"]),
     timer:sleep(100),
     ?assert(is_process_alive(whereis(mymaster))),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _Pid, _Reason} -> died
              after 400 -> still_alive end,
     process_flag(trap_exit, false),
@@ -137,7 +137,7 @@ t_reconnect_success_on_sentinel_connection_break(Config) when is_list(Config) ->
     timer:sleep(100),
     {links, LinkedPids3} = process_info(whereis(mymaster), links),
     ?assertMatch(2, length(LinkedPids3)),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _Pid, _Reason} -> died
              after 400 -> still_alive end,
     process_flag(trap_exit, false),
@@ -159,7 +159,7 @@ connect_eredis_sentinel(Options) ->
     ?assertMatch({ok, _}, Res),
     {ok, C} = Res,
     ?assertMatch({ok, [<<"master">> | _]}, eredis:q(C, ["ROLE"])),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _Pid, _Reason} -> died
              after 400 -> still_alive end,
     process_flag(trap_exit, false),

test/eredis_tcp_SUITE.erl

@@ -159,7 +159,7 @@ t_connect_local(Module, Config) when is_list(Config) ->
 t_stop(Config) when is_list(Config) ->
     process_flag(trap_exit, true),
     C = c(),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _, _} -> died
              after 1000 -> still_alive end,
     process_flag(trap_exit, false),
@@ -173,15 +173,15 @@ t_get_set(Config) when is_list(Config) ->
     ?assertEqual({ok, undefined}, eredis:q(C, ["GET", foo])),
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", foo, bar])),
     ?assertEqual({ok, <<"bar">>}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_set_get_term(Config) when is_list(Config) ->
     C = c(),
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", term])),
 
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", term, C])),
     ?assertEqual({ok, term_to_binary(C)}, eredis:q(C, ["GET", term])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_delete(Config) when is_list(Config) ->
     C = c(),
@@ -190,7 +190,7 @@ t_delete(Config) when is_list(Config) ->
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", foo, bar])),
     ?assertEqual({ok, <<"1">>}, eredis:q(C, ["DEL", foo])),
     ?assertEqual({ok, undefined}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_mset_mget(Config) when is_list(Config) ->
     C = c(),
@@ -204,7 +204,7 @@ t_mset_mget(Config) when is_list(Config) ->
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["MSET" | lists:flatten(KeyValuePairs)])),
     ?assertEqual({ok, ExpectedResult}, eredis:q(C, ["MGET" | Keys])),
     ?assertMatch({ok, _}, eredis:q(C, ["DEL" | Keys])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_exec(Config) when is_list(Config) ->
     C = c(),
@@ -222,7 +222,7 @@ t_exec(Config) when is_list(Config) ->
     ?assertEqual({ok, ExpectedResult}, eredis:q(C, ["EXEC"])),
 
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", "k1", "k2"])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_exec_nil(Config) when is_list(Config) ->
     C1 = c(),
@@ -234,8 +234,8 @@ t_exec_nil(Config) when is_list(Config) ->
     ?assertEqual({ok, <<"QUEUED">>}, eredis:q(C1, ["GET", "x"])),
     ?assertEqual({ok, undefined}, eredis:q(C1, ["EXEC"])),
     ?assertMatch({ok, _}, eredis:q(C1, ["DEL", "x"])),
-    ?assertMatch(ok, eredis:stop(C1)),
-    ?assertMatch(ok, eredis:stop(C2)).
+    ok = eredis:stop(C1),
+    ok = eredis:stop(C2).
 
 t_pipeline(Config) when is_list(Config) ->
     C = c(),
@@ -264,7 +264,7 @@ t_pipeline(Config) when is_list(Config) ->
                  eredis:qp(C, P3, 5000)),
 
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", a, b])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_pipeline_mixed(Config) when is_list(Config) ->
     C = c(),
@@ -280,15 +280,15 @@ t_pipeline_mixed(Config) when is_list(Config) ->
           end),
     timer:sleep(10),
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", c, d])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_q_noreply(Config) when is_list(Config) ->
     C = c(),
     ?assertEqual(ok, eredis:q_noreply(C, ["GET", foo])),
     ?assertEqual(ok, eredis:q_noreply(C, ["SET", foo, bar])),
     %% Even though q_noreply doesn't wait, it is sent before subsequent requests:
     ?assertEqual({ok, <<"bar">>}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_q_async(Config) when is_list(Config) ->
     C = c(),
@@ -304,7 +304,7 @@ t_q_async(Config) when is_list(Config) ->
             ?assertEqual(Msg2, {ok, <<"bar">>}),
             ?assertMatch({ok, _}, eredis:q(C, ["DEL", foo]))
     end,
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_undefined_database(Config) when is_list(Config) ->
     t_undefined_database(eredis, Config).
@@ -411,13 +411,13 @@ t_unknown_client_call(Config) when is_list(Config) ->
     C = c(),
     Request = {test},
     ?assertEqual(unknown_request, gen_server:call(C, Request)),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_unknown_client_cast(Config) when is_list(Config) ->
     C = c(),
     Request = {test},
     ?assertEqual(ok, gen_server:cast(C, Request)),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_tcp_closed(Config) when is_list(Config) ->
     {ok, C} = eredis:start_link([{reconnect_sleep, 1000}]),
@@ -426,7 +426,7 @@ t_tcp_closed(Config) when is_list(Config) ->
     tcp_closed_rig(C),
     timer:sleep(100), % Instant reconnect. No sleep before the first attempt.
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", foo], 500)),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_connect_no_reconnect(Config) when is_list(Config) ->
     t_connect_no_reconnect(eredis, Config).

test/eredis_test_utils.erl

@@ -26,7 +26,7 @@ get_tcp_ports(Pid) ->
 
 %% Start a server port and run Fun when a client connect.
 -spec start_server(Fun :: fun((ClientSocket::inet:socket()) -> ok)) ->
-          {ok, inet:port_number()}.
+          {ok, pid(), inet:port_number()}.
 start_server(Fun) ->
     Pid = spawn_link(?MODULE, start_server, [self(), Fun]),
     Port = receive_from(Pid, 5000),
@@ -35,7 +35,8 @@ start_server(Fun) ->
 %% Stop server and close client socket as well if needed.
 -spec stop_server(Pid :: pid()) -> ok.
 stop_server(Pid) ->
-    Pid ! shutdown.
+    Pid ! shutdown,
+    ok.
 
 %% Wait for a client to connect to server
 -spec await_connect(Pid :: pid()) -> ok.

test/eredis_tls_SUITE.erl

@@ -49,7 +49,7 @@ suite() -> [{timetrap, {minutes, 3}}].
 t_tls_connect(Config) when is_list(Config) ->
     C = c_tls(),
     process_flag(trap_exit, true),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     IsDead = receive {'EXIT', _, _} -> died
              after 1000 -> still_alive end,
     process_flag(trap_exit, false),
@@ -77,15 +77,15 @@ t_tls_get_set(Config) when is_list(Config) ->
     ?assertEqual({ok, undefined}, eredis:q(C, ["GET", foo])),
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", foo, bar])),
     ?assertEqual({ok, <<"bar">>}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_tls_closed(Config) when is_list(Config) ->
     C = c_tls(),
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", foo], 5000)),
     tls_closed_rig(C),
     timer:sleep(1300), %% Wait for reconnection (1000ms)
     ?assertMatch({ok, _}, eredis:q(C, ["DEL", foo], 5000)),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_tls_connect_database(Config) when is_list(Config) ->
     ExtraOptions = [{database, 2}],
@@ -95,14 +95,14 @@ t_tls_connect_database(Config) when is_list(Config) ->
     ?assertEqual({ok, undefined}, eredis:q(C, ["GET", foo])),
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", foo, bar])),
     ?assertEqual({ok, <<"bar">>}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 t_tls_1_2_cert_expired(Config) when is_list(Config) ->
     ExtraOptions = [],
     CertDir = "tls_expired_client_certs",
     C = c_tls(ExtraOptions, CertDir, [{versions, ['tlsv1.2']}]),
     ?assertMatch({error, no_connection}, eredis:q(C, ["GET", foo])),
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 
 -ifdef(OTP_RELEASE).
 -if(?OTP_RELEASE >= 22).
@@ -119,7 +119,7 @@ t_tls_1_3_cert_expired(Config) when is_list(Config) ->
         {error, {tls_alert, {certificate_expired, _}}} -> ok;
         {error, no_connection} -> ok
     end,
-    ?assertMatch(ok, eredis:stop(C)).
+    ok = eredis:stop(C).
 -endif.
 -endif.
 
@@ -153,7 +153,7 @@ t_soon_expiring_cert(Config) when is_list(Config) ->
     ?assertEqual({ok, <<"OK">>}, eredis:q(C, ["SET", foo, bar3])),
     ?assertEqual({ok, <<"bar3">>}, eredis:q(C, ["GET", foo])),
     ct:pal(user, "Stopping client"),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
 
     %% Reconnect, will give ok during connect+handshake
     %% but trigger a ssl_error that makes the client try reconnect
@@ -165,7 +165,7 @@ t_soon_expiring_cert(Config) when is_list(Config) ->
     timer:sleep(200),
 
     ?assertEqual({error, no_connection}, eredis:q(C2, ["SET", foo, bar4])),
-    ?assertMatch(ok, eredis:stop(C2)).
+    ok = eredis:stop(C2).
 
 %% Make sure a reconnect cleanup old sockets
 %% i.e we have maximum 1 tcp/tls port open
@@ -177,7 +177,7 @@ t_reconnect(Config) when is_list(Config) ->
     C = c_tls(ExtraOptions),
     timer:sleep(1000), %% expect a couple of reconnect attempts
     ?assert(length(get_tcp_ports()) =< 1),
-    ?assertMatch(ok, eredis:stop(C)),
+    ok = eredis:stop(C),
     timer:sleep(200), %% Wait for reconnect process to terminate
     ?assertEqual(0, length(get_tcp_ports())).