coreos/etcd — pull request #251 by benbjohnson (2013-10-18 15:31:09+00:00)
Title: v2 Handler Unit Tests

## Overview

This pull request includes nearly full coverage of the v2 handlers. I originally started adding mocks but hit a roadblock where Gorilla mux is really difficult to stub unless we hack it. So I ended up doing full end-to-end tests by running a single node `Server`/`PeerServer`, executing the test, and then stopping it. The v2 handler test suite is still fast (~200ms) -- just not as fast as mocking. But it does give us coverage that's more representative of actual use.

## Notes

There are a couple things I came across while adding coverage:

1. Executing a CAS with only `prevIndex` was throwing an error before. It expected a `prevValue` as well. This is fixed now.

2. Should `POST /v2/keys/:key` add a monotonically increasing value to the directory? Currently it adds the current Raft index so it can jump values within a single directory if multiple directories are being POST'd to (e.g. `/foo/3`, `/bar/4`, `/foo/5`). I don't think this is a problem but I just wanted to bring it up to make sure the expectation was correct.

3. Executing a `wait` with a `waitIndex=5` will pick up a change where the `index` is `4`. If I set `waitIndex=4` then I won't get the `index=4` change. This seems like a bug but maybe I don't understand the wait stuff well enough.

4. I had to copy over the `http.Server#ListenAndServe` implementation from `net/http` so that we can capture the listener and close it for each test. The implementation is fairly small but I wanted to give you a heads up. I had to do the same thing to do integration tests on Sky.

5. I added @stretchr's [testify](https://github.com/stretchr/testify) library. I know we're trying to limit external dependencies but it's a nice, simple assertion & mocking implementation. It really helps to clean up the tests by uisng `assert.Equal()` instead of `if ... { t.Fatal("msg") }` all over.

## Known Bugs

There are a couple of bugs that I found that I didn't fix. I wanted to get your thoughts first.

1. `prevIndex` only works on existing values. It seems like this should work on any key. It gets complicated because we have to start moving the `Create` functionality into `CompareAndSwap`. Again, I think the `Create`/`Update`/`Set`/`CAS` functionality can be combined into a single function.

2. The failure message for `prevIndex` is not clear: `{"errorCode":101,"message":"Test Failed","cause":"[ != XXX] [10 != 3]","index":4,"term":0}`. The `cause` combines `prevValue` and `prevIndex` without stating which one is which. In this case, I'm setting `XXX` as the new value and I'm using `prevIndex=10` but the `index` is actually `3`.

Overall, the `v2` handlers are working pretty well. I can keep adding coverage to the rest of the handers too. I'd like to speed up the `store` tests too. Right now they take 20+ seconds to run.

I'll add line notes to clarify some of this PR as well.

/cc: @xiangli-cmu @philips