Unit test refactoring #41

jtaubensee · 2016-12-15T23:13:16Z

Building off of Sean's previous PR #38:

Combining unit test classes, using XUnit MemberDataAttribute to provide test permutations.
Adding DisplayTestMethodNameAttribute to do default logging for each unit test.
Removed ITestOutputHelper since tests are all run in a single thread
Also setting constants, so there is only the need for a single connection string. We will provide an ARM template to automatically provision resources.

… TopicClientTestBase to a single file use async API only use xunit to parameterize test cases inputs

* Combining unit test classes, and using XUnit MemberDataAttribute to provide test permutations. * Adding DisplayTestMethodNameAttribute to do default logging for each unit test. * Removed ITestOutputHelper since tests are all run in a single thread

msftclas · 2016-12-15T23:13:19Z

Hi @jtaubensee, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (John Taubensee). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

jtaubensee · 2016-12-15T23:30:22Z

Not all tests are passing, so this is a work in progress.

SeanFeldman · 2016-12-16T00:17:12Z

@jtaubensee if it's still WIP, could you please change the title to [WIP] Unit test refactoring?
To get it even better, you could install this addon

jtaubensee · 2016-12-16T00:21:44Z

@SeanFeldman - Now you're just getting picky..... haha I've got the label on the item.

SeanFeldman · 2016-12-16T00:22:50Z

Labels come and go, titles stay man.

SeanFeldman

Like it! A few little items in the comments.

SeanFeldman · 2016-12-16T00:19:37Z

templates/azuredeploy.json

+		}
+	},
+	"variables": {
+		"partitionedQueueName": "partitionedqueue",


Since entities support dashes, what do you think about giving entity names some love for readability?
partitionedqueue =>partitioned-queue
nonpartitionedsessionqueue =>non-partitioned-session-queue #Resolved

I like it #Resolved

Wait, it's not the end of "Sean's rules" list 😛 #Resolved

SeanFeldman · 2016-12-16T00:33:53Z

test/Microsoft.Azure.ServiceBus.UnitTests/project.json

@@ -18,7 +18,8 @@
    },
    "Microsoft.NETCore.Platforms": "1.1.0",
    "xunit": "2.2.0-beta4-build3444",
-    "dotnet-test-xunit": "2.2.0-preview2-build1029"
+    "dotnet-test-xunit": "2.2.0-preview2-build1029",
+    "Microsoft.Extensions.PlatformAbstractions": "1.1.0"


What's this one is used for? #Resolved

Its used in the logger for DisplayTestMethodNameAttribute to log what platform the tests are running on. i.e. .NET Core #Resolved

SeanFeldman · 2016-12-16T00:35:45Z

test/Microsoft.Azure.ServiceBus.UnitTests/BrokeredMessageTests.cs

+            var deferredMessage = await queueClient.ReceiveBySequenceNumberAsync(deferredSequenceNumber);
+            await deferredMessage.CompleteAsync();
+
+            queueClient.Close();


SeanFeldman · 2016-12-16T00:36:31Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueClientTests.cs

+            }
+            finally
+            {
+                queueClient.Close();


Async? there are a few of these. #Resolved

SeanFeldman · 2016-12-16T00:37:07Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

        {
-            QueueClient queueClient = QueueClient.CreateFromConnectionString(this.ConnectionString);
+            var queueClient = QueueClient.CreateFromConnectionString(TestUtility.GetEntityConnectionString(queueName));


❤️ yes! #Resolved

SeanFeldman · 2016-12-18T06:42:01Z

@jtaubensee can I also suggest to add PowerShell steps to the readme.md file to create namespace with entities?

# log in
Login-AzureRmAccount

# see your subscriptions
Get-AzureSubscription

# select subscription to use
Set-AzureRmContext -SubscriptionName "<subscription-name>"

# create a new resource group (if needed)
New-AzureRmResourceGroup -Name "ASB-client-tests-RG" -Location "West US"

# verify deployment script
Test-AzureRmResourceGroupDeployment -ResourceGroupName "ASB-client-tests-RG" -TemplateFile <path-to>\templates\azuredeploy.json

# deploy
New-AzureRmResourceGroupDeployment -Name "ASB-client-tests-Deployment" -ResourceGroupName "ASB-client-tests-RG" -TemplateFile <path-to>\templates\azuredeploy.json

SeanFeldman · 2016-12-18T19:15:37Z

test/Microsoft.Azure.ServiceBus.UnitTests/BrokeredMessageTests.cs

    using Xunit;

    public class BrokeredMessageTests
    {
+        [Fact]
+        async Task BrokeredMessageOperationsTest()


This test is brutal 😞 Just the name BrokeredMessageOperationsTest indicates that there are multiple tests hidden within a single test.

I suggest to break this apart into multiple ones.

SeanFeldman · 2016-12-19T02:45:20Z

test/Microsoft.Azure.ServiceBus.UnitTests/BrokeredMessageTests.cs

+            // Send a Message, Receive/ Abandon and Complete it using BrokeredMessage methods
+            queueClient = QueueClient.CreateFromConnectionString(
+                TestUtility.GetEntityConnectionString(Constants.PartitionedQueueName),
+                ReceiveMode.ReceiveAndDelete);


How did this become ReceiveAndDelete? The comments says PeekLock. Test is failing because of of the incorrect mode used.

Again, it's a test on its own 😉 Personally, I'd call it When_receiving_in_PeekLock_mode.Should_be_able_to_abandon.

SeanFeldman · 2016-12-19T02:59:56Z

test/Microsoft.Azure.ServiceBus.UnitTests/BrokeredMessageTests.cs

+                TestUtility.GetEntityConnectionString(Constants.PartitionedQueueName),
+                ReceiveMode.ReceiveAndDelete);
+
+            await TestUtility.SendMessagesAsync(queueClient.InnerSender, 1);


Question: why do we use the internal queueClient.InnerSender of the queueClient this is a sign of something that is not right. Testing a unit (queue client) should go through the unit, not its internals.

I agree, we should test using the exposed classes. But this was to make the testing a little more easier and possibly re-use the code rather than have to rewrite the same code for queues/topic subscriptions..Since we'r still debating whether its a good idea to open the MessageSender/Receiver or keep a smaller surface area and making it as specific as possible until and unless for writing tests like here etc. Inputs welcome on that. But until then we thought we'll keep it this way early on. As we gather more feedback on opening the Sender/Receiver, we can change the tests too to reflect the same behavior.

SeanFeldman · 2016-12-19T03:00:21Z

test/Microsoft.Azure.ServiceBus.UnitTests/BrokeredMessageTests.cs

+                TestUtility.GetEntityConnectionString(Constants.PartitionedQueueName),
+                ReceiveMode.ReceiveAndDelete);
+
+            await TestUtility.SendMessagesAsync(queueClient.InnerSender, 1);


same question - why through internal?

SeanFeldman · 2016-12-19T03:24:23Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

+                Assert.True(message.MessageId == messageId);
+
+                TestUtility.Log("Sleeping 10 seconds...");
+                Thread.Sleep(TimeSpan.FromSeconds(10));


SeanFeldman · 2016-12-19T03:24:34Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

+                Assert.True(firstLockedUntilUtcTime >= initialSessionLockedUntilTime + TimeSpan.FromSeconds(10));
+
+                TestUtility.Log("Sleeping 5 seconds...");
+                Thread.Sleep(TimeSpan.FromSeconds(5));


Will repeat it #Resolved

SeanFeldman · 2016-12-19T04:05:53Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

+    {
+        public static IEnumerable<object> TestPermutations => new object[]
+        {
+            new object[] { Constants.PartitionedQueueName },


Using wrong queue name. Should be Constants.SessionPartitionedQueueName. #Resolved

SeanFeldman · 2016-12-19T04:06:13Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

+        public static IEnumerable<object> TestPermutations => new object[]
+        {
+            new object[] { Constants.PartitionedQueueName },
+            new object[] { Constants.NonPartitionedQueueName },


Using wrong queue name. Should be Constants.SessionNonPartitionedQueueName. #Resolved

The exception thrown in AmqpLinkCreator is not propagated properly. Original exception contains information about non-sessioned queue used for sessions. But exception thrown didn't contain that info.

SeanFeldman · 2016-12-19T04:14:49Z

test/Microsoft.Azure.ServiceBus.UnitTests/QueueSessionTests.cs

+                returnedSessionState = await sessionReceiver.GetStateAsync();
+                using (StreamReader reader = new StreamReader(returnedSessionState, Encoding.UTF8))
+                {
+                    string returnedSessionStateString = reader.ReadToEnd();


SeanFeldman · 2016-12-19T04:16:12Z

test/Microsoft.Azure.ServiceBus.UnitTests/TopicSessionTests.cs

+                var returnedSessionState = await sessionReceiver.GetStateAsync();
+                using (StreamReader reader = new StreamReader(returnedSessionState, Encoding.UTF8))
+                {
+                    string returnedSessionStateString = reader.ReadToEnd();


SeanFeldman · 2016-12-19T04:16:25Z

test/Microsoft.Azure.ServiceBus.UnitTests/TopicSessionTests.cs

+                returnedSessionState = await sessionReceiver.GetStateAsync();
+                using (StreamReader reader = new StreamReader(returnedSessionState, Encoding.UTF8))
+                {
+                    string returnedSessionStateString = reader.ReadToEnd();


SeanFeldman · 2016-12-19T04:32:43Z

test/Microsoft.Azure.ServiceBus.UnitTests/SenderReceiverClientTestBase.cs

-            receivedMessages = await TestUtility.ReceiveMessagesAsync(messageReceiver, messageCount - deferMessagesCount, this.Output);
-            await TestUtility.CompleteMessagesAsync(messageReceiver, receivedMessages, this.Output);
+            receivedMessages = await TestUtility.ReceiveMessagesAsync(messageReceiver, messageCount - deferMessagesCount);
+            await TestUtility.CompleteMessagesAsync(messageReceiver, receivedMessages);
            Assert.True(receivedMessages.Count() == messageCount - deferMessagesCount);

            // Receive / Abandon deferred messages
            receivedMessages = await messageReceiver.ReceiveBySequenceNumberAsync(sequenceNumbers);
            Assert.True(receivedMessages.Count() == 5);


Evil. When fails, all it says "expected True, but was False". Good luck with that :)

SeanFeldman

@jtaubensee added comments to point where tests are failing. Once those are addressed (and hopefully tests are more focused), PR should be good.

Some of the tests are failing due to #17 - no proper cleanup.

…o peeklock, and moving all brokered messages tests to a single class.

jtaubensee · 2016-12-19T18:58:05Z

This also addresses #43.

jtaubensee · 2016-12-19T19:31:36Z

@SeanFeldman - I agree with you about the test names, but I dont want to have two different conventions in the same repo. I know it could be a lot of work, but I'd prefer to have a PR where all of the test names are changed to the format you suggest, rather than bit by bit.

vinaysurya · 2016-12-19T21:24:06Z

test/Microsoft.Azure.ServiceBus.UnitTests/TopicClientTests.cs

+                await this.PeekLockWithDeadLetterTestCase(topicClient.InnerSender, subscriptionClient.InnerReceiver, deadLetterSubscriptionClient.InnerReceiver, messageCount);
+            }
+            finally
+            {


Close the subscriptionClient here as well

Thanks @vinaysurya - Hopefully that will help with the tests running multiple times!

vinaysurya · 2016-12-19T21:56:21Z

test/Microsoft.Azure.ServiceBus.UnitTests/TopicSessionTests.cs

+            Assert.True(message.MessageId == messageId);
+            TestUtility.Log($"Received Message: {message.MessageId} from Session: {sessionReceiver.SessionId}");
+            await message.CompleteAsync();
+            TestUtility.Log($"Completed Message: {message.MessageId} for Session: {sessionReceiver.SessionId}");


Add a last step to close the session receiver here as well

vinaysurya · 2016-12-19T22:16:28Z

SeanFeldman · 2016-12-19T22:32:16Z

@jtaubensee sadly, the new test names do not reflect the meaning.

...I'd prefer to have a PR where all of the test names are changed to the format you suggest, rather than bit by bit.

Then perhaps don't wipe the names and leave them as comments/TODOs? No offence, but the new names are cryptic and you'll be adding work to decypher them later. Which might become a hassle so no one will do it.

SeanFeldman and others added 2 commits December 15, 2016 01:29

reduce NonPartitionedTopicClientTests + PartitionedTopicClientTests +…

d38f3ba

… TopicClientTestBase to a single file use async API only use xunit to parameterize test cases inputs

msftclas added the cla-not-required label Dec 15, 2016

jtaubensee added the in progress label Dec 15, 2016

jtaubensee mentioned this pull request Dec 15, 2016

Simplify permutation based tests #38

Closed

Adding intial ARM template

48bf680

SeanFeldman suggested changes Dec 16, 2016

View reviewed changes

Changing to CloseAsync

46fa969

jtaubensee changed the title ~~Unit test refactoring~~ [WIP] Unit test refactoring Dec 16, 2016

SeanFeldman approved these changes Dec 16, 2016

View reviewed changes

making the names readable

9c48f05

SeanFeldman mentioned this pull request Dec 18, 2016

Prefix environment variables with project specific identifier #43

Closed

SeanFeldman reviewed Dec 18, 2016

View reviewed changes

SeanFeldman reviewed Dec 19, 2016

View reviewed changes

SeanFeldman suggested changes Dec 19, 2016

View reviewed changes

jtaubensee added 3 commits December 19, 2016 10:19

Fixing constants for session tests, updating borkered message tests t…

fb196a9

…o peeklock, and moving all brokered messages tests to a single class.

Thread.Sleep() -> await Task.Delay()

adc00bf

StreamReadAsync

156b7d1

jtaubensee requested a review from vinaysurya December 19, 2016 18:00

jtaubensee added unit test and removed unit test labels Dec 19, 2016

jtaubensee added 3 commits December 19, 2016 12:56

Refactored TestUtility to only load environment variable once.

6fb5f38

Couple of misc. var changes

995c864

Changing Environment Variable name

2403983

vinaysurya reviewed Dec 19, 2016

View reviewed changes

oops... Missed closing a SubscriptionClient.

4f17d7b

jtaubensee changed the title ~~[WIP] Unit test refactoring~~ Unit test refactoring Dec 19, 2016

vinaysurya reviewed Dec 19, 2016

View reviewed changes

vinaysurya approved these changes Dec 19, 2016

View reviewed changes

jtaubensee merged commit 9030d47 into Azure:master Dec 19, 2016

jtaubensee deleted the unit-tests branch December 19, 2016 22:40

jtaubensee modified the milestone: 0.0.1-preview Dec 20, 2016

Unit test refactoring #41

Unit test refactoring #41

Conversation

jtaubensee commented Dec 15, 2016

msftclas commented Dec 15, 2016

jtaubensee commented Dec 15, 2016

SeanFeldman commented Dec 16, 2016

jtaubensee commented Dec 16, 2016

SeanFeldman commented Dec 16, 2016

SeanFeldman left a comment

Choose a reason for hiding this comment

SeanFeldman Dec 16, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

jtaubensee Dec 16, 2016 • edited

Choose a reason for hiding this comment

SeanFeldman Dec 16, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman Dec 16, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

jtaubensee Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman Dec 16, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman Dec 16, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman commented Dec 18, 2016 • edited

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman Dec 19, 2016 • edited by jtaubensee

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman left a comment

Choose a reason for hiding this comment

jtaubensee commented Dec 19, 2016

jtaubensee commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinaysurya commented Dec 19, 2016

SeanFeldman commented Dec 19, 2016

SeanFeldman Dec 16, 2016 •

edited by jtaubensee

jtaubensee Dec 16, 2016 •

edited

SeanFeldman Dec 16, 2016 •

edited by jtaubensee

SeanFeldman Dec 16, 2016 •

edited by jtaubensee

jtaubensee Dec 16, 2016 •

edited

SeanFeldman Dec 16, 2016 •

edited by jtaubensee

SeanFeldman Dec 16, 2016 •

edited by jtaubensee

SeanFeldman commented Dec 18, 2016 •

edited

SeanFeldman Dec 19, 2016 •

edited

SeanFeldman Dec 19, 2016 •

edited by jtaubensee

SeanFeldman Dec 19, 2016 •

edited by jtaubensee

SeanFeldman Dec 19, 2016 •

edited by jtaubensee

SeanFeldman Dec 19, 2016 •

edited by jtaubensee

SeanFeldman Dec 19, 2016 •

edited by jtaubensee