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:
us...@infra.apache.org
With regards,
Apache Git Services
