search

disqus / pgshovel

A change data capture system for PostgreSQL
Apache License 2.0
11 stars 3 forks source link

Fix tests in replication branch #22

Closed Fluxx closed 9 years ago

Fluxx commented 9 years ago

Fixed the tests in the replication branch to be passing. In general tried to do my best to keep the spirit of the original tests, while adapting them to my understanding of the new behavior. There is also a decent amount of code duplication and there is some refactoring that I want to do with things once I get better oriented with the codebase, so I didn't concentrate or worry about that with this diff.

Fluxx commented 9 years ago

@tkaemming just added back that missing file. If everything looks good you wanna accept?

tkaemming commented 9 years ago

Overall this looks good, but it probably would make sense for the consumer validation tests to check the ConsumerState returned is valid, I think. Adding some coverage for the dead publisher behavior might be good too.

Fluxx commented 9 years ago

Overall this looks good, but it probably would make sense for the consumer validation tests to check the ConsumerState returned is valid, I think. Adding some coverage for the dead publisher behavior might be good too.

I'm working on another diff right now that adds tests, so I think I'll add tests for these things with that diff - should be early next week.

Fluxx commented 9 years ago

@tkaemming unless you have any further blockers, I'm going to land this. I'm currently working on new tests (WIP preview at https://github.com/disqus/pgshovel/compare/replication...new_tests), and plan to add tests for:

EDIT: Actually there appears to already be a test for dead publisher behavior?

https://github.com/disqus/pgshovel/blob/replication_fix_tests/tests/pgshovel/validation/consumers.py#L58-L71

fuziontech commented 9 years ago

Everything seems to make sense to me. :+1:

Fluxx commented 9 years ago

Last call on this, gonna merge before lunch.