Convert giddyup module into a gen_server

- Always copy log files to local directory
- Removed unused command-line options in escript
- Get rid of extraneous commented-out code
- Move as much GiddyUp-specific logic into `giddyup'
- Correctly upload config files
- Upload files asynchronously to scheduler can reclaim used nodes

Author: Brett Hazen

src/giddyup.erl

@@ -36,11 +36,19 @@ main(Args) ->
     ensure_dir(OutDir),
     lager_setup(OutDir),
 
-    {Tests, NonTests} = generate_test_lists(ParsedArgs),
+    %% Do we use GiddyUp for this run?
+    Platform = report_platform(ParsedArgs),
+    UseGiddyUp = case Platform of
+                     undefined -> false;
+                     _ -> true
+                 end,
+    start_giddyup(Platform),
+    {Tests, NonTests} = generate_test_lists(UseGiddyUp, ParsedArgs),
 
     ok = prepare(ParsedArgs, Tests, NonTests),
     Results = execute(Tests, OutDir, ParsedArgs),
-    finalize(Results, ParsedArgs).
+    finalize(Results, ParsedArgs),
+    stop_giddyup(UseGiddyUp).
 
 % @doc Validate the command-line options
 parse_args(Args) ->
@@ -64,14 +72,11 @@ cli_options() ->
  {help,                   $h, "help",     undefined,  "Print this usage page"},
  {config,                 $c, "conf",     string,     "specifies the project configuration"},
  {tests,                  $t, "tests",    string,     "specifies which tests to run"},
- {suites,                 $s, "suites",   string,     "which suites to run"},
- {groups,                 $g, "groups",   string,     "specifiy a list of test groups to run"},
  {dir,                    $d, "dir",      string,     "run all tests in the specified directory"},
  {skip,                   $x, "skip",     string,     "list of tests to skip in a directory"},
  {verbose,                $v, "verbose",  undefined,  "verbose output"},
  {outdir,                 $o, "outdir",   string,     "output directory"},
  {backend,                $b, "backend",  atom,       "backend to test [memory | bitcask | eleveldb]"},
- {upgrade_path,           $u, "upgrade-path", atom,   "comma-separated list representing an upgrade path (e.g. riak-1.3.4,riak_ee-1.4.12,riak_ee-2.0.0)"},
  {keep,            undefined, "keep",     boolean,    "do not teardown cluster"},
  {continue_on_fail,       $n, "continue", boolean,    "continues executing tests on failure"},
  {report,                 $r, "report",   string,     "you're reporting an official test run, provide platform info (e.g. ubuntu-1404-64)\nUse 'config' if you want to pull from ~/.riak_test.config"},
@@ -93,13 +98,13 @@ report_platform(ParsedArgs) ->
     end.
 
 %% @doc Print help string if it's specified, otherwise parse the arguments
-generate_test_lists(ParsedArgs) ->
+generate_test_lists(UseGiddyUp, ParsedArgs) ->
     %% Have to load the `riak_test' config prior to assembling the
     %% test metadata
 
     TestData = compose_test_data(ParsedArgs),
-    Backend = proplists:get_value(backend, ParsedArgs, bitcask),
-    {Tests, NonTests} = wrap_test_in_test_plan(report_platform(ParsedArgs), Backend, TestData),
+    Backends = [proplists:get_value(backend, ParsedArgs, bitcask)],
+    {Tests, NonTests} = wrap_test_in_test_plan(UseGiddyUp, Backends, TestData),
     Offset = rt_config:get(offset, undefined),
     Workers = rt_config:get(workers, undefined),
     shuffle_tests(Tests, NonTests, Offset, Workers).
@@ -168,24 +173,6 @@ execute(TestPlans, OutDir, ParsedArgs) ->
                                                    self()),
     wait_for_results(Executor, [], length(TestPlans), 0).
 
-%% TestResults = run_tests(Tests, Outdir, Report, HarnessArgs),
-%% lists:filter(fun results_filter/1, TestResults).
-
-%% run_test(Test, Outdir, TestMetaData, Report, HarnessArgs, NumTests) ->
-%%     rt_cover:maybe_start(Test),
-%%     SingleTestResult = riak_test_runner:run(Test, Outdir, TestMetaData, HarnessArgs),
-
-%% case NumTests of
-%%     1 -> keep_them_up;
-%%     _ -> rt_cluster:teardown()
-%% end,
-
-%% TODO: Do this in the test runner
-%%     CoverDir = rt_config:get(cover_output, "coverage"),
-%% CoverFile = rt_cover:maybe_export_coverage(Test, CoverDir, erlang:phash2(TestMetaData)),
-%% publish_report(SingleTestResult, CoverFile, Report),
-
-%%     [{coverdata, CoverFile} | SingleTestResult].
 
 %% TODO: Use `TestCount' and `Completed' to display progress output
 wait_for_results(Executor, TestResults, TestCount, Completed) ->
@@ -249,9 +236,6 @@ erlang_setup(_ParsedArgs) ->
     register(riak_test, self()),
     maybe_add_code_path("./ebin"),
 
-    %% ibrowse
-    load_and_start(ibrowse),
-
     %% Sets up extra paths earlier so that tests can be loadable
     %% without needing the -d flag.
     code:add_paths(rt_config:get(test_paths, [])),
@@ -274,10 +258,6 @@ maybe_add_code_path(Path, true) ->
 maybe_add_code_path(_, false) ->
     meh.
 
-load_and_start(Application) ->
-    application:load(Application),
-    application:start(Application).
-
 ensure_dir(undefined) ->
     ok;
 ensure_dir(Dir) ->
@@ -358,21 +338,18 @@ extract_test_names(Test, {CodePaths, TestNames}) ->
 %% @doc Determine which tests to run based on command-line argument
 %%      If the platform is defined, consult GiddyUp, otherwise just shovel
 %%      the whole thing into the Planner
--spec(load_up_test_planner(string() | undefined, string(), list()) -> list()).
-load_up_test_planner(undefined, Backend, CommandLineTests) ->
-    [rt_planner:add_test_plan(Name, undefined, Backend, undefined, undefined) || Name <- CommandLineTests];
-%% GiddyUp Flavor
-load_up_test_planner(Platform, Backend, CommandLineTests) ->
-    rt_planner:load_from_giddyup(Platform, Backend, CommandLineTests).
+-spec(load_up_test_planner(boolean(), [string()], list()) -> list()).
+load_up_test_planner(true, Backends, CommandLineTests) ->
+    rt_planner:load_from_giddyup(Backends, CommandLineTests);
+load_up_test_planner(_, Backends, CommandLineTests) ->
+    [rt_planner:add_test_plan(Name, undefined, Backends, undefined, undefined) || Name <- CommandLineTests].
 
 %% @doc Push all of the test into the Planner for now and wrap them in an `rt_test_plan'
 %% TODO: Let the Planner do the work, not the riak_test_executor
--spec(wrap_test_in_test_plan(string(), string(), [atom()]) -> {list(), list()}).
-wrap_test_in_test_plan(Platform, Backend, CommandLineTests) ->
-    %% ibrowse neededfor GiddyUp
-    load_and_start(ibrowse),
+-spec(wrap_test_in_test_plan(boolean(), [string()], [atom()]) -> {list(), list()}).
+wrap_test_in_test_plan(UseGiddyUp, Backends, CommandLineTests) ->
     {ok, _Pid} = rt_planner:start_link(),
-    load_up_test_planner(Platform, Backend, CommandLineTests),
+    load_up_test_planner(UseGiddyUp, Backends, CommandLineTests),
     TestPlans = [rt_planner:fetch_test_plan() || _ <- lists:seq(1, rt_planner:number_of_plans())],
     NonRunnableTestPlans = [rt_planner:fetch_test_non_runnable_plan() || _ <- lists:seq(1, rt_planner:number_of_non_runable_plans())],
     rt_planner:stop(),
@@ -407,26 +384,6 @@ match_group_attributes(Attributes, Groups) ->
                            || Group <- Groups, TestType <- TestTypes ])
     end.
 
-%% run_tests(Tests, Outdir, Report, HarnessArgs) ->
-    %% Need properties for tests prior to getting here Need server to
-    %% manage the aquisition of nodes and to handle comparison of test
-    %% `node_count' property with resources available. Also handle
-    %% notification of test completion. Hmm, maybe test execution
-    %% should be handled by a `gen_fsm' at this point to distinguish
-    %% the case when there are tests left to be tried with available
-    %% resources versus all have been tried or resources are
-    %% exhausted.
-
-%% [run_test(Test,
-%%                             Outdir,
-%%                             TestMetaData,
-%%                             Report,
-%%                             HarnessArgs,
-%%                             TestCount) ||
-%%                       {Test, TestMetaData} <- Tests],
-
-
-
 backend_list(Backend) when is_atom(Backend) ->
     atom_to_list(Backend);
 backend_list(Backends) when is_list(Backends) ->
@@ -493,3 +450,22 @@ so_kill_riak_maybe() ->
             io:format("Leaving Riak Up... "),
             rt:whats_up()
     end.
+
+%% @doc Start the GiddyUp reporting service if the report is defined
+start_giddyup(undefined) ->
+    ok;
+start_giddyup(Platform) ->
+    {ok, _Pid} = giddyup:start_link(Platform,
+                                    rt_config:get_default_version_product(),
+                                    rt_config:get_default_version_number(),
+                                    rt_config:get_default_version(),
+                                    rt_config:get(giddyup_host),
+                                    rt_config:get(giddyup_user),
+                                    rt_config:get(giddyup_password)).
+
+%% @doc Stop the GiddyUp reporting service if the report is defined
+stop_giddyup(true) ->
+    giddyup:stop();
+stop_giddyup(_) ->
+    ok.
+

src/riak_test_escript.erl

@@ -63,7 +63,7 @@
          expect_in_log/2,
          get_deps/0,
          get_ip/1,
-         get_node_logs/3,
+         get_node_logs/2,
          get_replica/5,
          get_ring/1,
          get_version/0,
@@ -1078,11 +1078,10 @@ pmap(F, L) ->
 setup_harness(_Test, _Args) ->
     rt_harness:setup().
 
-%% @doc Copy all of the nodes' log files to a local dir or
-%% open a port to each file to upload to GiddyUp
--spec(get_node_logs(boolean(), string(), string()) -> list()).
-get_node_logs(UploadToGiddyUp, LogFile, DestDir) ->
-    rt2:get_node_logs(UploadToGiddyUp, LogFile, DestDir).
+%% @doc Copy all of the nodes' log files to a local dir
+-spec(get_node_logs(string(), string()) -> list()).
+get_node_logs(LogFile, DestDir) ->
+    rt2:get_node_logs(LogFile, DestDir).
 
 check_ibrowse() ->
     rt2:check_ibrowse().

src/rt.erl

@@ -38,7 +38,7 @@
          expect_in_log/2,
          get_deps/0,
          get_ip/1,
-         get_node_logs/3,
+         get_node_logs/2,
          get_replica/5,
          get_version/0,
          is_mixed_cluster/1,
@@ -610,8 +610,8 @@ setup_harness(Test, Args) ->
 
 %% @doc Downloads any extant log files from the harness's running
 %%   nodes.
-get_node_logs(UploadToGiddyUp, LogFile, DestDir) ->
-    rt_harness:get_node_logs(UploadToGiddyUp, LogFile, DestDir).
+get_node_logs(LogFile, DestDir) ->
+    rt_harness:get_node_logs(LogFile, DestDir).
 
 check_ibrowse() ->
     try sys:get_status(ibrowse) of

src/rt2.erl

@@ -178,7 +178,7 @@ get_version(Vsn) ->
 resolve_version(Vsn, Versions) ->
     case find_atom_or_string(Vsn, Versions) of
         undefined ->
-            erlang:error("Could not find version", [Vsn]);
+            erlang:error("Could not find Riak version", [Vsn]);
         {Product, Tag} ->
             convert_to_string(Product) ++ "-" ++ convert_to_string(Tag);
         Version ->

src/rt_config.erl

@@ -37,7 +37,7 @@
          cmd/2,
          setup/0,
          get_deps/0,
-         get_node_logs/3,
+         get_node_logs/2,
          get_version/0,
          get_version/1,
          get_backends/0,
@@ -107,8 +107,8 @@ get_version(Node) ->
 get_backends() ->
     ?HARNESS_MODULE:get_backends().
 
-get_node_logs(UploadToGiddyUp, LogFile, DestDir) ->
-    ?HARNESS_MODULE:get_node_logs(UploadToGiddyUp, LogFile, DestDir).
+get_node_logs(LogFile, DestDir) ->
+    ?HARNESS_MODULE:get_node_logs(LogFile, DestDir).
 
 set_backend(Backend) ->
     ?HARNESS_MODULE:set_backend(Backend).

src/rt_harness.erl

@@ -36,7 +36,7 @@
 
 %% API
 -export([start_link/0,
-         load_from_giddyup/3,
+         load_from_giddyup/2,
          add_test_plan/5,
          fetch_test_plan/0,
          fetch_test_non_runnable_plan/0,
@@ -82,19 +82,19 @@ start_link() ->
 %%
 %% @end
 %%--------------------------------------------------------------------
--spec(load_from_giddyup(string(), string() | undefined, list()) -> ok).
-load_from_giddyup(Platform, Backend, CommandLineTests) ->
-    gen_server:call(?MODULE, {load_from_giddyup, Platform, Backend, CommandLineTests}).
+-spec(load_from_giddyup([string()] | undefined, list()) -> ok).
+load_from_giddyup(Backends, CommandLineTests) ->
+    gen_server:call(?MODULE, {load_from_giddyup, Backends, CommandLineTests}).
 
 %%--------------------------------------------------------------------
 %% @doc
 %% Queue up a new test plan
 %%
 %% @end
 %%--------------------------------------------------------------------
--spec(add_test_plan(string(), string(), rt_properties2:storage_backend(), rt_properties2:product_version(), rt_properties2:properties()) -> ok).
-add_test_plan(Module, Platform, Backend, Version, Properties) ->
-    gen_server:call(?MODULE, {add_test_plan, Module, Platform, Backend, Version, Properties}).
+-spec(add_test_plan(string(), string(), [string()], rt_properties2:product_version(), rt_properties2:properties()) -> ok).
+add_test_plan(Module, Platform, Backends, Version, Properties) ->
+    gen_server:call(?MODULE, {add_test_plan, Module, Platform, Backends, Version, Properties}).
 
 %%--------------------------------------------------------------------
 %% @doc
@@ -185,22 +185,33 @@ init([]) ->
     {stop, Reason :: term(), NewState :: #state{}}).
 %% Run only those GiddyUp tests which are specified on the command line
 %% If none are specified, run everything
-handle_call({load_from_giddyup, Platform, _Backend, CommandLineTests}, _From, State) ->
-    AllGiddyupTests = giddyup:get_suite(Platform),
-    FilteredTests = case CommandLineTests of
+handle_call({load_from_giddyup, Backends, CommandLineTests}, _From, State) ->
+    AllGiddyupTests = giddyup:get_test_plans(),
+    FilteredNames = case CommandLineTests of
         [] ->
-            [test_plan_from_giddyup(Test) || Test <- AllGiddyupTests];
+            AllGiddyupTests;
         _ ->
-            [test_plan_from_giddyup({GName, GMetaData}) || {GName, GMetaData} <- AllGiddyupTests,
-                                                                        CName <- CommandLineTests,
-                                                                        GName =:= CName]
+            [TestPlan || TestPlan <- AllGiddyupTests,
+                                     CName <- CommandLineTests,
+                                     rt_test_plan:get_module(TestPlan) =:= CName]
+        end,
+    FilteredTests = case Backends of
+        undefined ->
+            FilteredNames;
+        _ ->
+            [TestPlan || TestPlan <- FilteredNames,
+                                     lists:member(rt_test_plan:get(backend, TestPlan), Backends)]
         end,
     State1 = lists:foldl(fun sort_and_queue/2, State, FilteredTests),
     {reply, ok, State1};
 %% Add a single test plan to the queue
-handle_call({add_test_plan, Module, Platform, Backend, _Version, _Properties}, _From, State) ->
-    TestPlan = rt_test_plan:new([{module, Module}, {platform, Platform}, {backend, Backend}]),
-    {reply, ok, sort_and_queue(TestPlan, State)};
+handle_call({add_test_plan, Module, Platform, Backends, _Version, _Properties}, _From, State) ->
+    State1 = lists:foldl(fun(Backend, AccState) ->
+            TestPlan = rt_test_plan:new([{module, Module}, {platform, Platform}, {backend, Backend}]),
+            sort_and_queue(TestPlan, AccState)
+        end,
+        State, Backends),
+    {reply, ok, State1};
 handle_call(fetch_test_plan, _From, State) ->
     Q = State#state.runnable_test_plans,
     {Item, Q1} = queue:out(Q),
@@ -291,37 +302,6 @@ code_change(_OldVsn, State, _Extra) ->
 %%% Internal functions
 %%%===================================================================
 
-%%--------------------------------------------------------------------
-%% @private
-%% @doc
-%% Translate GiddyUp Output into an `rt_test_plan' record
-%%
-%% @end
-%%--------------------------------------------------------------------
-
--spec(set_giddyup_field(atom(), {proplists:proplist(), rt_test_plan:test_plan()}) -> rt_test_plan:test_plan()).
-set_giddyup_field(Field, {MetaData, TestPlan}) ->
-    {ok, TestPlan1} = case proplists:is_defined(Field, MetaData) of
-        true ->
-            rt_test_plan:set(Field, proplists:get_value(Field, MetaData), TestPlan);
-        _ ->
-            {ok, TestPlan}
-    end,
-    {MetaData, TestPlan1}.
-
--spec(test_plan_from_giddyup({atom(), term()}) -> rt_test_plan:test_plan()).
-test_plan_from_giddyup({Name, MetaData}) ->
-    Plan0 = rt_test_plan:new([{module, Name}]),
-    GiddyUpFields = [id, backend, platform, project],
-    {_, Plan1} = lists:foldl(fun set_giddyup_field/2, {MetaData, Plan0}, GiddyUpFields),
-    %% Special treatment for the upgrade path
-    {ok, Plan2} = case proplists:is_defined(upgrade_version, MetaData) of
-        true ->
-            rt_test_plan:set(upgrade_path, [rt_config:get_version(proplists:get_value(upgrade_version, MetaData)), rt_config:get_default_version()], Plan1);
-        _ ->
-            {ok, Plan1}
-    end,
-    Plan2.
 
 %%--------------------------------------------------------------------
 %% @private
@@ -350,10 +330,3 @@ is_runnable_test_plan(TestPlan) ->
     code:ensure_loaded(Mod),
     erlang:function_exported(Mod, Fun, 0) orelse
         erlang:function_exported(Mod, Fun, 1).
-
--ifdef(TEST).
-set_giddyup_field_test() ->
-    S = {#state{runnable_test_plans =queue:new()},[]},
-    T = test_plan_from_giddyup({test, [{id, 5},{backend,riak_kv_eleveldb_backend}, {platform, "os-x"}, {version, "2.0.5"},{project,<<"riak_ee">>}]}, S),
-    ?assertEqual(T, {#state{runnable_test_plans = {[{rt_test_plan_v1,5,test,<<"riak_ee">>,"os-x",riak_kv_eleveldb_backend,[],undefined}], []}},[]}).
--endif.