finagle-redis tests need refactoring to from specs2 to scalatest

[penland365]

WIP Do not merge!

The refactoring required for replacing all specs tests with scalatests requires a few intermediate code reviews.
This commit removes none of the original tests, but adds refacorted tests from the Naggati codec and the Client
Integration tests. Code review is needed before continuing.

Tested w/

1. Scala 2.9.2
2. Redis 2.8.9
3. FreeBSD 10.0

Now that the formal stuff is out of the way, I want to ensure that the path I'm driving down for the test refactoring doesn't end up with me going off a cliff Thelma and Louise style. My goal on this path is:

1. DRY up some of the testing code
2. Separate out setup / tear down code from actual tests
3. Separate tests out by both functionality ( Codec / Client / Server ), and group them by Command. For example, there is a current file named SimpleClientSuite.scala that simply removed the "simple" tests from ClientSpec.scala. However, I find this difficult to grasp in practice ( how does one qualify a "simple" test? ) and would like to change that hierarchy to command groupings as Redis does. I show an example of this in the file KeyCommandsSuite.scala , which contains all codec tests for a command.

I'm sure I'm missing something or made something overly complicated. Please yell at me in an orderly fashion.

[mosesn]

Looks good to me! Thanks a ton for doing this 👍

[mosesn]

Let me know if there's anything you want help with here. Happy to help.

[penland365]

Thanks @mosesn . Sorry for the radio silence on this one - I got pulled into something at work that had to be finished before the quarter wrapped up. I'm grinding on this today and over the holiday weekend. I'll definitely ping you at some point for some guidance on this.

[mosesn]

No worries! I've appointed you the czar of finagle-redis 2.11-ification, so proceed at your own pace.

[penland365]

Is it alright if I roll one command into this ongoing PR? The command is a simple one, FLUSHALL .

Delete all the keys of all the existing databases, not just the currently selected one. This command
never fails.

I'm running into several areas in the refactor where tests are fouling due to unstated preconditions / postconditions. Right now I'm doing something like this when the client loaner is returned:

val dbs = List(1,2,3,4,5,6,7,8,9,10,11,12,13,14,15)
dbs.foreach( db => {
  Await.result(client(Select(db)))
  Await.result(client(FlushDB))
}

I can leave it as is for right now if you want to keep any new Commands out of the PR.

[mosesn]

Sounds good to me.

[penland365]

Alright @mosesn , now I do need some help ( or most likely a slap ). The last two commits show the completed outline of how I would finish this out. Here's a picture of the folder outline =>

I'm not gonna lie, I kinda hate the name KeyClientServerIntegrationSuite.scala because it feels so Captain Java Enterprise~y Dev. Also, the future file ServerClientServerIntegrationSuite.scala is causing me to rethink my profession and I haven't even written it yet.

Thoughts on the finalized outline? Naming suggestions?

[penland365]

FLUSHALL command added in the latest commit.

[mosesn]

Keys already provides a package namespace, maybe just call it
ClientIntegrationTest.Scala?

Looks good otherwise. We might want to consider rearranging finagle-redis
to match the package organization style here.
On Jul 4, 2014 4:31 PM, "Jeffrey N. Davis" notifications@github.com wrote:

Alright @mosesn https://github.com/mosesn , now I do need some help (
or most likely a slap ). The last two commits show the completed outline of
how I would finish this out. Here's a picture of the folder outline =>
[image: screen shot 2014-07-04 at 3 26 04 pm]
https://cloud.githubusercontent.com/assets/1630235/3484885/b39b7336-03b9-11e4-8496-7f8eb912b790.png

I'm not gonna lie, I kinda hate the name
KeyClientServerIntegrationSuite.scala because it feels so Captain Java
Enterprise~y Dev. Also, the future file
ServerClientServerIntegrationSuite.scala is causing me to rethink my
profession and haven't even written it yet.

Thoughts on the finalized outline? Naming suggestions?

—
Reply to this email directly or view it on GitHub
#283 (comment).

[penland365]

I like that naming, though it does leave something to be desired when reading the test output on the command line - it just shows two different ClientServerIntegrationTest, and you'd have to break out your context clues to determine which file it's actually in. Maybe that's OK, maybe we want to be more specific?

[mosesn]

I typically only look at command line output of tests when there's a failure, in which case they'll provide a stacktrace, which will have the full class name (with the packages). I think it's probably OK. Do you have another use case for parsing the class names?

[penland365]

Spent 15 minutes thinking about it and the answer is nope. Any report and/or command line output is going to print the fully qualified class name, which I hadn't thought about originally.

I'm in the middle of the LIST commands, then I'll change the names of the tests to lineup w/ this new schema.

Thanks @mosesn for talking it through with me . . . w/ stuff like this that's half the battle.

[mosesn]

@penland365 is there anything we can do to unblock you from this? Happy to help!

[penland365]

@mosesn My apologies, It's been crazy at work recently. I wish I had a better excuse than "there's only so much code my brain can process" in a week, but I don't. I'm going to grind on this this this weekend and hopefully have it wrapped up soon so I can unblock anyone working on redis related stuff.

Thank y'all for your patience.

[mosesn]

No need to apologize, thanks for keeping us in the loop. Take your time. Don't hurry on our account, just wanted to make sure it hadn't fallen off your radar. Hope work gets less crazy 😸

[penland365]

Hey, look, actual code! The whole mess of current LIST tests has been refactored this evening . . . one of the things that's really apparent while doing this is that some commands don't have the test coverage I thought they did when I first looked at those three huge scary files.

[mosesn]

For tests which are named things like Finagle_Suite or Finagle_Test, let's just drop the finagle bit. We already know they're finagle because they're in the finagle package.

The other thing is, sorry I didn't talk to you about this before, but I just noticed that you've been taking every assert and pulling it apart into three lines (actual/expected/assert). I think this is actually less readable, and since none of the vals is being reused, we don't end up with the lessening of cognitive load of being able to see how different bits fit together. Could you change that back?

[penland365]

Sure thing on both those. I'm currently in the middle of teasing apart the SET commands - I'll rewrite it in the updated style and push those specifically to make sure it fits.

On the multi-line test, I think I got started on that when I was trying to tease apart a few tests I didn't understand originally. I do think you're right though, it's too much boilerplate and too little readability.

[penland365]

Found time => I'm back. @mosesn , this last commit reflects ( I think ) your desire to collapse the assertions into one line. This last commit was the Set commands. Can you look at:

1. test/scala/com/twitter/finagle/redis/wip/commands/set/SetClientIntegrationSuite.scala
2. test/scala/com/twitter/finagle/redis/wip/commands/set/SetClientServerIntegrationSuite.scala
3. test/scala/com/twitter/finagle/redis/wip/commands/set/SetCodecSuite.scala

And let me know if that's the O.K. style? If it is, I'll go back and and swap out my original assertions.

[mosesn]

Yep, inlined asserts looks great. One last thing though, could you add the RunWith(JUnitRunner) annotation back in to the tests? We use junit for running our tests, and it won't be able to pick them up without the annotation.

[travisbrown]

Now in develop (see also #331).