broker tests #67 #68
broker tests #67 #68
Conversation
ryanstinson
commented
Oct 23, 2017
•
ryanstinson
commented
- Test broker: New()
- Broker struct is initialized properly
- Serf.Bootstrap is called correctly
- Raft.Bootstrap is called correctly
- Handle all error cases
- Test broker: Join()
- Add test for serf error
- Test broker: partition()
- Add test for empty topic
- Test broker: startReplica()
- Add test for starting as leader
- Add test for starting as follower
- Add test for existing topic
- Add test for duplicate partition
- Add test for commitlog New() error
- Test Broker New - Broker struct is initialized properly - Serf.Bootstrap is called correctly - Raft.Bootstrap is called correctly - Handle all error cases - 100% coverage for Broker New
- Test Broker New - Broker struct is initialized properly - Serf.Bootstrap is called correctly - Raft.Bootstrap is called correctly - Handle all error cases - 100% coverage for Broker New
- Add missing packages
|
I wrote some similar tests this weekend, can you update your PR, you've got some more error condition tests we can add. Thanks. |
|
Just saw the commit as I got the merge conflicts. I'll merge yours in and keep the new tests. Going to continue testing Broker, so leave the rest to me unless you'd like to do the rest yourself. |
|
@ryanstinson sounds good. I was going to hack on the tests because I feel like them lacking is holding back the development of everything else, but if you can push those forward in the next 2-3 days I'll wait. I just pushed another change removing the individual/private handler tests because I want to test all those through the public Run api like the existing api versions handling is tested - so make sure you pull that before continuing. Can you push up your changes piecemeal, whenever you have something that works to look at, I just don't want to waste your time pushing up something I don't like/doesn't fit. Thanks for the help! |
- Add broker/the-topic test folder to gitignore
|
@travisjeffery I agree, my goal is to help you get broker tested 100% ASAP. I will pull your changes and submit small pieces at a time. Please give feedback regarding the tests for Broker New and don't hold back - I have a thick skin. The more feedback, the better my contributions will be going forward. Thanks. |
- Merge broker: test New() from @travisjeffery
|
@travisjeffery merged our broker: test New() together. I made some changes to be able to use deep copy for comparing expected Broker to actual. With this merge we are at 21.1% code coverage on Broker - only 78.9% to go. |
- Fix mock serf JoinFn
|
@travisjeffery Regarding the individual private handlers for Broker, do you think those handlers should be pulled out as a separate func with a handler interface? That way they could also be independent and would reduce the Broker code. |
|
@ryanstinson the way they're setup is ok for now i think, there's isn't much the same between them as they are. did you merge or rebase master? looks like no, i'd recommend doing that before continuing. or create a new branch off master and start fresh with your changes. |
Broker tests
|
@travisjeffery Yes, I am completely merged up to your latest commit. I just confirmed. |
- Test broker: Join() - Add test for serf error - Test broker: partition() - Add test for empty topic - Test broker: startReplica() - Add test for starting as leader - Add test for starting as follower - Add test for existing topic - Add test for duplicate partition - Add test for commitlog New() error
|
@travisjeffery Can you merge this? |
|
Thanks @ryanstinson, merged. Checkout 6553b9a for some fixes on the style, test names. The way you had them named were more topical, I want them named more for the reason the test is there, for instance if it's testing an error, instead of saying "new broker port error"/"broker port error" I want it to say "no broker port error" |
|
@travisjeffery Got it. I reviewed your changes and will follow that naming convention going forward. |
