Convert riak_pipe eunit system tests to riak_test #143

Merged
merged 4 commits into from Feb 19, 2013

Projects

None yet

2 participants

@beerriot
beerriot commented Jan 7, 2013

A belated New Years gift for @joedevivo: Riak Pipe's hairy eunit tests converted to riak_test.

This relies on basho/riak_pipe@4709002 from the bwf-riak_test branch of riak_pipe, to export riak_pipe:zero_part/1

This will allow us to remove the fragile setup/teardown logic in the riak_pipe module.

riak_test module <- riak_pipe eunit test
-------------------------------
pipe_verify_examples <- half of riak_pipe:basic_test_
pipe_verify_basics <- other half of riak_pipe:basic_test_
pipe_verify_exceptions <- riak_pipe:exception_test_
pipe_verify_handoff <- riak_pipe:limits_test_
pipe_verify_sink_types <- riak_pipe:sink_type_test_

Things I'm not entirely happy with, but which work:

  1. lots of rpc calls … kind of necessary, since pipe has no direct external interface
  2. lots of module injection … also kind of necessary, since we need the module from riak_test available on the cluster
  3. the cluster is not reset for each test … it would be trivial to change that, but would slow the tests significantly
  4. the handoff test had to be rewritten significantly … but I'm not sure it was actually working in its eunit state
  5. the choice of cluster size … everything except the handoff test was running with 1 node in eunit mode; I change to three nodes in most tests here just because I could so easily; exception being pipe_verify_exceptions because verify_worker_restart_failure_input_forwarding/1 uses ETS table heirs (which don't work cross-node) to track restarts
@beerriot beerriot referenced this pull request in basho/riak_pipe Jan 7, 2013
Merged

Move eunit system tests to riak_test #62

@joedevivo joedevivo commented on an outdated diff Jan 10, 2013
tests/pipe_verify_basics.erl
+ arg=testeoi}],
+ Options = [{sink, rt_pipe:self_sink()},{trace,[restart]},{log,sink}],
+ {ok, Pipe} = rpc:call(RN, riak_pipe, exec, [Spec, Options]),
+ ok = rpc:call(RN, riak_pipe, queue_work, [Pipe, 3]),
+ riak_pipe:eoi(Pipe),
+ {eoi, Res, Trc} = riak_pipe:collect_results(Pipe),
+ ?assertEqual([{counter,0},{counter,0},{counter,0},
+ {counter,1},{counter,2},{counter,3}],
+ Res),
+ try
+ [{counter,
+ {trace,[restart],{vnode,{restart,_}}}}] = Trc
+ catch error:{badmatch,[]} ->
+ %% If `Trace' is empty, the done/eoi race was
+ %% not triggered. So, in theory, we should
+ %% re-run the test. Except that EUnit tests
@joedevivo
joedevivo Jan 10, 2013

Food for thought: This ain't EUnit.

@joedevivo joedevivo commented on the diff Jan 10, 2013
tests/pipe_verify_examples.erl
+verify_example([RN|_]) ->
+ lager:info("Run riak_pipe:example/0"),
+ ?assertMatch({eoi, [{empty_pass, "hello"}], _Trc},
+ rpc:call(RN, riak_pipe, example, [])).
+
+verify_example_transform([RN|_]) ->
+ lager:info("Run riak_pipe:example_transform/0"),
+ ?assertEqual({eoi, [{"generic transform", 55}], []},
+ rpc:call(RN, riak_pipe, example_transform, [])).
+
+verify_example_reduce([RN|_]) ->
+ lager:info("Run riak_pipe:example_reduce/0"),
+ {eoi, Res, []} = rpc:call(RN, riak_pipe, example_reduce, []),
+ ?assertEqual([{"sum reduce", {a, [55]}},
+ {"sum reduce", {b, [155]}}],
+ lists:sort(Res)).
@joedevivo
joedevivo Jan 10, 2013

pipe_verify_examples +1

@joedevivo joedevivo commented on an outdated diff Jan 10, 2013
tests/pipe_verify_basics.erl
+ {counter,1},{counter,2},{counter,3}],
+ Res),
+ try
+ [{counter,
+ {trace,[restart],{vnode,{restart,_}}}}] = Trc
+ catch error:{badmatch,[]} ->
+ %% If `Trace' is empty, the done/eoi race was
+ %% not triggered. So, in theory, we should
+ %% re-run the test. Except that EUnit tests
+ %% aren't good at checking non-deterministic
+ %% tests.
+ lager:warning("Warning: recursive countdown test"
+ " #2 did not trigger the done/eoi"
+ " race it tests."
+ " Consider re-running.")
+ end.
@joedevivo
joedevivo Jan 10, 2013

pipe_verify_basics +1, though you could add some logic for rerunning verify_recursive_countdown_2

@joedevivo joedevivo commented on an outdated diff Jan 10, 2013
tests/pipe_verify_exceptions.erl
+ rpc:call(RN, riak_pipe, generic_transform,
+ [fun sleep1fun/1,
+ fun send_100_100/1,
+ ?ALL_LOG, 1]),
+
+ %% all inputs make it through, after blocking
+ ?assertEqual(100, length(Res)),
+
+ %% we get as many unblocking messages as blocking messages
+ Full = length(rt_pipe:extract_queue_full(Trace)),
+ NoLongerFull = length(rt_pipe:extract_unblocking(Trace)),
+ ?assertEqual(Full, NoLongerFull),
+
+ case Full of
+ [] ->
+ lager:warning("Queues were never full; consider rerunning");
@joedevivo
joedevivo Jan 10, 2013

you could programmatically rerun this.

@joedevivo joedevivo commented on an outdated diff Jan 10, 2013
tests/pipe_verify_exceptions.erl
+%%
+%% These tests used to be known as riak_pipe:exception_test_/0.
+
+-module(pipe_verify_exceptions).
+
+-export([
+ %% riak_test's entry
+ confirm/0
+ ]).
+
+-include_lib("eunit/include/eunit.hrl").
+
+%% local copy of riak_pipe.hrl
+-include("rt_pipe.hrl").
+
+%% must be 1 for verify_worker_restart_failure_input_forwarding
@joedevivo
joedevivo Jan 10, 2013

Do you want to roll that into a different test, so rest can be run on multi node clusters? nodes().

Bryan Fink added some commits Jan 7, 2013
Bryan Fink convert riak_pipe eunit system tests to riak_test
This will allow us to remove the fragile setup/teardown logic in the
riak_pipe module.

pipe_verify_examples <- half of riak_pipe:basic_test_
pipe_verify_basics <- other half of riak_pipe:basic_test_
pipe_verify_exceptions <- riak_pipe:exception_test_
pipe_verify_handof <- riak_pipe:limits_test_
pipe_verify_sink_types <- riak_pipe:sink_type_test_
cde2d2d
Bryan Fink use an intercept instead of riak_core_tracer in pipe handoff test
riak_core_tracer was not providing accurate operation counts, but
riak_pipe_vnode_intercept seems to
4775f25
Bryan Fink retry non-deterministic tests 10 times on failure
we may be able to do something smarter for
pipe_verify_exceptions:verify_queue_limit, like a coordinating process
as used in pipe_verify_handoff, but do the simple thing for now
95f6b72
Bryan Fink break pipe restart input forwarding into its own test
The input-forward-after-restart-failure test must run on a single node,
but it's nice to be able to run the others in pipe_verify_exceptions on
multi-node clusters. This commit splits them into different tests to
make that possible.
1652fda
@beerriot

I've just rebased to pick up support for intercepts, which are now used in the handoff test. I've also addressed Joe's comments up to this point.

@joedevivo joedevivo commented on the diff Feb 19, 2013
tests/pipe_verify_exceptions.erl
+verify_queue_limit(RN, Retries) when Retries > 0 ->
+ {eoi, Res, Trace} =
+ rpc:call(RN, riak_pipe, generic_transform,
+ [fun sleep1fun/1,
+ fun send_100_100/1,
+ ?ALL_LOG, 1]),
+
+ %% all inputs make it through, after blocking
+ ?assertEqual(100, length(Res)),
+
+ %% we get as many unblocking messages as blocking messages
+ Full = length(rt_pipe:extract_queue_full(Trace)),
+ NoLongerFull = length(rt_pipe:extract_unblocking(Trace)),
+ ?assertEqual(Full, NoLongerFull),
+
+ case Full of
@joedevivo
joedevivo Feb 19, 2013

Is there a way this test can keep trying until it's full?

@beerriot
beerriot Feb 19, 2013

The test could be rewritten, yes, to use the same blocking patterns used in pipe_verify_handoff. This is just a copy over of the tests that were already in riak_pipe's eunit.

@joedevivo
Test Results:
pipe_verify_sink_types-bitcask              : fail
pipe_verify_restart_input_forwarding-bitcask: pass
pipe_verify_handoff-bitcask                 : pass
pipe_verify_exceptions-bitcask              : fail
pipe_verify_examples-bitcask                : pass
pipe_verify_basics-bitcask                  : pass
---------------------------------------------
2 Tests Failed
4 Tests Passed
================ pipe_verify_sink_types failure stack trace =====================
{{badmatch,{badarg,fst,
                   ["chashfun",32,115,112,101,99,105,102,105,101,115,32,
                    "riak_pipe",58,"zero_part",47,"1",44,32,119,104,105,99,
                    104,32,105,115,32,110,111,116,32,101,120,112,111,114,116,
                    101,100]}},
 [{pipe_verify_sink_types,verify_fsm_sync_period,1,
                          [{file,"tests/pipe_verify_sink_types.erl"},
                           {line,139}]},
  {pipe_verify_sink_types,confirm,0,
                          [{file,"tests/pipe_verify_sink_types.erl"},
                           {line,47}]}]}
=================================================================================
================ pipe_verify_exceptions failure stack trace =====================
{{badmatch,{badarg,"worker limit mult pipes",
                   ["chashfun",32,115,112,101,99,105,102,105,101,115,32,
                    "riak_pipe",58,"zero_part",47,"1",44,32,119,104,105,99,
                    104,32,105,115,32,110,111,116,32,101,120,112,111,114,116,
                    101,100]}},
 [{pipe_verify_exceptions,verify_worker_limit_multiple,1,
                          [{file,"tests/pipe_verify_exceptions.erl"},
                           {line,393}]},
  {pipe_verify_exceptions,confirm,0,
                          [{file,"tests/pipe_verify_exceptions.erl"},
                           {line,63}]}]}
=================================================================================
@joedevivo

+1 to merge. This is a straight port of the tests from riak_pipe. There might be some ways to make a few of these tests more robust, but that will be handled by opening issues during the 1.3 test retrospective.

@beerriot beerriot merged commit 1652fda into master Feb 19, 2013
@beerriot beerriot pushed a commit that referenced this pull request Feb 19, 2013
Bryan Fink use records and utils now that they're available
The rt_pipe.hrl and rt_pipe.erl files were added in #143. This commit
cleans up the TODOs from #209.
3603645
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment