Various corrections and updates to improve error handling and reporting

OTP-8933

9 files changed, 148 insertions, 82 deletions

diff --git a/lib/common_test/doc/src/config_file_chapter.xml b/lib/common_test/doc/src/config_file_chapter.xml
index 59151a73ec..6fc6638bf7 100644
--- a/lib/common_test/doc/src/config_file_chapter.xml
+++ b/lib/common_test/doc/src/config_file_chapter.xml
@@ -285,7 +285,7 @@
           <c>{ok, Config}</c> - if the configuration variables are read successfully,
         </item>
         <item>
-          <c>{error, Error, ErrorDetails}</c> - if the callback module fails to
+          <c>{error, {Error, ErrorDetails}}</c> - if the callback module fails to
           proceed with the given configuration parameters.
         </item>
       </list>
@@ -422,14 +422,14 @@ stop()->
 call(Client, Request)->
     case whereis(?REGISTERED_NAME) of
         undefined->
-            {error, not_started, Request};
+            {error, {not_started, Request}};
         Pid->
             Pid ! {Client, Request},
             receive
                 Reply->
                     {ok, Reply}
             after 4000->
-                {error, timeout, Request}
+                {error, {timeout, Request}}
             end
     end.
 
diff --git a/lib/common_test/src/ct_config.erl b/lib/common_test/src/ct_config.erl
index 6b75937668..fc51aea7f3 100644
--- a/lib/common_test/src/ct_config.erl
+++ b/lib/common_test/src/ct_config.erl
@@ -204,9 +204,9 @@ get_config_file_list(Opts) ->
     DefaultConfigs = process_default_configs(Opts),
     CfgFiles =
 	if
-	    DefaultConfigs == []->
+	    DefaultConfigs == [] ->
 		[];
-	    true->
+	    true ->
 		[{?ct_config_txt, DefaultConfigs}]
 	end ++
 	process_user_configs(Opts, []),
@@ -240,12 +240,12 @@ read_config_files(Opts) ->
 		  end,
 
     ConfigFiles = case lists:keyfind(config, 1, Opts) of
-		      {config,ConfigLists}->
+		      {config,ConfigLists} ->
                           lists:foldr(fun({Callback,Files}, Acc) ->
 					      AddCallback(Callback,Files)
 						  ++ Acc
 				      end,[],ConfigLists);
-		      false->
+		      false ->
 			  []
 		  end,
     read_config_files_int(ConfigFiles, fun store_config/3).
@@ -255,7 +255,9 @@ read_config_files_int([{Callback, File}|Files], FunToSave) ->
 	{ok, Config} ->
 	    FunToSave(Config, Callback, File),
 	    read_config_files_int(Files, FunToSave);
-	{error, ErrorName, ErrorDetail}->
+	{error, {ErrorName, ErrorDetail}} ->
+	    {user_error, {ErrorName, File, ErrorDetail}};
+	{error, ErrorName, ErrorDetail} ->
 	    {user_error, {ErrorName, File, ErrorDetail}}
     end;
 read_config_files_int([], _FunToSave) ->
@@ -283,7 +285,7 @@ rewrite_config(Config, Callback, File) ->
 			      config=File,_='_'}),
     Updater = fun({Key, Value}) ->
 	case keyfindall(Key, #ct_conf.key, OldRows) of
-	    []->
+	    [] ->
 		ets:insert(?attr_table,
 			   #ct_conf{key=Key,
 				    value=Value,
@@ -453,9 +455,9 @@ update_conf(Name, NewConfig) ->
 
 reload_conf(KeyOrName) ->
     case lookup_handler_for_config(KeyOrName) of
-	[]->
+	[] ->
 	    undefined;
-	HandlerList->
+	HandlerList ->
 	    HandlerList2 = lists:usort(HandlerList),
 	    read_config_files_int(HandlerList2, fun rewrite_config/3),
 	    get_config(KeyOrName)
@@ -711,13 +713,13 @@ random_bytes_1(N, Acc) -> random_bytes_1(N-1, [random:uniform(255)|Acc]).
 
 check_callback_load(Callback) ->
     case code:is_loaded(Callback) of
-	{file, _Filename}->
+	{file, _Filename} ->
 	    check_exports(Callback);
-	false->
+	false ->
 	    case code:load_file(Callback) of
-		{module, Callback}->
+		{module, Callback} ->
 		    check_exports(Callback);
-		{error, Error}->
+		{error, Error} ->
 		    {error, Error}
 	    end
     end.
@@ -745,14 +747,14 @@ check_config_files(Configs) ->
 					  end,
 					  Files)
 			end;
-		{error, Why}->
+		{error, Why} ->
 		    {error, {callback, {Callback,Why}}}
 	    end;
 	({Callback, []}) ->
 	    case check_callback_load(Callback) of
-		{ok, Callback}->
+		{ok, Callback} ->
 		     Callback:check_parameter([]);
-		{error, Why}->
+		{error, Why} ->
 		     {error, {callback, {Callback,Why}}}
 	    end
     end,
@@ -773,15 +775,15 @@ prepare_user_configs([], Acc, _) ->
 
 prepare_config_list(Args) ->
     ConfigFiles = case lists:keysearch(ct_config, 1, Args) of
-		      {value,{ct_config,Files}}->
+		      {value,{ct_config,Files}} ->
 			  [{?ct_config_txt,[filename:absname(F) || F <- Files]}];
-		      false->
+		      false ->
 			  []
 		  end,
     UserConfigs = case lists:keysearch(userconfig, 1, Args) of
-		      {value,{userconfig,UserConfigFiles}}->
+		      {value,{userconfig,UserConfigFiles}} ->
 			  prepare_user_configs(UserConfigFiles, [], new);
-		      false->
+		      false ->
 			  []
 		  end,
     ConfigFiles ++ UserConfigs.
diff --git a/lib/common_test/src/ct_config_plain.erl b/lib/common_test/src/ct_config_plain.erl
index 3fbc8af9fb..6698332379 100644
--- a/lib/common_test/src/ct_config_plain.erl
+++ b/lib/common_test/src/ct_config_plain.erl
@@ -29,7 +29,7 @@ read_config(ConfigFile) ->
 	{ok,Config} ->
 	    {ok, Config};
 	{error,enoent} ->
-	    {error, config_file_error, enoent};
+	    {error,{config_file_error,file:format_error(enoent)}};
 	{error,Reason} ->
 	    Key =
 		case application:get_env(common_test, decrypt) of
@@ -45,23 +45,27 @@ read_config(ConfigFile) ->
 		end,
 	    case Key of
 		{error,no_crypt_file} ->
-		    {error, config_file_error, Reason};
+		    {error,{config_file_error,
+			    lists:flatten(
+			      io_lib:format("~s",[file:format_error(Reason)]))}};
 		{error,CryptError} ->
-		    {error, decrypt_file_error, CryptError};
+		    {error,{decrypt_file_error,CryptError}};
 		_ when is_list(Key) ->
-		    case ct_config:decrypt_config_file(ConfigFile, undefined, {key,Key}) of
+		    case ct_config:decrypt_config_file(ConfigFile,
+						       undefined,
+						       {key,Key}) of
 			{ok,CfgBin} ->
 			    case read_config_terms(CfgBin) of
 				{error,ReadFail} ->
-				    {error, config_file_error, ReadFail};
+				    {error,{config_file_error,ReadFail}};
 				Config ->
-				    {ok, Config}
+				    {ok,Config}
 			    end;
 			{error,DecryptFail} ->
-			    {error, decrypt_config_error, DecryptFail}
+			    {error,{decrypt_config_error,DecryptFail}}
 		    end;
 		_ ->
-		    {error, bad_decrypt_key, Key}
+		    {error,{bad_decrypt_key,Key}}
 	    end
     end.
 
@@ -69,9 +73,9 @@ read_config(ConfigFile) ->
 check_parameter(File)->
     case filelib:is_file(File) of
 	true->
-	    {ok, {file, File}};
+	    {ok,{file,File}};
 	false->
-	    {error, {nofile, File}}
+	    {error,{nofile,File}}
     end.
 
 read_config_terms(Bin) when is_binary(Bin) ->
diff --git a/lib/common_test/src/ct_config_xml.erl b/lib/common_test/src/ct_config_xml.erl
index 8a6e75e635..794174e663 100644
--- a/lib/common_test/src/ct_config_xml.erl
+++ b/lib/common_test/src/ct_config_xml.erl
@@ -27,30 +27,30 @@
 % read config file
 read_config(ConfigFile) ->
     case catch do_read_xml_config(ConfigFile) of
-	{ok, Config}->
-	    {ok, Config};
-	{error, Error, ErroneousString}->
-	    {error, Error, ErroneousString}
+	{ok,Config} ->
+	    {ok,Config};
+	Error = {error,_} ->
+	    Error
     end.
 
 % check file exists
-check_parameter(File)->
+check_parameter(File) ->
     case filelib:is_file(File) of
-	true->
-	    {ok, {file, File}};
-	false->
-	    {error, {nofile, File}}
+	true ->
+	    {ok,{file,File}};
+	false ->
+	    {error,{nofile,File}}
     end.
 
 % actual reading of the config
-do_read_xml_config(ConfigFile)->
+do_read_xml_config(ConfigFile) ->
     case catch xmerl_sax_parser:file(ConfigFile,
-	    [{event_fun, fun event/3},
-	    {event_state, []}]) of
-	{ok, EntityList, _}->
-	    {ok, lists:reverse(transform_entity_list(EntityList))};
-	Oops->
-	    {error, parsing_failed, Oops}
+	    [{event_fun,fun event/3},
+	     {event_state,[]}]) of
+	{ok,EntityList,_} ->
+	    {ok,lists:reverse(transform_entity_list(EntityList))};
+	Oops ->
+	    {error,{parsing_failed,Oops}}
     end.
 
 % event callback for xmerl_sax_parser
@@ -92,18 +92,18 @@ tag(_El, State) ->
 	State.
 
 % transform of the ugly deeply nested entity list to the key-value "tree"
-transform_entity_list(EntityList)->
+transform_entity_list(EntityList) ->
     lists:map(fun transform_entity/1, EntityList).
 
 % transform entity from {list(), list()} to {atom(), term()}
 transform_entity({Tag, [Value|Rest]}) when
-    is_tuple(Value)->
+    is_tuple(Value) ->
 	{list_to_atom(Tag), transform_entity_list(lists:reverse([Value|Rest]))};
-transform_entity({Tag, String})->
+transform_entity({Tag, String}) ->
     case list_to_term(String) of
-	{ok, Value}->
+	{ok, Value} ->
 	    {list_to_atom(Tag), Value};
-	Error->
+	Error ->
 	     throw(Error)
     end.
 
@@ -111,8 +111,8 @@ transform_entity({Tag, String})->
 list_to_term(String) ->
     {ok, T, _} = erl_scan:string(String++"."),
     case catch erl_parse:parse_term(T) of
-        {ok, Term} ->
-            {ok, Term};
+        {ok,Term} ->
+            {ok,Term};
         Error ->
-            {error, Error, String}
+            {error,{Error,String}}
     end.
diff --git a/lib/common_test/src/ct_framework.erl b/lib/common_test/src/ct_framework.erl
index b730f1e46e..29caa27d94 100644
--- a/lib/common_test/src/ct_framework.erl
+++ b/lib/common_test/src/ct_framework.erl
@@ -245,7 +245,12 @@ add_defaults(Mod,Func,FuncInfo,DoInit) ->
 		Error = {error,_} -> {SuiteInfo,Error};
 		MergedInfo -> {SuiteInfo,MergedInfo}
 	    end;
-	{'EXIT',Reason} -> 
+	{'EXIT',Reason} ->
+	    ErrStr = io_lib:format("~n*** ERROR *** "
+				   "~w:suite/0 failed: ~p~n",
+				   [Mod,Reason]),
+	    io:format(ErrStr, []),
+	    io:format(user, ErrStr, []),
 	    {suite0_failed,{exited,Reason}};
 	SuiteInfo when is_list(SuiteInfo) ->
 	    case lists:all(fun(E) when is_tuple(E) -> true;
@@ -261,9 +266,19 @@ add_defaults(Mod,Func,FuncInfo,DoInit) ->
 			MergedInfo -> {SuiteInfo1,MergedInfo}
 		    end;
 		false ->
+		    ErrStr = io_lib:format("~n*** ERROR *** "
+					   "Invalid return value from "
+					   "~w:suite/0: ~p~n", [Mod,SuiteInfo]),
+		    io:format(ErrStr, []),
+		    io:format(user, ErrStr, []),
 		    {suite0_failed,bad_return_value}
 	    end;
-	_ ->
+	SuiteInfo ->
+	    ErrStr = io_lib:format("~n*** ERROR *** "
+				   "Invalid return value from "
+				   "~w:suite/0: ~p~n", [Mod,SuiteInfo]),
+	    io:format(ErrStr, []),
+	    io:format(user, ErrStr, []),
 	    {suite0_failed,bad_return_value}
     end.
 
diff --git a/lib/common_test/src/ct_logs.erl b/lib/common_test/src/ct_logs.erl
index 761b906392..f34eb83afa 100644
--- a/lib/common_test/src/ct_logs.erl
+++ b/lib/common_test/src/ct_logs.erl
@@ -484,8 +484,8 @@ logger_loop(State) ->
 							  [Str,Args]),
 						%% stop the testcase, we need
 						%% to see the fault
-						exit(Pid,logging_failed),
-						ok;
+						exit(Pid,{log_printout_error,Str,Args}),
+						[];
 					    IoStr when IoList == [] ->
 						[IoStr];
 					    IoStr ->
diff --git a/lib/common_test/src/ct_run.erl b/lib/common_test/src/ct_run.erl
index c3b0348b26..4ff062c2fa 100644
--- a/lib/common_test/src/ct_run.erl
+++ b/lib/common_test/src/ct_run.erl
@@ -667,6 +667,16 @@ run_test(StartOpt) when is_tuple(StartOpt) ->
     run_test([StartOpt]);
 
 run_test(StartOpts) when is_list(StartOpts) ->
+    CTPid = spawn(fun() -> run_test1(StartOpts) end),
+    Ref = monitor(process, CTPid),
+    receive
+	{'DOWN',Ref,process,CTPid,{user_error,Error}} ->
+		    Error;
+	{'DOWN',Ref,process,CTPid,Other} ->
+		    Other
+    end.
+
+run_test1(StartOpts) when is_list(StartOpts) ->
     case proplists:get_value(refresh_logs, StartOpts) of
 	undefined ->
 	    Tracing = start_trace(StartOpts),
@@ -675,7 +685,7 @@ run_test(StartOpts) when is_list(StartOpts) ->
 	    Res =
 		case ct_repeat:loop_test(func, StartOpts) of
 		    false ->
-			case catch run_test1(StartOpts) of
+			case catch run_test2(StartOpts) of
 			    {'EXIT',Reason} ->
 				file:set_cwd(Cwd),
 				{error,Reason};
@@ -686,13 +696,13 @@ run_test(StartOpts) when is_list(StartOpts) ->
 			Result
 		end,
 	    stop_trace(Tracing),
-	    Res;
+	    exit(Res);
 	RefreshDir ->
 	    refresh_logs(?abs(RefreshDir)),
-	    ok
+	    exit(done)
     end.
 
-run_test1(StartOpts) ->
+run_test2(StartOpts) ->
     %% label
     Label = get_start_opt(label, fun(Lbl) when is_list(Lbl) -> Lbl;
 				    (Lbl) when is_atom(Lbl) -> atom_to_list(Lbl)
@@ -981,7 +991,7 @@ run_dir(Opts = #opts{logdir = LogDir,
 	{Dir=[Hd|_],undefined,[]} when is_list(Dir) and is_integer(Hd) ->
 	    reformat_result(catch do_run(tests(Dir), [], Opts1, StartOpts));
 
-	{Dir,undefined,[]} when is_atom(Dir) ->
+	{Dir,undefined,[]} when is_atom(Dir) and (Dir /= undefined) ->
 	    reformat_result(catch do_run(tests(atom_to_list(Dir)),
 					 [], Opts1, StartOpts));
 
@@ -1052,6 +1062,9 @@ run_dir(Opts = #opts{logdir = LogDir,
 		    end
 	    end;
 
+	{undefined,undefined,[]} ->
+	    exit(no_test_specified);
+
 	{Dir,Suite,GsAndCs} ->
 	    exit({incorrect_start_options,{Dir,Suite,GsAndCs}})
     end.
@@ -1064,19 +1077,38 @@ run_dir(Opts = #opts{logdir = LogDir,
 %%% the same as those used in test specification files.
 %%% @equiv ct:run_testspec/1
 %%%-----------------------------------------------------------------
-
 run_testspec(TestSpec) ->
+    CTPid = spawn(fun() -> run_testspec1(TestSpec) end),
+    Ref = monitor(process, CTPid),
+    receive
+	{'DOWN',Ref,process,CTPid,{user_error,Error}} ->
+		    Error;
+	{'DOWN',Ref,process,CTPid,Other} ->
+		    Other
+    end.
+
+run_testspec1(TestSpec) ->
     {ok,Cwd} = file:get_cwd(),
     io:format("~nCommon Test starting (cwd is ~s)~n~n", [Cwd]),
-    case catch run_testspec1(TestSpec) of
+    case catch run_testspec2(TestSpec) of
 	{'EXIT',Reason} ->
 	    file:set_cwd(Cwd),
-	    {error,Reason};
+	    exit({error,Reason});
 	Result ->
-	    Result
+	    exit(Result)
     end.
 
-run_testspec1(TestSpec) ->
+run_testspec2(File) when is_list(File), is_integer(hd(File)) ->
+    case file:read_file_info(File) of
+	{ok,_} ->
+	    exit("Bad argument, "
+		 "use ct:run_test([{spec," ++ File ++ "}])");
+	_ ->
+	    exit("Bad argument, list of tuples expected, "
+		 "use ct:run_test/1 for test specification files")
+    end;
+
+run_testspec2(TestSpec) ->
     case catch ct_testspec:collect_tests_from_list(TestSpec, false) of
 	{E,CTReason}  when E == error ; E == 'EXIT' ->
 	    exit(CTReason);
@@ -1295,7 +1327,6 @@ do_run(Tests, Misc, LogDir, LogOpts) when is_list(Misc),
 
 do_run(Tests, Skip, Opts, Args) when is_record(Opts, opts) ->
     #opts{label = Label, profile = Profile, cover = Cover} = Opts,
-
     %% label - used by ct_logs
     TestLabel =
 	if Label == undefined -> undefined;
@@ -2478,8 +2509,8 @@ start_trace(Args) ->
 			    false
 		    end;
 		{_,Error} ->
-		    io:format("Warning! Tracing not started. Reason: ~p~n~n",
-			      [Error]),
+		    io:format("Warning! Tracing not started. Reason: ~s~n~n",
+			      [file:format_error(Error)]),
 		    false
 	    end;
 	false ->
diff --git a/lib/common_test/src/ct_testspec.erl b/lib/common_test/src/ct_testspec.erl
index bf1dfa24ea..2cba1d8410 100644
--- a/lib/common_test/src/ct_testspec.erl
+++ b/lib/common_test/src/ct_testspec.erl
@@ -249,11 +249,15 @@ collect_tests_from_file1([Spec|Specs],TestSpec,Relaxed) ->
     SpecDir = filename:dirname(filename:absname(Spec)),
     case file:consult(Spec) of
 	{ok,Terms} ->	    
-	    TestSpec1 = collect_tests(Terms,TestSpec#testspec{spec_dir=SpecDir},
+	    TestSpec1 = collect_tests(Terms,
+				      TestSpec#testspec{spec_dir=SpecDir},
 				      Relaxed),
 	    collect_tests_from_file1(Specs,TestSpec1,Relaxed);
 	{error,Reason} ->
-	    throw({error,{Spec,Reason}})
+	    ReasonStr =
+		lists:flatten(io_lib:format("~s",
+					    [file:format_error(Reason)])),
+	    throw({error,{Spec,ReasonStr}})
     end;
 collect_tests_from_file1([],TS=#testspec{config=Cfgs,event_handler=EvHs,
 					 include=Incl,tests=Tests},_) ->
diff --git a/lib/common_test/src/ct_util.erl b/lib/common_test/src/ct_util.erl
index ef94c25364..f393ea103d 100644
--- a/lib/common_test/src/ct_util.erl
+++ b/lib/common_test/src/ct_util.erl
@@ -124,13 +124,15 @@ do_start(Parent,Mode,LogDir) ->
 	ok -> ok;
 	E -> exit(E)
     end,
+    DoExit = fun(Reason) -> file:set_cwd(StartDir), exit(Reason) end,
     Opts = case read_opts() of
 	       {ok,Opts1} ->
 		   Opts1;
 	       Error ->
 		   Parent ! {self(),Error},
-		   exit(Error)
+		   DoExit(Error)
 	   end,
+
     %% start an event manager (if not already started by master)
     case ct_event:start_link() of
 	{error,{already_started,_}} ->
@@ -143,16 +145,23 @@ do_start(Parent,Mode,LogDir) ->
 		    ct_event:add_handler([{vts,VtsPid}])
 	    end
     end,
+
     %% start ct_config server
-    ct_config:start(Mode),
+    try ct_config:start(Mode) of
+	_ -> ok
+    catch
+	_Class:CfgError ->
+	    DoExit(CfgError)
+    end,
+
     %% add user event handlers
     case lists:keysearch(event_handler,1,Opts) of
 	{value,{_,Handlers}} ->
 	    Add = fun({H,Args}) ->
 			  case catch gen_event:add_handler(?CT_EVMGR_REF,H,Args) of
 			      ok -> ok;
-			      {'EXIT',Why} -> exit(Why);
-			      Other -> exit({event_handler,Other})
+			      {'EXIT',Why} -> DoExit(Why);
+			      Other -> DoExit({event_handler,Other})
 			  end
 		  end,
 	    case catch lists:foreach(Add,Handlers) of
@@ -171,10 +180,11 @@ do_start(Parent,Mode,LogDir) ->
 			   data={StartTime,
 				 lists:flatten(TestLogDir)}}),
     %% Initialize ct_hooks
-    case catch ct_hooks:init(Opts) of
+    try ct_hooks:init(Opts) of
 	ok ->
-	    Parent ! {self(),started};
-	{_,CTHReason} ->
+	    Parent ! {self(),started}
+    catch
+	_:CTHReason ->
 	    ct_logs:tc_print('Suite Callback',CTHReason,[]),
 	    self() ! {{stop,{self(),{user_error,CTHReason}}},
 		      {Parent,make_ref()}}