OpenRiak / riak_test

I'm in your cluster, testing your riaks
http://basho.github.com/riak_test/
0 stars 0 forks source link

Pass dialyzer for riak_test #23

Closed martinsumner closed 2 months ago

martinsumner commented 4 months ago

Depends on updates to the riak erlang clients, most issues with dialyzer were related to types not being correctly or completely defined within these clients.

Most of the remaining changes are related to removing dead/unused code paths, or converting assertions that can never fail (according to types) to explicit pattern matches.

In some cases there is hidden magic within clients that allow incorrect types to work, and be tested. For example with using string or binary as content type for PUT, the test has been changed to reflect the advertised type.

Some deprecated stuff has been removed, rather than having the effort of fixing it:

To run dialyzer in OTP 26 uncomment the no_unknown warning in rebar.config for check profile. As riak_test does not explicitly load riak, then any internal riak functions referred to in a test will be unknown.

There are two cases of explicitly allowing dialyzer errors:

martinsumner commented 2 months ago

Now updated to make the otp version specific rebar.config changes via rebar.config.script - so no edits required to run ./rebar3 as check do dialyzer on both OTP24 and OTP26.

Should now be clean for both OTP versions.

tburghart commented 2 months ago

Regarding tests/job_enable_common.erl:

This is very old code, and I wouldn't take my 8-year-old comments as gospel today ;)

The only functional difference I see if that rt:httpc/1 invokes rt:wait_for_service(Node, riak_kv) along the way, so whatever I ran into may have been a timing issue that would be better handled further up the stack.

My inclination would be to remove the -dialyzer exclusion, change the code to that which doesn't require it, and add a comment that there could be a potential timing issue that should be handled in Node setup ... which may not even be relevant given that all of the rt:deploy_nodes(...) code has itself been thoroughly reworked.

This also eliminates the need for inclusion of rhc/include/rhc.hrl.

Possible (entirely untested) replacement:

make_url(Node, Parts) ->
    make_url(Node, http, Parts).

%% @hidden
%% Previous implementations of this function noted reliability issues that
%% may have been due to timing and could benefit from assuring that
%%  `rt:wait_for_service(Node, riak_kv)'
%% is invoked before this function.
make_url(Node, http = Scheme, Parts) ->
    {ok, {IP, Port}} = rt:get_http_conn_info(Node),
    make_url(Scheme, IP, Port, Parts);
make_url(Node, https = Scheme, Parts) ->
    {ok, {IP, Port}} = rt:get_https_conn_info(Node),
    make_url(Scheme, IP, Port, Parts).

make_url(Scheme, Host, Port, Parts) ->
    lists:flatten([io_lib:format("~s://~s:~b", [Scheme, Host, Port]), Parts]).
tburghart commented 2 months ago

My results:

# git status
On branch nhse-d34-wdaymerge-dialyzer
Your branch is up to date with 'origin/nhse-d34-wdaymerge-dialyzer'.
# rebar3 --version
rebar 3.22.1 on Erlang/OTP 24 Erts 12.3.2.17
# ls -l riak
... riak -> ../../wday/riak-3.2/riak
# rebar3 as check dialyzer
Warnings written to _build/check/24.3.4.17.dialyzer_warnings
Warnings occurred running dialyzer: 43

Caveats:

Results file attached.

24.3.4.17.dialyzer_warnings

tburghart commented 2 months ago

I was able to hack together a build of Riak from the nhse-develop-3.4 branch using our eleveldb build but still commenting out the riak_kv and riak_repl deps in the check profile. This gets me down to 37 warnings: 24.3.4.17.dialyzer_warnings

martinsumner commented 2 months ago

I think all dialyzer warnings should now be resolved