praekeltfoundation / vumi

Messaging engine for the delivery of SMS, Star Menu and chat messages to diverse audiences in emerging markets and beyond.
BSD 3-Clause "New" or "Revised" License
420 stars 131 forks source link

Fake connections for tests #974

Closed jerith closed 8 years ago

jerith commented 8 years ago

Using real network connections (even over localhost) in tests is unnecessary in most cases and leads to occasional random build failures.

This branch uses the same ideas as the recently-added FakeSMSC to test various other transports, applications, and utilities.

Review on Reviewable

jerith commented 8 years ago

I still want to switch all the remaining transport tests over, but the current state is ready for review. (I've split up the work into separate commits that can be reviewed independently.)

jerith commented 8 years ago

I've switched all the remaining tests except for the raven ones, because there's no easy way to inject an appropriate agent in those. This PR is now complete except for things that come up during review.


Review status: 0 of 43 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

smn commented 8 years ago

Reviewed 2 of 2 files at r1. Review status: 0 of 43 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


vumi/tests/fake_connection.py, line 208 [r1] (raw file): Having these things patched & called without some kind of warning/logging seems like something that's going to be biting us later.


Comments from the review on Reviewable.io

jerith commented 8 years ago

Review status: 0 of 43 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


vumi/tests/fake_connection.py, line 211 [r1] (raw file): The patching is okay here because we're specifically operating on instances of _LoopbackTransport the we get from the loopbackAsync() machinery (unless something else has interfered with the connection setup somehow, in which case all bets are off anyway). We only patch the transport if the methods we're patching onto it don't exist, which protects us from future changes to the transport implementation.

The only feasible alternative is completely reimplementing most of twisted.protocols.loopback, which I don't particularly want to do in vumi.


Comments from the review on Reviewable.io

smn commented 8 years ago

Reviewed 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, 4 of 4 files at r6, 2 of 2 files at r7. Review status: 13 of 43 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


setup.py, line 39 [r8] (raw file): What changed in 13.2.0 vs 13.1.0 that needs this?


Comments from the review on Reviewable.io

smn commented 8 years ago

Reviewed 2 of 2 files at r8, 6 of 6 files at r9, 4 of 4 files at r10, 3 of 3 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, 2 of 2 files at r14, 2 of 2 files at r15, 2 of 2 files at r16, 2 of 2 files at r17, 2 of 2 files at r18, 1 of 1 files at r19. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

jerith commented 8 years ago

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


setup.py, line 39 [r8] (raw file): IAgent was introduced there.


Comments from the review on Reviewable.io

smn commented 8 years ago

Reviewed 2 of 2 files at r20. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

smn commented 8 years ago

:+1:


Comments from the review on Reviewable.io