search

flightstats / hub

fault tolerant, highly available service for data storage and distribution
http://www.flightstats.com
MIT License
103 stars 35 forks source link

Add some tests around WebhookManager and ActiveWebhooks #1117

Closed lkemmerer closed 5 years ago

lkemmerer commented 5 years ago

I'm not sure this was all strictly necessary, but it helped me understand what was going on in these two classes. I opted to do integration tests for ActiveWebhooks because a lot of what it does is so ZK-specific and I'm not sure its behavior is directly tested anywhere else.

If this looks reasonable, I should probably deploy the branch to a dev cluster and make sure stuff is still behaving as expected. I'm still having problems remembering how the various hub configs change the implementation (local, test, aws...clustered vs single...) and sometimes change which tests run or what's asserted?

lkemmerer commented 5 years ago

@crozierm Looks like I can't assign you as a reviewer on this thing. Not sure why.

Exide commented 5 years ago

Aside from my comments I like what I see. Nice work.

Exide commented 5 years ago

👍

lkemmerer commented 5 years ago

i'm holding off on merging this until after the holidays. It's unlikely that there will be any critical bugs to fix in prod over the holidays, but I don't want this hanging around in master in case something comes up.