Added spock test framework and tests for queue service #4940

sima-zhu · 2019-08-12T17:18:08Z

No description provided.

…into storage_test

sdk/storage/azure-storage-queue/pom.xml

sdk/storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/APISpec.groovy

...e-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAsyncAPITests.groovy

…into storage_test

alzimmermsft · 2019-08-13T18:11:45Z

Make sure to delete out the old test files and playback records

sima-zhu · 2019-08-13T18:33:36Z

Will have another PR for deletion work.

sdk/storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/APISpec.groovy

...storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueAPITests.groovy

rickle-msft · 2019-08-12T21:33:51Z

...storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueAPITests.groovy

+        then:
+        QueueTestHelper.assertResponseStatusCode(createQueueResponse1, 201)
+        def e = thrown(StorageErrorException)
+        QueueTestHelper.assertExceptionStatusCodeAndMessage(e, 409, "QueueAlreadyExists")


This test is mostly exercising the service. Though we do need at least one test per api that returns a failed status code to validate that the Exceptions are successfully translated to a StorageException. This test can be repurposed for that.

Removed. Did not have exception case for create from queue client since all error will be caught from builder.

...storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueAPITests.groovy

rickle-msft · 2019-08-13T18:59:56Z

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

+        primaryQueueServiceClient = queueServiceBuilderHelper(interceptorManager).buildClient()
+    }
+
+    def "Get queue client from queue service client"() {


I think we can remove this test. I'm not really sure what it's covering.

After a second thought, I think this one can be used to test if it can return the right class in case it uses a wrong sync and async class. Also have a high code coverage.

then: queueClient instanceOf QueueClient

Added one more assertion for better code coverage

I haven't found it yet, so correct me if I'm wrong, but there used to be a similar test as this one but it would make a service call to the queue it got to show that getQueueClient/getQueueAsyncClient doesn't create the queue in the service. If that test doesn't exist could we change this test to do that?

I'm ok with having some validation that getQueueClient does not make a service request.

The getClient did nothing but to initialize a new client class.
I don't think the service checking is needed as there is no logic within the service.
I have checked the class type.

rickle-msft · 2019-08-13T19:00:42Z

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

+        QueueTestHelper.assertResponseStatusCode(enqueueMessageResponse, 201)
+    }
+
+    def "Create queues with duplicate name from queue service client"() {


I think we can remove this test

rickle-msft · 2019-08-13T20:07:32Z

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

+        "a_b"          | 400        | StorageErrorCode.INVALID_RESOURCE_NAME
+        "-ab"          | 400        | StorageErrorCode.INVALID_RESOURCE_NAME
+        "a--b"         | 400        | StorageErrorCode.INVALID_RESOURCE_NAME
+        // null | 400 | "InvalidResourceName" TODO: Need to fix the RestProxy before having null parameter


We should probably be asserting not null in the builder, no?

Not for API layer. There is checking inside of builder.
I think it makes sense to check null in API as well.
I have updated the API

I have made changes about checking null for the resource name

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

alzimmermsft

Let's try to remove as much JUnit as we can and make sure we don't have collisions during concurrent live test runs. Overall it is looking good.

sdk/storage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/APISpec.groovy

...orage/azure-storage-queue/src/main/java/com/azure/storage/queue/models/StorageErrorCode.java

alzimmermsft · 2019-08-14T17:00:54Z

sdk/storage/azure-storage-queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

@@ -62,6 +63,7 @@
     * @param queueName Name of the queue
     */
    QueueAsyncClient(AzureQueueStorageImpl client, String queueName) {
+        Objects.requireNonNull(queueName);


This check should be in QueueServiceClient as it allows for a null queue name to be passed, the builders should already be checking for this.

All the entries which asked the queueName will fall into this constructor.
I think it is better to have null checking here and remove the one in buildAsyncClient.

While that is true, having it remain in each individually will offer a closer indication to what went wrong and in the case of the client builder it will early out much sooner.

sdk/storage/azure-storage-queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

alzimmermsft · 2019-08-14T17:03:53Z

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

+        primaryQueueServiceClient = queueServiceBuilderHelper(interceptorManager).buildClient()
+    }
+
+    def "Get queue client from queue service client"() {


I haven't found it yet, so correct me if I'm wrong, but there used to be a similar test as this one but it would make a service call to the queue it got to show that getQueueClient/getQueueAsyncClient doesn't create the queue in the service. If that test doesn't exist could we change this test to do that?

...ge/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueAysncAPITests.groovy

.../azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueServiceAPITests.groovy

...orage/azure-storage-queue/src/test/java/com/azure/storage/queue/spock/QueueTestHelper.groovy

sima-zhu · 2019-08-14T21:24:22Z

Let's try to remove as much JUnit as we can and make sure we don't have collisions during concurrent live test runs. Overall it is looking good.

Valid point on live test collisions.
I am going to send final commit on JUnit deletion to avoid large diff.

alzimmermsft

LGTM

Just make sure to give the tests one more run for validation before merging in.

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

sima-zhu added 3 commits August 9, 2019 16:35

Migrate queue to spock test framework

e4e81fc

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

25a0bbc

…into storage_test

Clean up the dependency

444b658

sima-zhu requested review from alzimmermsft, jaschrep-msft and rickle-msft as code owners August 12, 2019 17:18

alzimmermsft requested changes Aug 12, 2019

View reviewed changes

sima-zhu added 2 commits August 13, 2019 11:10

Address feedback

f4bd47b

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

62bd946

…into storage_test

Using storage error code

7f70869

sima-zhu requested review from danieljurek, mitchdenny and weshaggard as code owners August 13, 2019 18:32

sima-zhu requested a review from alzimmermsft August 13, 2019 18:33

rickle-msft suggested changes Aug 13, 2019

View reviewed changes

Addressed comments

7453724

sima-zhu requested a review from rickle-msft August 14, 2019 00:44

Added queue name null check

1c1679d

alzimmermsft requested changes Aug 14, 2019

View reviewed changes

Remove spock test Junit dependency APIs and minor changes.

0a1c956

sima-zhu requested a review from alzimmermsft August 15, 2019 16:47

Adjust table format

1d33562

alzimmermsft approved these changes Aug 15, 2019

View reviewed changes

sima-zhu added 3 commits August 15, 2019 11:41

Make assertion correct in async api

e759060

Check in the fix of clean up

a5bf306

Use a simpler way of getting class name

cf22ae3

rickle-msft approved these changes Aug 15, 2019

View reviewed changes

Remove all Junit tests and dependencies

4534e8f

sima-zhu merged commit 6eb9e89 into Azure:master Aug 15, 2019

sima-zhu deleted the storage_test branch August 15, 2019 21:10

pull bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-java Aug 15, 2019

Added spock test framework and tests for queue service (#4940)

24be4c9

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

pull bot pushed a commit to test-repo-billy/azure-sdk-for-java that referenced this pull request Aug 15, 2019

Added spock test framework and tests for queue service (Azure#4940)

98341e7

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

pull bot pushed a commit to test-repo-tih/azure-sdk-for-java that referenced this pull request Aug 15, 2019

Added spock test framework and tests for queue service (Azure#4940)

b6b58b1

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added spock test framework and tests for queue service #4940

Added spock test framework and tests for queue service #4940

sima-zhu commented Aug 12, 2019

alzimmermsft commented Aug 13, 2019

sima-zhu commented Aug 13, 2019

rickle-msft Aug 12, 2019

sima-zhu Aug 13, 2019

rickle-msft Aug 13, 2019

sima-zhu Aug 13, 2019

sima-zhu Aug 14, 2019

alzimmermsft Aug 14, 2019

rickle-msft Aug 15, 2019

sima-zhu Aug 15, 2019

rickle-msft Aug 13, 2019

sima-zhu Aug 13, 2019

rickle-msft Aug 13, 2019

sima-zhu Aug 13, 2019

sima-zhu Aug 15, 2019

alzimmermsft left a comment

alzimmermsft Aug 14, 2019

sima-zhu Aug 14, 2019

alzimmermsft Aug 15, 2019

alzimmermsft Aug 14, 2019

sima-zhu commented Aug 14, 2019

alzimmermsft left a comment

Added spock test framework and tests for queue service #4940

Added spock test framework and tests for queue service #4940

Conversation

sima-zhu commented Aug 12, 2019

alzimmermsft commented Aug 13, 2019

sima-zhu commented Aug 13, 2019

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

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

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

Choose a reason for hiding this comment

alzimmermsft left a comment

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

Choose a reason for hiding this comment

sima-zhu commented Aug 14, 2019

alzimmermsft left a comment

Choose a reason for hiding this comment