Should I test UDP server code, and if so - why and how?

Question

I don't have much experience doing unit testing. From what I learned, code should be decoupled, and I should not strive to test private code, just public methods, setters, etc etc.

Now, I have grasped some basic testing concepts, but I have troubles applying more advanced stuff to this case... Dependency Injection, Inversion of Control, Mock Objects, etc - can't get my head around it just yet :(

Before I move on to code, here are the questions.

• In a given class, what exactly should I try to test?
• How can I accomplish those test tasks?
• Is there something seriously wrong with class design, which prevents testing from being done properly (or is something just plain wrong even outside of testing context)?
• What design patterns are useful to testing network code in general?

Also, I've been trying to obey "write tests first, then write code to make tests pass", that's why I wrote first two tests that simply instantiate class and run it, but then, when server was able to start and accept packets, I didn't know what to test next...

Okay, here goes the code snippet. (note: original code is split in several namespaces, that's why it may appear a little out of order)

using System;
using System.Collections.Generic;
using System.Text;
using System.Net;
using System.Net.Sockets;
using System.Threading;

namespace MyProject1
{
    /// <summary>
    /// Packet buffer that is sent to/received from connection
    /// </summary>
    public class UDPPacketBuffer
    {
        /// <summary>
        /// Buffer size constant
        /// </summary>
        public const int BUFFER_SIZE = 4096;

        private byte[] _data;

        /// <summary>
        /// Byte array with buffered data
        /// 
        /// DataLength is automatically updated when Data is set
        /// </summary>
        /// <see cref="DataLength"/>
        public byte[] Data { get { return _data; } set { _data = value; DataLength = value.Length; } }

        /// <summary>
        /// Integer with length of buffered data
        /// </summary>
        public int DataLength;

        /// <summary>
        /// Remote end point (IP Address and Port)
        /// </summary>
        public EndPoint RemoteEndPoint;

        /// <summary>
        /// Initializes <see cref="UDPPacketBuffer"/> class
        /// </summary>
        public UDPPacketBuffer()
        {
            // initialize byte array
            this.Data = new byte[BUFFER_SIZE];

            // this will be filled in by the caller (eg. udpSocket.BeginReceiveFrom)
            RemoteEndPoint = (EndPoint)new IPEndPoint(IPAddress.Any, 0);
        }

        /// <summary>
        /// Returns <see cref="Data"/> as a byte array shortened to <see cref="DataLength"/> number of bytes
        /// </summary>
        public byte[] ByteContent
        {
            get
            {
                if (DataLength > 0)
                {
                    byte[] content = new byte[DataLength];
                    for (int i = 0; i < DataLength; i++)
                        content[i] = Data[i];
                    return content;
                }
                else
                {
                    return Data;
                }
            }
        }

        /// <summary>
        /// Returns <see cref="ByteContent"/> converted to string
        /// </summary>
        public string StringContent { get { return Encoding.ASCII.GetString(ByteContent); } }
    }

    /// <summary>
    /// UDP packet-related event arguments passed when invoking events
    /// </summary>
    /// <example>
    /// This example shows how to use UDPPacketEventArgs class when event is invoked.
    /// <code>
    /// if (PacketSent != null)
    ///     PacketSent(this, new UDPPacketEventArgs(buffer, bytesSent));
    /// </code>
    /// </example>
    public class UDPPacketEventArgs : EventArgs
    {
        /// <summary>
        /// Instance of UDPPacketBuffer, holding current event-related buffer
        /// </summary>
        public UDPPacketBuffer buffer { get; private set; }

        /// <summary>
        /// Number of bytes sent to remote end point
        /// </summary>
        public int sent { get; private set; }

        /// <summary>
        /// Initializes <see cref="buffer"/> only. Used when receiving data.
        /// </summary>
        /// <param name="buffer">Buffer sent to or received from remote endpoint</param>
        public UDPPacketEventArgs(UDPPacketBuffer buffer)
        {
            this.buffer = buffer;
        }

        /// <summary>
        /// Initializes <see cref="buffer"/> and <see cref="sent"/> variables. Used when sending data.
        /// </summary>
        /// <param name="buffer">buffer that has been sent</param>
        /// <param name="sent">number of bytes sent</param>
        public UDPPacketEventArgs(UDPPacketBuffer buffer, int sent)
        {
            this.buffer = buffer;
            this.sent = sent;
        }
    }

    /// <summary>
    /// Asynchronous UDP server
    /// </summary>
    public class AsyncUdp : ServerBase
    {
        private const int _defaultPort = 45112;

        private int _udpPort;

        /// <summary>
        /// Port number on which server should listen
        /// </summary>
        public int udpPort { get { return _udpPort; } private set { _udpPort = value; } }

        // should server listen for broadcasts?
        private bool broadcast = false;

        // server socket
        private Socket udpSocket;

        // the ReaderWriterLock is used solely for the purposes of shutdown (Stop()).
        // since there are potentially many "reader" threads in the internal .NET IOCP
        // thread pool, this is a cheaper synchronization primitive than using
        // a Mutex object.  This allows many UDP socket "reads" concurrently - when
        // Stop() is called, it attempts to obtain a writer lock which will then
        // wait until all outstanding operations are completed before shutting down.
        // this avoids the problem of closing the socket with outstanding operations
        // and trying to catch the inevitable ObjectDisposedException.
        private ReaderWriterLock rwLock = new ReaderWriterLock();

        // number of outstanding operations.  This is a reference count
        // which we use to ensure that the threads exit cleanly. Note that
        // we need this because the threads will potentially still need to process
        // data even after the socket is closed.
        private int rwOperationCount = 0;

        // the all important shutdownFlag.  This is synchronized through the ReaderWriterLock.
        private bool shutdownFlag = true;

        /// <summary>
        /// Returns server running state
        /// </summary>
        public bool IsRunning
        {
            get { return !shutdownFlag; }
        }

        /// <summary>
        /// Initializes UDP server with arbitrary default port
        /// </summary>
        public AsyncUdp()
        {
            this.udpPort = _defaultPort;
        }

        /// <summary>
        /// Initializes UDP server with specified port number
        /// </summary>
        /// <param name="port">Port number for server to listen on</param>
        public AsyncUdp(int port)
        {
            this.udpPort = port;
        }

        /// <summary>
        /// Initializes UDP server with specified port number and broadcast capability
        /// </summary>
        /// <param name="port">Port number for server to listen on</param>
        /// <param name="broadcast">Server will have broadcasting enabled if set to true</param>
        public AsyncUdp(int port, bool broadcast)
        {
            this.udpPort = port;
            this.broadcast = broadcast;
        }

        /// <summary>
        /// Raised when packet is received via UDP socket
        /// </summary>
        public event EventHandler PacketReceived;

        /// <summary>
        /// Raised when packet is sent via UDP socket
        /// </summary>
        public event EventHandler PacketSent;

        /// <summary>
        /// Starts UDP server
        /// </summary>
        public override void Start()
        {
            if (! IsRunning)
            {
                // create and bind the socket
                IPEndPoint ipep = new IPEndPoint(IPAddress.Any, udpPort);
                udpSocket = new Socket(
                    AddressFamily.InterNetwork,
                    SocketType.Dgram,
                    ProtocolType.Udp);
                udpSocket.EnableBroadcast = broadcast;
                // we don't want to receive our own broadcasts, if broadcasting is enabled
                if (broadcast)
                    udpSocket.MulticastLoopback = false;
                udpSocket.Bind(ipep);

                // we're not shutting down, we're starting up
                shutdownFlag = false;

                // kick off an async receive.  The Start() method will return, the
                // actual receives will occur asynchronously and will be caught in
                // AsyncEndRecieve().
                // I experimented with posting multiple AsyncBeginReceives() here in an attempt
                // to "queue up" reads, however I found that it negatively impacted performance.
                AsyncBeginReceive();
            }
        }

        /// <summary>
        /// Stops UDP server, if it is running
        /// </summary>
        public override void Stop()
        {
            if (IsRunning)
            {
                // wait indefinitely for a writer lock.  Once this is called, the .NET runtime
                // will deny any more reader locks, in effect blocking all other send/receive
                // threads.  Once we have the lock, we set shutdownFlag to inform the other
                // threads that the socket is closed.
                rwLock.AcquireWriterLock(-1);
                shutdownFlag = true;
                udpSocket.Close();
                rwLock.ReleaseWriterLock();

                // wait for any pending operations to complete on other
                // threads before exiting.
                while (rwOperationCount > 0)
                    Thread.Sleep(1);
            }
        }

        /// <summary>
        /// Dispose handler for UDP server. Stops the server first if it is still running
        /// </summary>
        public override void Dispose()
        {
            if (IsRunning == true)
                this.Stop();
        }

        private void AsyncBeginReceive()
        {
            // this method actually kicks off the async read on the socket.
            // we aquire a reader lock here to ensure that no other thread
            // is trying to set shutdownFlag and close the socket.
            rwLock.AcquireReaderLock(-1);

            if (!shutdownFlag)
            {
                // increment the count of pending operations
                Interlocked.Increment(ref rwOperationCount);

                // allocate a packet buffer
                UDPPacketBuffer buf = new UDPPacketBuffer();

                try
                {
                    // kick off an async read
                    udpSocket.BeginReceiveFrom(
                        buf.Data,
                        0,
                        UDPPacketBuffer.BUFFER_SIZE,
                        SocketFlags.None,
                        ref buf.RemoteEndPoint,
                        new AsyncCallback(AsyncEndReceive),
                        buf);
                }
                catch (SocketException)
                {
                    // an error occurred, therefore the operation is void.  Decrement the reference count.
                    Interlocked.Decrement(ref rwOperationCount);
                }
            }

            // we're done with the socket for now, release the reader lock.
            rwLock.ReleaseReaderLock();
        }

        private void AsyncEndReceive(IAsyncResult iar)
        {
            // Asynchronous receive operations will complete here through the call
            // to AsyncBeginReceive

            // aquire a reader lock
            rwLock.AcquireReaderLock(-1);

            if (!shutdownFlag)
            {
                // start another receive - this keeps the server going!
                AsyncBeginReceive();

                // get the buffer that was created in AsyncBeginReceive
                // this is the received data
                UDPPacketBuffer buffer = (UDPPacketBuffer)iar.AsyncState;

                try
                {
                    // get the length of data actually read from the socket, store it with the
                    // buffer
                    buffer.DataLength = udpSocket.EndReceiveFrom(iar, ref buffer.RemoteEndPoint);

                    // this operation is now complete, decrement the reference count
                    Interlocked.Decrement(ref rwOperationCount);

                    // we're done with the socket, release the reader lock
                    rwLock.ReleaseReaderLock();

                    // run event PacketReceived(), passing the buffer that
                    // has just been filled from the socket read.
                    if (PacketReceived != null)
                        PacketReceived(this, new UDPPacketEventArgs(buffer));
                }
                catch (SocketException)
                {
                    // an error occurred, therefore the operation is void.  Decrement the reference count.
                    Interlocked.Decrement(ref rwOperationCount);

                    // we're done with the socket for now, release the reader lock.
                    rwLock.ReleaseReaderLock();
                }
            }
            else
            {
                // nothing bad happened, but we are done with the operation
                // decrement the reference count and release the reader lock
                Interlocked.Decrement(ref rwOperationCount);
                rwLock.ReleaseReaderLock();
            }
        }

        /// <summary>
        /// Send packet to remote end point speified in <see cref="UDPPacketBuffer"/>
        /// </summary>
        /// <param name="buf">Packet to send</param>
        public void AsyncBeginSend(UDPPacketBuffer buf)
        {
            // by now you should you get the idea - no further explanation necessary

            rwLock.AcquireReaderLock(-1);

            if (!shutdownFlag)
            {
                try
                {
                    Interlocked.Increment(ref rwOperationCount);
                    udpSocket.BeginSendTo(
                        buf.Data,
                        0,
                        buf.DataLength,
                        SocketFlags.None,
                        buf.RemoteEndPoint,
                        new AsyncCallback(AsyncEndSend),
                        buf);
                }
                catch (SocketException)
                {
                    throw new NotImplementedException();
                }
            }

            rwLock.ReleaseReaderLock();
        }

        private void AsyncEndSend(IAsyncResult iar)
        {
            // by now you should you get the idea - no further explanation necessary

            rwLock.AcquireReaderLock(-1);

            if (!shutdownFlag)
            {
                UDPPacketBuffer buffer = (UDPPacketBuffer)iar.AsyncState;

                try
                {
                    int bytesSent = udpSocket.EndSendTo(iar);

                    // note that in invocation of PacketSent event - we are passing the number
                    // of bytes sent in a separate parameter, since we can't use buffer.DataLength which
                    // is the number of bytes to send (or bytes received depending upon whether this
                    // buffer was part of a send or a receive).
                    if (PacketSent != null)
                        PacketSent(this, new UDPPacketEventArgs(buffer, bytesSent));
                }
                catch (SocketException)
                {
                    throw new NotImplementedException();
                }
            }

            Interlocked.Decrement(ref rwOperationCount);
            rwLock.ReleaseReaderLock();
        }
    }

    /// <summary>
    /// Base class used for all network-oriented servers.
    /// <para>Disposable. All methods are abstract, including Dispose().</para>
    /// </summary>
    /// <example>
    /// This example shows how to inherit from ServerBase class.
    /// <code>
    /// public class SyncTcp : ServerBase {...}
    /// </code>
    /// </example>
    abstract public class ServerBase : IDisposable
    {
        /// <summary>
        /// Starts the server.
        /// </summary>
        abstract public void Start();

        /// <summary>
        /// Stops the server.
        /// </summary>
        abstract public void Stop();

        #region IDisposable Members

        /// <summary>
        /// Cleans up after server.
        /// <para>It usually calls Stop() if server is running.</para>
        /// </summary>
        public abstract void Dispose();

        #endregion
    }
}

"Test code" follows.

namespace MyProject1
{
    class AsyncUdpTest
    {
        [Fact]
        public void UdpServerInstance()
        {
            AsyncUdp udp = new AsyncUdp();
            Assert.True(udp is AsyncUdp);
            udp.Dispose();
        }

        [Fact]
        public void StartStopUdpServer()
        {
            using (AsyncUdp udp = new AsyncUdp(5000))
            {
                udp.Start();
                Thread.Sleep(5000);
            }
        }


        string udpReceiveMessageSend = "This is a test message";
        byte[] udpReceiveData = new byte[4096];
        bool udpReceivePacketMatches = false;

        [Fact]
        public void UdpServerReceive()
        {

            using (AsyncUdp udp = new AsyncUdp(5000))
            {
                udp.Start();
                udp.PacketReceived += new EventHandler(delegate(object sender, EventArgs e)
                {
                    UDPPacketEventArgs ea = e as UDPPacketEventArgs;
                    if (this.udpReceiveMessageSend.Equals(ea.buffer.StringContent))
                    {
                        udpReceivePacketMatches = true;
                    }
                });
                // wait 20 ms for a socket to be bound etc
                Thread.Sleep(20);

                UdpClient sock = new UdpClient();
                IPEndPoint iep = new IPEndPoint(IPAddress.Loopback, 5000);

                this.udpReceiveData = Encoding.ASCII.GetBytes(this.udpReceiveMessageSend);
                sock.Send(this.udpReceiveData, this.udpReceiveData.Length, iep);
                sock.Close();

                // wait 20 ms for an event to fire, it should be enough
                Thread.Sleep(20);

                Assert.True(udpReceivePacketMatches);
            }
        }
    }
}

note: code is c#, testing framework xUnit

A big thanks to everyone who takes time to go through my question and answer it!

Answer 1

Should you test ? Absolutely. You need to engineer your code for testability to make this simple. Your first statement is largely correct. So, some further comments:

Unit testing is largely testing code alone against test data and not reliant on external systems/servers etc. Functional/integration testing then brings in your external servers/databases etc. You can use dependency injection to inject either that real external system reference, or a test (mocked) system implementing the same interface, and thus your code becomes easily testable.

So in the above you would probably want to inject the UDP data source into your receiver. Your data source would implement a particular interface, and a mocked (or simple test) source would provide different packets for testing (e.g. empty, containing valid data, containing invalid data). That would form the basis of your unit test.

Your integration (or functional? I never know what to call it) test would perhaps start up a test UDP data source in the same VM, for each test, and pump data via UDP to your receiver. So now you've tested the basic functionality in the face of different packets via your unit test, and you're testing the actual UDP client/server function via your integration test.

So now you've tested your packet transmission/reception, you can test further parts of your code. Your UDP receiver will plug into another component, and here you can use dependency injection to inject the UDP receiver into your upstream component, or a mocked/test receiver implementing the same interface (and so on).

(Note: given that UDP transmission is unreliable even intra-host you should be prepared to cater for that somehow, or accept that infrequently you'll have problems. But that's a UDP-specific issue).

Answer 2

I notices some design problems in your code, and I think that this problems also interfere with ability to test this code.

1. I don't really unserstand UDPPacketBuffer class purpose. This class isn't encapsulate anything. It contains read/write Data property and I noticed only one probably useful is StringContent. If you assume pass through UDP some application level packets, maybe you should create appropriate abstractions for this packets. Also, using UDP you should create something that helps you gather all parts in one (because you can receive parts of your packets in different order). Also, I don't understand why your UDPPacketBuffer contains IPEndPoint. That's why you can't test this class, because there no obvious purpose for this class.

2. It's really hard to test class that sends and receives data over network. But I notices some problems with your AsyncUdp implementation.

2.1 There is no packets delivery guarantee. I mean, who responsible for reliable packet delivery?

2.2 Ther is no thread safe (due to lack of exception safety).

What happens if Start method would be called simultaneously from separate threads?

And, consider following code (From Stop method):

rwLock.AcquireWriterLock(-1);
shutdownFlag = true;
udpSocket.Close();
rwLock.ReleaseWriterLock();

What if updSocket.Close method raise an exception? In this case rwLock whould stay in acquired state.

And in AsyncBeginReceive: what if UDPPacketBuffer ctor throws an exception, or udpSocket.BeginReceiveFrom throws SecurityException or ArgumentOutOfRangeException.

Other functions also not thread safe due to unhandled exception.

In this case you may create some helper class, that can be used in using statemant. Something like this:

class ReadLockHelper : IDisposable
{
  public ReadLockHelper(ReaderWriterLockSlim rwLock)
  {
    rwLock.AcquireReadLock(-1);
    this.rwLock = rwLock;
  }
  public void Dispose()
  {
    rwLock.ReleaseReadLock();
  }
  private ReaderWriterLockSlim rwLock;
}

And than use it in your methods:

using (var l = new ReadLockHelper(rwLock))
{
  //all other stuff
}

And finally. You should use ReaderWriterLockSlim instead ReaderWriterLock.

Important note from MSDN:

The .NET Framework has two reader-writer locks, ReaderWriterLockSlim and ReaderWriterLock. ReaderWriterLockSlim is recommended for all new development. ReaderWriterLockSlim is similar to ReaderWriterLock, but it has simplified rules for recursion and for upgrading and downgrading lock state. ReaderWriterLockSlim avoids many cases of potential deadlock. In addition, the performance of ReaderWriterLockSlim is significantly better than ReaderWriterLock.