Improve merge of command line options and config options

Bug: option 'spec' is not specifically handled when merging options
from the command line with options from rebar.config. Due to this, if
the config specifies a 'spec', then this will take precedence over any
'dir' and/or 'suite' on the command line.

This commit takes special care of all options that can be used to
select tests - meaning that if any of the options 'spec', 'dir',
'suite', 'group' or 'case' are specified on the command line, then all
'spec', 'dir', 'suite', 'group' and 'case' options in rebar.config
will be ignored.

2 files changed, 136 insertions, 27 deletions

diff --git a/src/rebar_prv_common_test.erl b/src/rebar_prv_common_test.erl
index be31e8c..5fa7ae4 100644
--- a/src/rebar_prv_common_test.erl
+++ b/src/rebar_prv_common_test.erl
@@ -227,20 +227,42 @@ select_tests(State, ProjectApps, CmdOpts, CfgOpts) ->
     rebar_utils:reread_config(Configs),
     code:set_path(OldPath),
 
-    Merged = lists:ukeymerge(1,
-                             lists:ukeysort(1, CmdOpts),
-                             lists:ukeysort(1, CfgOpts)),
-    %% make sure `dir` and/or `suite` from command line go in as
-    %% a pair overriding both `dir` and `suite` from config if
-    %% they exist
-    Opts = case {proplists:get_value(suite, CmdOpts), proplists:get_value(dir, CmdOpts)} of
-        {undefined, undefined} -> Merged;
-        {_Suite, undefined}    -> lists:keydelete(dir, 1, Merged);
-        {undefined, _Dir}      -> lists:keydelete(suite, 1, Merged);
-        {_Suite, _Dir}         -> Merged
-    end,
+    Opts = merge_opts(CmdOpts,CfgOpts),
     discover_tests(State, ProjectApps, Opts).
 
+%% Merge the option lists from command line and rebar.config:
+%%
+%% - Options set on the command line will replace the same options if
+%%   set in rebar.config.
+%%
+%% - Special care is taken with options that select which tests to
+%%   run - ANY such option on the command line will replace ALL such
+%%   options in the config.
+%%
+%%   Note that if 'spec' is given, common_test will ignore all 'dir',
+%%   'suite', 'group' and 'case', so there is no need to explicitly
+%%   remove any options from the command line.
+%%
+%%   All faulty combinations of options are also handled by
+%%   common_test and are not taken into account here.
+merge_opts(CmdOpts0, CfgOpts0) ->
+    TestSelectOpts = [spec,dir,suite,group,testcase],
+    CmdOpts = lists:ukeysort(1, CmdOpts0),
+    CfgOpts1 = lists:ukeysort(1, CfgOpts0),
+    CfgOpts = case is_any_defined(TestSelectOpts,CmdOpts) of
+                  false ->
+                      CfgOpts1;
+                  true ->
+                       [Opt || Opt={K,_} <- CfgOpts1,
+                               not lists:member(K,TestSelectOpts)]
+              end,
+    lists:ukeymerge(1, CmdOpts, CfgOpts).
+
+is_any_defined([Key|Keys],Opts) ->
+    proplists:is_defined(Key,Opts) orelse is_any_defined(Keys,Opts);
+is_any_defined([],_Opts) ->
+    false.
+
 sys_config_list(CmdOpts, CfgOpts) ->
     CmdSysConfigs = split_string(proplists:get_value(sys_config, CmdOpts, "")),
     case proplists:get_value(sys_config, CfgOpts, []) of
@@ -253,11 +275,10 @@ sys_config_list(CmdOpts, CfgOpts) ->
     end.
 
 discover_tests(State, ProjectApps, Opts) ->
-    case {proplists:get_value(suite, Opts), proplists:get_value(dir, Opts)} of
-        %% no dirs or suites defined, try using `$APP/test` and `$ROOT/test`
-        %%  as suites
-        {undefined, undefined} -> {ok, [default_tests(State, ProjectApps)|Opts]};
-        {_, _}                 -> {ok, Opts}
+    case is_any_defined([spec,dir,suite],Opts) of
+        %% no tests defined, try using `$APP/test` and `$ROOT/test` as dirs
+        false -> {ok, [default_tests(State, ProjectApps)|Opts]};
+        true  -> {ok, Opts}
     end.
 
 default_tests(State, ProjectApps) ->
@@ -397,14 +418,20 @@ readable(State) ->
     end.
 
 test_dirs(State, Apps, Opts) ->
-    case {proplists:get_value(suite, Opts), proplists:get_value(dir, Opts)} of
-        {Suites, undefined} -> set_compile_dirs(State, Apps, {suite, Suites});
-        {undefined, Dirs}   -> set_compile_dirs(State, Apps, {dir, Dirs});
-        {Suites, Dir} when is_integer(hd(Dir)) ->
-            set_compile_dirs(State, Apps, join(Suites, Dir));
-        {Suites, [Dir]} when is_integer(hd(Dir)) ->
-            set_compile_dirs(State, Apps, join(Suites, Dir));
-        {_Suites, _Dirs}    -> {error, "Only a single directory may be specified when specifying suites"}
+    case proplists:get_value(spec, Opts) of
+        undefined ->
+            case {proplists:get_value(suite, Opts), proplists:get_value(dir, Opts)} of
+                {Suites, undefined} -> set_compile_dirs(State, Apps, {suite, Suites});
+                {undefined, Dirs}   -> set_compile_dirs(State, Apps, {dir, Dirs});
+                {Suites, Dir} when is_integer(hd(Dir)) ->
+                    set_compile_dirs(State, Apps, join(Suites, Dir));
+                {Suites, [Dir]} when is_integer(hd(Dir)) ->
+                    set_compile_dirs(State, Apps, join(Suites, Dir));
+                {_Suites, _Dirs}    -> {error, "Only a single directory may be specified when specifying suites"}
+            end;
+        _Specs ->
+            %% Currently not adding any directories from the spec files.
+            {ok, rebar_state:project_apps(State, Apps)}
     end.
 
 join(Suite, Dir) when is_integer(hd(Suite)) ->
diff --git a/test/rebar_ct_SUITE.erl b/test/rebar_ct_SUITE.erl
index 8e989b5..1995690 100644
--- a/test/rebar_ct_SUITE.erl
+++ b/test/rebar_ct_SUITE.erl
@@ -51,7 +51,9 @@
          cover_compiled/1,
          misspecified_ct_opts/1,
          misspecified_ct_compile_opts/1,
-         misspecified_ct_first_files/1]).
+         misspecified_ct_first_files/1,
+         testspec/1,
+         cmd_vs_cfg_opts/1]).
 
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("common_test/include/ct.hrl").
@@ -67,7 +69,9 @@ all() -> [{group, basic_app},
           cfg_atom_suites,
           misspecified_ct_opts,
           misspecified_ct_compile_opts,
-          misspecified_ct_first_files].
+          misspecified_ct_first_files,
+          testspec,
+          cmd_vs_cfg_opts].
 
 groups() -> [{basic_app, [], [basic_app_default_dirs,
                               basic_app_default_beams,
@@ -1234,6 +1238,84 @@ misspecified_ct_first_files(Config) ->
 
     {badconfig, {"Value `~p' of option `~p' must be a list", {some_file, ct_first_files}}} = Error.
 
+testspec(Config) ->
+    C = rebar_test_utils:init_rebar_state(Config, "ct_testspec_"),
+
+    AppDir = ?config(apps, C),
+
+    Name = rebar_test_utils:create_random_name("ct_testspec_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
+    Spec = filename:join([AppDir, "test", "some.spec"]),
+    ok = filelib:ensure_dir(Spec),
+    ok = file:write_file(Spec, "[].\n"),
+
+    {ok, State} = rebar_test_utils:run_and_check(C,
+                                                 [],
+                                                 ["as", "test", "lock"],
+                                                 return),
+
+    Providers = rebar_state:providers(State),
+    Namespace = rebar_state:namespace(State),
+    CommandProvider = providers:get_provider(ct, Providers, Namespace),
+    GetOptSpec = providers:opts(CommandProvider),
+
+    {ok, GetOptResult1} = getopt:parse(GetOptSpec, ["--spec","test/some.spec"]),
+    State1 = rebar_state:command_parsed_args(State, GetOptResult1),
+    Tests1 = rebar_prv_common_test:prepare_tests(State1),
+    {ok, _} = rebar_prv_common_test:compile(State1, Tests1),
+
+    ok.
+
+cmd_vs_cfg_opts(Config) ->
+    C = rebar_test_utils:init_rebar_state(Config, "ct_cmd_vs_cfg_opts_"),
+
+    AppDir = ?config(apps, C),
+
+    Name = rebar_test_utils:create_random_name("ct_cmd_vs_cfg_opts_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
+
+    RebarConfig = [{ct_opts, [{spec,"mytest.spec"},
+                              {dir,"test"},
+                              {suite,"some_SUITE"},
+                              {group,"some_group"},
+                              {testcase,"some_test"}]}],
+
+    {ok, State} = rebar_test_utils:run_and_check(C, RebarConfig, ["as", "test", "lock"], return),
+
+    {ok, TestOpts} = rebar_prv_common_test:prepare_tests(State),
+    true = lists:member({spec, "mytest.spec"}, TestOpts),
+    true = lists:member({dir, "test"}, TestOpts),
+    true = lists:member({suite, "some_SUITE"}, TestOpts),
+    true = lists:member({group, "some_group"}, TestOpts),
+    true = lists:member({testcase, "some_test"}, TestOpts),
+
+    Providers = rebar_state:providers(State),
+    Namespace = rebar_state:namespace(State),
+    CommandProvider = providers:get_provider(ct, Providers, Namespace),
+    GetOptSpec = providers:opts(CommandProvider),
+
+    {ok, GetOptResult1} = getopt:parse(GetOptSpec, ["--spec","test/some.spec"]),
+    State1 = rebar_state:command_parsed_args(State, GetOptResult1),
+    {ok, TestOpts1} = rebar_prv_common_test:prepare_tests(State1),
+    true = lists:member({spec, ["test/some.spec"]}, TestOpts1),
+    false = lists:keymember(dir, 1, TestOpts1),
+    false = lists:keymember(suite, 1, TestOpts1),
+    false = lists:keymember(group, 1, TestOpts1),
+    false = lists:keymember(testcase, 1, TestOpts1),
+
+    {ok, GetOptResult2} = getopt:parse(GetOptSpec, ["--suite","test/some_SUITE"]),
+    State2 = rebar_state:command_parsed_args(State, GetOptResult2),
+    {ok, TestOpts2} = rebar_prv_common_test:prepare_tests(State2),
+    true = lists:member({suite, ["test/some_SUITE"]}, TestOpts2),
+    false = lists:keymember(spec, 1, TestOpts2),
+    false = lists:keymember(dir, 1, TestOpts2),
+    false = lists:keymember(group, 1, TestOpts2),
+    false = lists:keymember(testcase, 1, TestOpts2),
+
+    ok.
+
 %% helper for generating test data
 test_suite(Name) ->
     io_lib:format("-module(~ts_SUITE).\n"