Merge pull request #6825 from lafirest/fix/exhook_cover

fix(emqx_exhook): improve test coverage of the emqx_exhook

apps/emqx_exhook/src/emqx_exhook_api.erl

@@ -194,15 +194,9 @@ exhooks(get, _) ->
     {200, Infos};
 
 exhooks(post, #{body := Body}) ->
-    case emqx_exhook_mgr:update_config([exhook, servers], {add, Body}) of
-        {ok, _} ->
-            #{<<"name">> := Name} = Body,
-            get_nodes_server_info(Name);
-        {error, Error} ->
-            {500, #{code => <<"BAD_RPC">>,
-                    message => Error
-                   }}
-    end.
+    {ok, _} = emqx_exhook_mgr:update_config([exhook, servers], {add, Body}),
+    #{<<"name">> := Name} = Body,
+    get_nodes_server_info(Name).
 
 action_with_name(get, #{bindings := #{name := Name}}) ->
     get_nodes_server_info(Name);

apps/emqx_exhook/src/emqx_exhook_metrics.erl

@@ -110,8 +110,8 @@ on_server_deleted(Name) ->
 
 -spec server_metrics(server_name()) -> metrics_info().
 server_metrics(SvrName) ->
-    Hooks = ets:match(?HOOKS_METRICS,
-                      {metrics, {SvrName, '_'}, '_', '_', '_', '_', '_'}),
+    Hooks = ets:match_object(?HOOKS_METRICS,
+                             {metrics, {SvrName, '_'}, '_', '_', '_', '_', '_'}),
 
     Fold = fun(#metrics{succeed = Succeed,
                         failed = Failed,
@@ -155,8 +155,8 @@ servers_metrics() ->
 
 -spec hooks_metrics(server_name()) -> hooks_metrics().
 hooks_metrics(SvrName) ->
-    Hooks = ets:match(?HOOKS_METRICS,
-                      {metrics, {SvrName, '_'}, '_', '_', '_', '_', '_'}),
+    Hooks = ets:match_object(?HOOKS_METRICS,
+                             {metrics, {SvrName, '_'}, '_', '_', '_', '_', '_'}),
 
     Fold = fun(#metrics{index = ?INDEX(_, HookPoint),
                         succeed = Succeed,

apps/emqx_exhook/src/emqx_exhook_mgr.erl

@@ -149,8 +149,8 @@ pre_config_update(_, {add, Conf}, OldConf) ->
 
 pre_config_update(_, {update, Name, Conf}, OldConf) ->
     case replace_conf(Name, fun(_) -> Conf end, OldConf) of
-                    not_found -> {error, not_found};
-                    NewConf -> {ok, NewConf}
+        not_found -> {error, not_found};
+        NewConf -> {ok, NewConf}
     end;
 
 pre_config_update(_, {delete, ToDelete}, OldConf) ->
@@ -433,7 +433,7 @@ ensure_reload_timer(none, Waiting, Stopped, TimerRef) ->
 ensure_reload_timer({Name, #{auto_reconnect := Intv}, Iter},
                     Waiting,
                     Stopped,
-                    TimerRef) ->
+                    TimerRef) when is_integer(Intv) ->
     Next = maps:next(Iter),
     case maps:is_key(Name, TimerRef) of
         true ->

apps/emqx_exhook/test/emqx_exhook_SUITE.erl

@@ -28,7 +28,19 @@ exhook {
   servers = [
     { name = default,
       url = \"http://127.0.0.1:9000\"
-    }]
+    },
+    { name = enable,
+      enable = false,
+      url = \"http://127.0.0.1:9000\"
+    },
+    { name = error,
+      url = \"http://127.0.0.1:9001\"
+    },
+    { name = not_reconnect,
+      auto_reconnect = false,
+      url = \"http://127.0.0.1:9001\"
+    }
+  ]
 }
 ">>).
 
@@ -104,6 +116,122 @@ t_access_failed_if_no_server_running(_) ->
                  emqx_exhook_handler:on_message_publish(Message)),
     emqx_exhook_mgr:enable(<<"default">>).
 
+t_lookup(_) ->
+    Result = emqx_exhook_mgr:lookup(<<"default">>),
+    ?assertMatch(#{name := <<"default">>, status := _}, Result),
+    not_found = emqx_exhook_mgr:lookup(<<"not_found">>).
+
+t_list(_) ->
+    [H | _] = emqx_exhook_mgr:list(),
+    ?assertMatch(#{name := _,
+                   status := _,
+                   hooks := _}, H).
+
+t_unexpected(_) ->
+    ok = gen_server:cast(emqx_exhook_mgr, unexpected),
+    unexpected = erlang:send(erlang:whereis(emqx_exhook_mgr), unexpected),
+    Result = gen_server:call(emqx_exhook_mgr, unexpected),
+    ?assertEqual(Result, ok).
+
+t_timer(_) ->
+    Pid = erlang:whereis(emqx_exhook_mgr),
+    refresh_tick = erlang:send(Pid, refresh_tick),
+    _ = erlang:send(Pid, {timeout, undefined, {reload, <<"default">>}}),
+    _ = erlang:send(Pid, {timeout, undefined, {reload, <<"not_found">>}}),
+    _ = erlang:send(Pid, {timeout, undefined, {reload, <<"error">>}}),
+    ok.
+
+t_error_update_conf(_) ->
+    Path = [exhook, servers],
+    Name = <<"error_update">>,
+    ErrorCfg = #{<<"name">> => Name},
+    {error, _} = emqx_exhook_mgr:update_config(Path, {update, Name, ErrorCfg}),
+    {error, _} = emqx_exhook_mgr:update_config(Path, {move, Name, top, <<>>}),
+    {error, _} = emqx_exhook_mgr:update_config(Path, {enable, Name, true}),
+
+    ErrorAnd = #{<<"name">> => Name, <<"url">> => <<"http://127.0.0.1:9001">>},
+    {ok, _} = emqx_exhook_mgr:update_config(Path, {add, ErrorAnd}),
+
+    DisableAnd = #{<<"name">> => Name, <<"url">> => <<"http://127.0.0.1:9001">>, <<"enable">> => false},
+    {ok, _} = emqx_exhook_mgr:update_config(Path, {add, DisableAnd}),
+
+    {ok, _} = emqx_exhook_mgr:update_config(Path, {delete, <<"error">>}),
+    {ok, _} = emqx_exhook_mgr:update_config(Path, {delete, <<"delete_not_exists">>}),
+    ok.
+
+t_error_server_info(_) ->
+    not_found = emqx_exhook_mgr:server_info(<<"not_exists">>),
+    ok.
+
+t_metrics(_) ->
+    ok = emqx_exhook_metrics:succeed(<<"default">>, 'client.connect'),
+    ok = emqx_exhook_metrics:failed(<<"default">>, 'client.connect'),
+    true = emqx_exhook_metrics:update(1000),
+    timer:sleep(100),
+    SvrMetrics = emqx_exhook_metrics:server_metrics(<<"default">>),
+    ?assertMatch(#{succeed := _, failed := _, rate := _, max_rate := _}, SvrMetrics),
+
+    SvrsMetrics = emqx_exhook_metrics:servers_metrics(),
+    ?assertMatch(#{<<"default">> := #{succeed := _}}, SvrsMetrics),
+
+    HooksMetrics = emqx_exhook_metrics:hooks_metrics(<<"default">>),
+    ?assertMatch(#{'client.connect' := #{succeed := _}}, HooksMetrics),
+    ok.
+
+t_handler(_) ->
+    %% connect
+    {ok, C} = emqtt:start_link([{host, "localhost"},
+                                {port, 1883},
+                                {username, <<"gooduser">>},
+                                {clientid, <<"exhook_gooduser">>}]),
+    {ok, _} = emqtt:connect(C),
+
+    %% pub/sub
+    {ok, _, _} = emqtt:subscribe(C, <<"/exhook">>, qos0),
+    timer:sleep(100),
+    ok = emqtt:publish(C, <<"/exhook">>, <<>>, qos0),
+    ok = emqtt:publish(C, <<"/ignore">>, <<>>, qos0),
+    timer:sleep(100),
+    {ok, _, _} = emqtt:unsubscribe(C, <<"/exhook">>),
+
+    %% sys pub/sub
+    ok = emqtt:publish(C, <<"$SYS">>, <<>>, qos0),
+    {ok, _, _} = emqtt:subscribe(C, <<"$SYS/systest">>, qos1),
+    timer:sleep(100),
+    {ok, _} = emqtt:publish(C, <<"$SYS/systest">>, <<>>, qos1),
+    ok = emqtt:publish(C, <<"$SYS/ignore">>, <<>>, qos0),
+    timer:sleep(100),
+    {ok, _, _} = emqtt:unsubscribe(C, <<"$SYS/systest">>),
+
+    %% ack
+    {ok, _, _} = emqtt:subscribe(C, <<"/exhook1">>, qos1),
+    timer:sleep(100),
+    {ok, _} = emqtt:publish(C, <<"/exhook1">>, <<>>, qos1),
+    timer:sleep(100),
+    emqtt:stop(C),
+    timer:sleep(100),
+    ok.
+
+t_simulated_handler(_) ->
+    ClientInfo = #{clientid => <<"user-id-1">>,
+                   username => <<"usera">>,
+                   peerhost => {127,0,0,1},
+                   sockport => 1883,
+                   protocol => mqtt,
+                   mountpoint => undefined
+                  },
+    %% resume/takeover
+    ok = emqx_exhook_handler:on_session_resumed(ClientInfo, undefined),
+    ok = emqx_exhook_handler:on_session_discarded(ClientInfo, undefined),
+    ok = emqx_exhook_handler:on_session_takenover(ClientInfo, undefined),
+    ok.
+
+t_misc_test(_) ->
+    "5.0.0" = emqx_exhook_proto_v1:introduced_in(),
+    <<"test">> = emqx_exhook_server:name(#{name => <<"test">>}),
+    _ = emqx_exhook_server:format(#{name => <<"test">>, hookspec => #{}}),
+    ok.
+
 %%--------------------------------------------------------------------
 %% Utils
 %%--------------------------------------------------------------------

apps/emqx_exhook/test/emqx_exhook_api_SUITE.erl

@@ -37,7 +37,9 @@ exhook {
 ">>).
 
 all() ->
-    [t_list, t_get, t_add, t_move_1, t_move_2, t_delete, t_hooks, t_update].
+    [ t_list, t_get, t_add, t_move_top, t_move_bottom
+    , t_move_before, t_move_after, t_delete, t_hooks, t_update
+    ].
 
 init_per_suite(Config) ->
     application:load(emqx_conf),
@@ -131,7 +133,15 @@ t_add(Cfg) ->
 
     ?assertMatch([<<"default">>, <<"test1">>], emqx_exhook_mgr:running()).
 
-t_move_1(_) ->
+t_move_top(_) ->
+    Result = request_api(post, api_path(["exhooks", "default", "move"]), "",
+                         auth_header_(),
+                         #{position => top, related => <<>>}),
+
+    ?assertMatch({ok, <<>>}, Result),
+    ?assertMatch([<<"default">>, <<"test1">>], emqx_exhook_mgr:running()).
+
+t_move_bottom(_) ->
     Result = request_api(post, api_path(["exhooks", "default", "move"]), "",
                          auth_header_(),
                          #{position => bottom, related => <<>>}),
@@ -139,7 +149,7 @@ t_move_1(_) ->
     ?assertMatch({ok, <<>>}, Result),
     ?assertMatch([<<"test1">>, <<"default">>], emqx_exhook_mgr:running()).
 
-t_move_2(_) ->
+t_move_before(_) ->
     Result = request_api(post, api_path(["exhooks", "default", "move"]), "",
                          auth_header_(),
                          #{position => before, related => <<"test1">>}),
@@ -147,6 +157,14 @@ t_move_2(_) ->
     ?assertMatch({ok, <<>>}, Result),
     ?assertMatch([<<"default">>, <<"test1">>], emqx_exhook_mgr:running()).
 
+t_move_after(_) ->
+    Result = request_api(post, api_path(["exhooks", "default", "move"]), "",
+                         auth_header_(),
+                         #{position => 'after', related => <<"test1">>}),
+
+    ?assertMatch({ok, <<>>}, Result),
+    ?assertMatch([<<"test1">>, <<"default">>], emqx_exhook_mgr:running()).
+
 t_delete(_) ->
     Result = request_api(delete, api_path(["exhooks", "test1"]), "",
                          auth_header_()),