New cluster/handoff CLI coverage

[jadeallenx]

Add CLI test coverage for the new handoff and cluster commands added in riak_core from basho/riak_core#659, basho/riak_core#660 and basho/riak_core#661. These commands include

• handoff enable|disable
• handoff summary
• handoff details
• cluster status
• cluster partitions
• cluster partition-count
• cluster partition id|index
• cluster members

There is also coverage for the "set / show / describe" cuttlefish configuration values in riak_cli.

Also tracking #707 (this PR may conflict depending on merge order.)

[macintux]

Reviewing

[macintux]

Looks like something has changed. Here's a test run against EE 2.0.4 tag:

09:54:57.616 [info] "---- Cluster Status ----\nRing ready: true\n\n+--------------------+------+-------+-----+-------+\n|        node        |status| avail |ring |pending|\n+--------------------+------+-------+-----+-------+\n| (C) [email protected] |valid |  up   |100.0|  --   |\n+--------------------+------+-------+-----+-------+\n\nKey: (C) = Claimant; availability marked with '!' is unexpected\n"
09:54:57.616 [error] Error in process <0.189.0> on node '[email protected]' with exit value: {{assertEqual_failed,[{module,riak_admin_console_tests},{line,283},{expression,"Out"},{expected,"pass"},{value,"---- Cluster Status ----\nRing ready: true\n\n+--------------------+------+-------+-----+-------+\n|   ...


09:54:57.616 [warning] riak_admin_console_tests failed: {{assertEqual_failed,[{module,riak_admin_console_tests},{line,283},{expression,"Out"},{expected,"pass"},{value,"---- Cluster Status ----\nRing ready: true\n\n+--------------------+------+-------+-----+-------+\n|        node        |status| avail |ring |pending|\n+--------------------+------+-------+-----+-------+\n| (C) [email protected] |valid |  up   |100.0|  --   |\n+--------------------+------+-------+-----+-------+\n\nKey: (C) = Claimant; availability marked with '!' is unexpected\n"}]},[{riak_admin_console_tests,'-check_admin_cmd/2-fun-1-',2,[{file,"tests/riak_admin_console_tests.erl"},{line,283}]},{riak_admin_console_tests,cluster_tests,1,[{file,"tests/riak_admin_console_tests.erl"},{line,57}]},{riak_admin_console_tests,confirm,0,[{file,"tests/riak_admin_console_tests.erl"},{line,272}]},{riak_test_runner,return_to_exit,3,[{file,"src/riak_test_runner.erl"},{line,159}]}]}
09:54:57.617 [error]
================ riak_admin_console_tests failure stack trace =====================
{{assertEqual_failed,[{module,riak_admin_console_tests},
                      {line,283},
                      {expression,"Out"},
                      {expected,"pass"},
                      {value,"---- Cluster Status ----\nRing ready: true\n\n+--------------------+------+-------+-----+-------+\n|        node        |status| avail |ring |pending|\n+--------------------+------+-------+-----+-------+\n| (C) [email protected] |valid |  up   |100.0|  --   |\n+--------------------+------+-------+-----+-------+\n\nKey: (C) = Claimant; availability marked with '!' is unexpected\n"}]},
 [{riak_admin_console_tests,'-check_admin_cmd/2-fun-1-',2,
                            [{file,"tests/riak_admin_console_tests.erl"},
                             {line,283}]},
  {riak_admin_console_tests,cluster_tests,1,
                            [{file,"tests/riak_admin_console_tests.erl"},
                             {line,57}]},
  {riak_admin_console_tests,confirm,0,
                            [{file,"tests/riak_admin_console_tests.erl"},
                             {line,272}]},
  {riak_test_runner,return_to_exit,3,
                    [{file,"src/riak_test_runner.erl"},{line,159}]}]}
===================================================================================

[jadeallenx]

Hmm, this looks a bit hairy. Will have a think on it.

OK, so the current test grabs the output from rt and does some simple comparisons. For example...

The tests for riak-admin member-status use this intercept which is implemented here and quoted below:

verify_console_member_status(Val) ->
    case Val of
        [] -> ?PASS;
        _ -> ?FAIL
    end.

Each one of the console commands is hooked to its implementation function. With the new handoff commands we are interacting with just a single function riak_core_console:command/1 so it's not clear that the current test pattern of hooking implementation functions is the right one.

[andrewjstone]

@mrallen1 I think I see your confusion. I took a look at the tests as well and the output of those intercept function calls can't possibly match Val. I then realized that Val is the list of arguments passed to the function being intercepted. Therefore these tests are only testing that the commands parsed by the shell result in the right callback function firing when called from bash via nodetool.

In our case, since we only call one function from nodetool and the parsing and callbacks are handled by clique, we'd have to make sure we intercept the registered callback called from clique. However, since clique handles parse errors automatically and won't call the callback in that case, we couldn't do negative testing this way. It would however test that the commands are actually registered correctly and get called when given a correct command line string. I'd give that a try. You wouldn't even have to check that the arguments were right. If the function got called it is correct, so you just return ?PASS.

[andrewjstone]

I'm now wondering what happens if the intercept doesn't fire because a bad string is passed. I was thinkng there would be a timeout, and the test would fail, but now I'm not so sure. I bet if it's just not called and the test passes. In that case it makes these tests pretty useless, since you can't tell when they actually pass or fail. Something to look into @mrallen1.

[jadeallenx]

OK, thanks @andrewjstone - I will check into it more closely tomorrow.

[jadeallenx]

@andrewjstone @macintux Please review.

This is what I'm getting:

riak_admin_console_tests-bitcask: pass
---------------------------------------------
0 Tests Failed
1 Tests Passed
That's 100.0% for those keeping score

⚡

[andrewjstone]

@mrallen1 After looking at the comment at the top of this file, which I should have done in the first place, I realize that this test is only intended to check for bash portability bugs via intercepts. That's why it only checks the input to the intercepted function, as I noted in my previous comments.

Your tests check the output of the calls to ensure they return something. So while they do indeed prove that a correct round trip call occurred, they are quite brittle as they break with a changing message, which is not really the thing we are gaining by testing with them.

As noted, handoff commands aren't directly parsed by bash, so the original purpose of the test is not necessary for our new clique based commands. Since they aren't really useful for the purpose of this riak_test, and are very brittle, I'd say we just don't bother updating this test the new cli stuff at all.

[macintux]

Mark and I discussed the incongruity of his tests vs what was being done already; thanks for catching why.

I agree, makes more sense rely on clique's tests. Fortunately, Mark and I both learned a fair bit about the intercepts and riak_test in general while working on this, so definitely not wasted energy.

[jadeallenx]

OK, in light of that, I'm going to close this PR. Thanks for the feedback.