[GitHub] [activemq-nms-amqp] Havret opened a new pull request #9: [PoC] Testing toolkit

[GitBox <gi...@apache.org>]

Havret opened a new pull request #9: [PoC] Testing toolkit
URL: https://github.com/apache/activemq-nms-amqp/pull/9
 
 
   Work in progress. Do not merge.
   
   During the work on transactions support https://github.com/apache/activemq-nms-amqp/pull/8, I realized that the current approach to integration testing is not comprehensive enough, and we will need something more sophisticated to cover all required scenarios. 
   
   As you may know, for most of our integration tests, ContainerHost component from amqpnetlite library is being used. As I started my rework during failover implementation, I needed some solution to write integration tests, which wouldn't require spinning up an actual broker. ContainerHost seemed to be a feasible solution but as the work progressed more and more its shortcomings and limitations turned up. Certain tests were really hard to write, other impossible to write. For example to test durable message consumer closing scenario I had to either pull TestListener class from amqpnetlite and use it instead of ContainerHost for this particular scenario, or request some changes in ContainerHost (it turned out that I did both).
   
   As we are aiming to make this project .NET equivalent of QPID JMS, I think we should follow its approach to integration testing. This PR is a PoC of testing toolkit inspired by QPID JMS TestAmqpPeer. It enables us to write integration tests with frame by frame assertions. 
   
   The example test may look like this:
   
   ```
   [Test, Timeout(20_000)]
   public void TestRemotelyCloseConnectionDuringSessionCreation()
   {
       string errorMessage = "buba";
   
       using (TestAmqpPeer2 testPeer = new TestAmqpPeer2())
       {
           testPeer.Open();
           IConnection connection = EstablishConnection(testPeer);
   
           // Expect the begin, then explicitly close the connection with an error
           testPeer.ExpectBegin(sendResponse: false);
           testPeer.RemotelyCloseConnection(expectCloseResponse: true, errorCondition: AmqpError.NOT_ALLOWED, errorMessage: errorMessage);
   
           try
           {
               connection.CreateSession();
               Assert.Fail("Expected exception to be thrown");
           }
           catch (NMSException e)
           {
               Assert.True(e.Message.Contains(errorMessage));
           }
   
           connection.Close();
       }
   }
   ```
   
   
   Rewriting just a couple of our tests using this approach helped me to tackle two issues with the current implementation:
   1) During connection close, we are explicitly closing sessions before we actually close the underlying amqp connection (QPID just closes the connection without bothering with sessions).
   https://github.com/apache/activemq-nms-amqp/blob/2d565c7dcf41135f77bb5225d05d2e0d21c60696/src/NMS.AMQP/Provider/Amqp/AmqpConnection.cs#L135-L154
   2) We do not handle properly the scenario when the connection is remotely closed. When somebody tries to use NMS connection which was remotely closed, we are throwing "The Connection is closed" exception instead of the actual underlying cause of loss of connection. 
   
   These are just small issues, but I am sure that writing our tests exactly the same way as they are written in the reference solution will help us improve the overall quality of the library.
   
   What do you think about it? @michaelandrepearce @RagnarPaulson @cjwmorgan-sol

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services