StefanKopieczek / gossip

SIP stack in Golang
GNU Lesser General Public License v2.1
336 stars 109 forks source link

Lots more testing #25

Closed rynorris closed 7 years ago

rynorris commented 9 years ago

Lots more testing! Proper UTs for transaction layer with mocked transport! UTs for mock timers module! Several bug fixes as a result!

Not ready to merge yet, some conntable tests now fail as a result of Elapse() no longer blocking on the timer channel. Not sure if there's a better way to fix them other than just putting in 50ms waits. There's no notification for when a connection expires, so there's no way for us to deterministically wait for it to happen.

rynorris commented 9 years ago

What do you think Re: conntable tests?

Essentially, there's a timing window after we call Elapse() until the conns actually expire. So sometimes the test continues before the expiry, and fails. It got away with it before, because Elapse() used to block on the timer channel, so the window was very small.

I made Elapse() not block (necessary, since before if it popped a timer nobody was listening to, your test would hang). So now the window is a bit larger, since the test exits out of Elapse a bit faster.

As I said above, there's no notification of actually when a conn has been retired, so there's no guaranteed way for us to know when it's expired. Do we just have to stick in a small wait to let it finish?

rynorris commented 8 years ago

Also worth noting that on master the tests fail spectacularly on Mac (hanging forever). On this branch they fail the same way as on Linux. Consistency is better.

StefanKopieczek commented 7 years ago

Eesh, catching up on PRs and just reread this @DiscoViking . Let me sleep on it and get back to you :)

rynorris commented 7 years ago

See latest commit for what I think is a reasonably neat fix for the conntable tests. All tests now pass on my machine at least.

gossip-jenkins commented 7 years ago

Can one of the admins verify this patch?

StefanKopieczek commented 7 years ago

@gossip-jenkins ok to test

StefanKopieczek commented 7 years ago

retest this please

StefanKopieczek commented 7 years ago

Yep, I think this is as clean as we're going to get. Happy to merge this; thanks for all your hard work putting these tests together, mate - nice work.