rework tests for most simple case of child termination

increase test coverage for whole of terminate_simple_children
fix bug in timeout handling clause of terminate_simple_children
cosmetic (trailing whitespace)

2 files changed, 98 insertions, 121 deletions

diff --git a/src/supervisor2.erl b/src/supervisor2.erl
index e26df7d7..5af38573 100644
--- a/src/supervisor2.erl
+++ b/src/supervisor2.erl
@@ -255,10 +255,10 @@ behaviour_info(_Other) ->
 %%% ---------------------------------------------------
 start_link(Mod, Args) ->
     gen_server:start_link(?MODULE, {self, Mod, Args}, []).
- 
+
 start_link(SupName, Mod, Args) ->
     gen_server:start_link(SupName, ?MODULE, {SupName, Mod, Args}, []).
- 
+
 %%% ---------------------------------------------------
 %%% Interface functions.
 %%% ---------------------------------------------------
@@ -298,9 +298,9 @@ check_childspecs(ChildSpecs) when is_list(ChildSpecs) ->
 check_childspecs(X) -> {error, {badarg, X}}.
 
 %%% ---------------------------------------------------
-%%% 
+%%%
 %%% Initialize the supervisor.
-%%% 
+%%%
 %%% ---------------------------------------------------
 init({SupName, Mod, Args}) ->
     process_flag(trap_exit, true),
@@ -319,7 +319,7 @@ init({SupName, Mod, Args}) ->
 	Error ->
 	    {stop, {bad_return, {Mod, init, Error}}}
     end.
-	
+
 init_children(State, StartSpec) ->
     SupName = State#state.name,
     case check_startspec(StartSpec) of
@@ -349,7 +349,7 @@ init_dynamic(_State, StartSpec) ->
 %% Func: start_children/2
 %% Args: Children = [#child] in start order
 %%       SupName = {local, atom()} | {global, atom()} | {pid(),Mod}
-%% Purpose: Start all children.  The new list contains #child's 
+%% Purpose: Start all children.  The new list contains #child's
 %%          with pids.
 %% Returns: {ok, NChildren} | {error, NChildren}
 %%          NChildren = [#child] in termination order (reversed
@@ -381,7 +381,7 @@ do_start_child(SupName, Child) ->
 	    NChild = Child#child{pid = Pid},
 	    report_progress(NChild, SupName),
 	    {ok, Pid, Extra};
-	ignore -> 
+	ignore ->
 	    {ok, undefined};
 	{error, What} -> {error, What};
 	What -> {error, What}
@@ -400,12 +400,12 @@ do_start_child_i(M, F, A) ->
 	What ->
 	    {error, What}
     end.
-    
+
 
 %%% ---------------------------------------------------
-%%% 
+%%%
 %%% Callback functions.
-%%% 
+%%%
 %%% ---------------------------------------------------
 handle_call({start_child, EArgs}, _From, State) when ?is_simple(State) ->
     #child{mfa = {M, F, A}} = hd(State#state.children),
@@ -414,11 +414,11 @@ handle_call({start_child, EArgs}, _From, State) when ?is_simple(State) ->
         {ok, undefined} ->
             {reply, {ok, undefined}, State};
 	{ok, Pid} ->
-	    NState = State#state{dynamics = 
+	    NState = State#state{dynamics =
 				 ?DICT:store(Pid, Args, State#state.dynamics)},
 	    {reply, {ok, Pid}, NState};
 	{ok, Pid, Extra} ->
-	    NState = State#state{dynamics = 
+	    NState = State#state{dynamics =
 				 ?DICT:store(Pid, Args, State#state.dynamics)},
 	    {reply, {ok, Pid, Extra}, NState};
 	What ->
@@ -497,7 +497,7 @@ handle_call(which_children, _From, State) ->
 %%% Hopefully cause a function-clause as there is no API function
 %%% that utilizes cast.
 handle_cast(null, State) ->
-    error_logger:error_msg("ERROR: Supervisor received cast-message 'null'~n", 
+    error_logger:error_msg("ERROR: Supervisor received cast-message 'null'~n",
 			   []),
 
     {noreply, State}.
@@ -527,7 +527,7 @@ handle_info({'EXIT', Pid, Reason}, State) ->
     end;
 
 handle_info(Msg, State) ->
-    error_logger:error_msg("Supervisor received unexpected message: ~p~n", 
+    error_logger:error_msg("Supervisor received unexpected message: ~p~n",
 			   [Msg]),
     {noreply, State}.
 %%
@@ -577,13 +577,13 @@ check_flags({Strategy, MaxIntensity, Period}) ->
 check_flags(What) ->
     {bad_flags, What}.
 
-update_childspec(State, StartSpec)  when ?is_simple(State) -> 
-    case check_startspec(StartSpec) of                        
-        {ok, [Child]} ->                                      
-            {ok, State#state{children = [Child]}};            
-        Error ->                                              
-            {error, Error}                                    
-    end;                                                      
+update_childspec(State, StartSpec)  when ?is_simple(State) ->
+    case check_startspec(StartSpec) of
+        {ok, [Child]} ->
+            {ok, State#state{children = [Child]}};
+        Error ->
+            {error, Error}
+    end;
 
 update_childspec(State, StartSpec) ->
     case check_startspec(StartSpec) of
@@ -604,7 +604,7 @@ update_childspec1([Child|OldC], Children, KeepOld) ->
     end;
 update_childspec1([], Children, KeepOld) ->
     % Return them in (keeped) reverse start order.
-    lists:reverse(Children ++ KeepOld).  
+    lists:reverse(Children ++ KeepOld).
 
 update_chsp(OldCh, Children) ->
     case lists:map(fun (Ch) when OldCh#child.name =:= Ch#child.name ->
@@ -618,7 +618,7 @@ update_chsp(OldCh, Children) ->
 	NewC ->
 	    {ok, NewC}
     end.
-    
+
 %%% ---------------------------------------------------
 %%% Start a new child.
 %%% ---------------------------------------------------
@@ -630,12 +630,12 @@ handle_start_child(Child, State) ->
 		{ok, Pid} ->
 		    Children = State#state.children,
 		    {{ok, Pid},
-		     State#state{children = 
+		     State#state{children =
 				 [Child#child{pid = Pid}|Children]}};
 		{ok, Pid, Extra} ->
 		    Children = State#state.children,
 		    {{ok, Pid, Extra},
-		     State#state{children = 
+		     State#state{children =
 				 [Child#child{pid = Pid}|Children]}};
 		{error, What} ->
 		    {{error, {What, Child}}, State}
@@ -866,7 +866,7 @@ timeout_stop(#child{shutdown = Time}, TRef, Msg, false) when is_integer(Time) ->
     after
         0 -> ok
     end;
-timeout_stop(#child{}, ok, _Msg, _Timedout) ->
+timeout_stop(#child{}, _TRef, _Msg, _Timedout) ->
     ok.
 
 do_terminate(Child, SupName) when Child#child.pid =/= undefined ->
@@ -888,17 +888,17 @@ do_terminate(Child, _SupName) ->
     Child.
 
 %%-----------------------------------------------------------------
-%% Shutdowns a child. We must check the EXIT value 
+%% Shutdowns a child. We must check the EXIT value
 %% of the child, because it might have died with another reason than
-%% the wanted. In that case we want to report the error. We put a 
-%% monitor on the child an check for the 'DOWN' message instead of 
-%% checking for the 'EXIT' message, because if we check the 'EXIT' 
-%% message a "naughty" child, who does unlink(Sup), could hang the 
-%% supervisor. 
+%% the wanted. In that case we want to report the error. We put a
+%% monitor on the child an check for the 'DOWN' message instead of
+%% checking for the 'EXIT' message, because if we check the 'EXIT'
+%% message a "naughty" child, who does unlink(Sup), could hang the
+%% supervisor.
 %% Returns: ok | {error, OtherReason}  (this should be reported)
 %%-----------------------------------------------------------------
 shutdown(Pid, brutal_kill) ->
-  
+
     case monitor_child(Pid) of
 	ok ->
 	    exit(Pid, kill),
@@ -908,16 +908,16 @@ shutdown(Pid, brutal_kill) ->
 		{'DOWN', _MRef, process, Pid, OtherReason} ->
 		    {error, OtherReason}
 	    end;
-	{error, Reason} ->      
+	{error, Reason} ->
 	    {error, Reason}
     end;
 
 shutdown(Pid, Time) ->
-    
+
     case monitor_child(Pid) of
 	ok ->
 	    exit(Pid, shutdown), %% Try to shutdown gracefully
-	    receive 
+	    receive
 		{'DOWN', _MRef, process, Pid, shutdown} ->
 		    ok;
 		{'DOWN', _MRef, process, Pid, OtherReason} ->
@@ -929,14 +929,14 @@ shutdown(Pid, Time) ->
 			    {error, OtherReason}
 		    end
 	    end;
-	{error, Reason} ->      
+	{error, Reason} ->
 	    {error, Reason}
     end.
 
 %% Help function to shutdown/2 switches from link to monitor approach
 monitor_child(Pid) ->
-    
-    %% Do the monitor operation first so that if the child dies 
+
+    %% Do the monitor operation first so that if the child dies
     %% before the monitoring is done causing a 'DOWN'-message with
     %% reason noproc, we will get the real reason in the 'EXIT'-message
     %% unless a naughty child has already done unlink...
@@ -946,22 +946,22 @@ monitor_child(Pid) ->
     receive
 	%% If the child dies before the unlik we must empty
 	%% the mail-box of the 'EXIT'-message and the 'DOWN'-message.
-	{'EXIT', Pid, Reason} -> 
-	    receive 
+	{'EXIT', Pid, Reason} ->
+	    receive
 		{'DOWN', _, process, Pid, _} ->
 		    {error, Reason}
 	    end
-    after 0 -> 
+    after 0 ->
 	    %% If a naughty child did unlink and the child dies before
-	    %% monitor the result will be that shutdown/2 receives a 
+	    %% monitor the result will be that shutdown/2 receives a
 	    %% 'DOWN'-message with reason noproc.
 	    %% If the child should die after the unlink there
 	    %% will be a 'DOWN'-message with a correct reason
-	    %% that will be handled in shutdown/2. 
-	    ok   
+	    %% that will be handled in shutdown/2.
+	    ok
     end.
-    
-   
+
+
 %%-----------------------------------------------------------------
 %% Child/State manipulating functions.
 %%-----------------------------------------------------------------
@@ -1015,7 +1015,7 @@ remove_child(Child, State) ->
 %% Args: SupName = {local, atom()} | {global, atom()} | self
 %%       Type = {Strategy, MaxIntensity, Period}
 %%         Strategy = one_for_one | one_for_all | simple_one_for_one |
-%%                    rest_for_one 
+%%                    rest_for_one
 %%         MaxIntensity = integer()
 %%         Period = integer()
 %%       Mod :== atom()
@@ -1110,10 +1110,10 @@ validChildType(supervisor) -> true;
 validChildType(worker) -> true;
 validChildType(What) -> throw({invalid_child_type, What}).
 
-validName(_Name) -> true. 
+validName(_Name) -> true.
 
-validFunc({M, F, A}) when is_atom(M), 
-                          is_atom(F), 
+validFunc({M, F, A}) when is_atom(M),
+                          is_atom(F),
                           is_list(A) -> true;
 validFunc(Func)                      -> throw({invalid_mfa, Func}).
 
@@ -1131,7 +1131,7 @@ validDelay(Delay) when is_number(Delay),
                        Delay >= 0 -> true;
 validDelay(What)                  -> throw({invalid_delay, What}).
 
-validShutdown(Shutdown, _) 
+validShutdown(Shutdown, _)
   when is_integer(Shutdown), Shutdown > 0 -> true;
 validShutdown(infinity, supervisor)    -> true;
 validShutdown(brutal_kill, _)          -> true;
@@ -1157,7 +1157,7 @@ validMods(Mods) -> throw({invalid_modules, Mods}).
 %%% Returns: {ok, State'} | {terminate, State'}
 %%% ------------------------------------------------------
 
-add_restart(State) ->  
+add_restart(State) ->
     I = State#state.intensity,
     P = State#state.period,
     R = State#state.restarts,
diff --git a/src/supervisor2_tests.erl b/src/supervisor2_tests.erl
index 6898cfa6..599dfac9 100644
--- a/src/supervisor2_tests.erl
+++ b/src/supervisor2_tests.erl
@@ -17,89 +17,66 @@
 -module(supervisor2_tests).
 -behaviour(supervisor2).
 
--export([test_all/0, start_sup/0, start_link/0, start_child/0]).
+-export([test_all/0, start_link/0]).
 -export([init/1]).
 
--define(NUM_CHILDREN, 1000).
-
--export([test_all/0]).
-
 -include_lib("eunit/include/eunit.hrl").
 
+-define(TEST_RUNS, 2000).
+-define(SLOW_TEST_RUNS, 45).
+-define(CHILDREN, 100).
+
 test_all() ->
-    eunit:test(supervisor2, [verbose]).
+    eunit:test(?MODULE, [verbose]).
 
-terminate_simple_children_without_deadlock_test_() ->
-    lists:flatten(
-      lists:duplicate(
-        100, [{setup, fun init_supervisor/0,
-               {with, [fun ensure_children_are_alive/1,
-                       fun shutdown_and_verify_all_children_died/1]}},
-              {setup, fun init_supervisor/0,
-               {with, [fun shutdown_whilst_interleaving_exits_occur/1]}}])).
+simple_child_shutdown_without_deadlock_test_() ->
+    lists:duplicate(?TEST_RUNS,
+                    [{timeout, 60, fun test_clean_stop/0}]).
 
-%%
-%% Public (test facing) API
-%%
+simple_child_shutdown_with_timeout_test_() ->
+    lists:duplicate(?SLOW_TEST_RUNS,
+                    [{timeout, 120, fun test_timeout/0}]).
 
-start_sup() ->
-    supervisor2:start_link({local, ?MODULE}, ?MODULE, []).
+test_clean_stop() ->
+    test_it(stop).
 
-start_link() ->
-    Pid = spawn_link(fun loop_infinity/0),
-    {ok, Pid}.
+test_timeout() ->
+    test_it(ignored).
 
-start_child() ->
-    {ok, Pid} = supervisor2:start_child(?MODULE, []),
-    Pid.
+test_it(SigStop) ->
+    {ok, Pid} = supervisor2:start_link(?MODULE, [?CHILDREN]),
+    start_and_terminate_children(SigStop, Pid, ?CHILDREN),
+    unlink(Pid),
+    exit(Pid, shutdown).
 
-%%
-%% supervisor2 callbacks
-%%
+start_link() ->
+    Pid = spawn_link(fun () ->
+                             process_flag(trap_exit, true),
+                             receive stop -> ok end
+                     end),
+    {ok, Pid}.
 
-init([parent]) ->
+init([N]) ->
     {ok, {{one_for_one, 0, 1},
-          [{test_sup, {?MODULE, start_sup, []},
-            transient, 5000, supervisor, [?MODULE]}]}};
+          [{test_sup, {supervisor2, start_link,
+                       [{local, ?MODULE}, ?MODULE, []]},
+            transient, N * 100, supervisor, [?MODULE]}]}};
 init([]) ->
     {ok, {{simple_one_for_one_terminate, 0, 1},
           [{test_worker, {?MODULE, start_link, []},
-            temporary, 1000, worker, []}]}}.
-
-%%
-%% Private API
-%%
-
-ensure_children_are_alive({_, ChildPids}) ->
-    ?assertEqual(true,
-                 lists:all(fun erlang:is_process_alive/1, ChildPids)).
-
-shutdown_and_verify_all_children_died({Parent, ChildPids} = State) ->
-    ensure_children_are_alive(State),
-    TestSup = erlang:whereis(?MODULE),
-    ?assertEqual(true, erlang:is_process_alive(TestSup)),
-    ?assertMatch(ok, supervisor2:terminate_child(Parent, test_sup)),
-    ?assertMatch([], [P || P <- ChildPids,
-                           erlang:is_process_alive(P)]),
-    ?assertEqual(false, erlang:is_process_alive(TestSup)).
-
-shutdown_whilst_interleaving_exits_occur({Parent, ChildPids} = State) ->
-    ensure_children_are_alive(State),
-    TestPid = self(),
-    Ref = erlang:make_ref(),
-    spawn(fun() ->
-                  TestPid ! {Ref, supervisor2:terminate_child(Parent, test_sup)}
-          end),
-    [P ! stop || P <- ChildPids],
-    receive {Ref, Res} ->
-            ?assertEqual(ok, Res)
-    end.
-
-init_supervisor() ->
-    {ok, Pid} = supervisor2:start_link(?MODULE, [parent]),
-    {Pid, [start_child() || _ <- lists:seq(1, ?NUM_CHILDREN)]}.
-
-loop_infinity() ->
+            temporary, 1000, worker, [?MODULE]}]}}.
+
+start_and_terminate_children(SigStop, Sup, N) ->
+    TestSupPid = whereis(?MODULE),
+    ChildPids = [begin
+                     {ok, ChildPid} = supervisor2:start_child(TestSupPid, []),
+                     ChildPid
+                 end || _ <- lists:seq(1, N)],
+    erlang:monitor(process, TestSupPid),
+    [P ! SigStop || P <- ChildPids],
+    ?assertEqual(ok, supervisor2:terminate_child(Sup, test_sup)),
+    ?assertMatch({ok,_}, supervisor2:restart_child(Sup, test_sup)),
     receive
-        stop -> ok
+        {'DOWN', _MRef, process, TestSupPid, Reason} ->
+                ?assertMatch(shutdown, Reason)
     end.