[Synapse] Rework and extend unit tests #18010

chamons · 2021-01-15T18:48:10Z

This is the first in a set of PRs intended to complete a basic level of test coverage on Synapse. This one in particular is more extension, and less additive, as it refactors and restructures the existing tests.

Some notes:

I've removed the shared testing base class, and moved some test helpers into static methods that declare the needed resources instead of digging into the base class's public variables. I also explicitly instance the test class in each test.
- I prefer less magic and more explicit behavior as a rule. I find this easier to maintain.
- I also updated test and sample namespaces to match the guidance from the template Foo.Bar.Tests or Foo.Bar.Samples
A few new tests are included, just to prevent code coverage regressions. Some of them are [Ignored] due to current limitations in the test-resources.json instance, or issues with the service/SDK that I'm looking at.
I am aware that some of the tests I fixed up in the refactor don't assert much, or could be improved further. As I noted above, additional test work will continue after this.
I added a bunch of "stub" test classes just to fill out the set of test files, and to make it explicit what isn't being tested yet.
This PR is best gone by commit by commit. The "Test recordings" PR can be skipped.

jsquire

LGTM, though you may want to get another pair of eyes more familiar with the recorded test infrastructure to take a peek.

jsquire · 2021-01-20T14:19:19Z

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/BigDataPoolsClientLiveTests.cs

+            ));
+        }
+
+        [Ignore("This test case cannot be automated due to the inability to configure infrastructure to test against.")]


There's a test category "Manually" that seems to be the recommended approach for these types. Tests with that category are not run automatically by the pipelines.

I believe most of these could be automated, I don't want to give up yet. Filing an issue and using it to track.

jsquire · 2021-01-20T14:20:30Z

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/DataFlowClientLiveTests.cs


-            DataFlowCreateOrUpdateDataFlowOperation createOperation = await DataFlowClient.StartCreateOrUpdateDataFlowAsync(dataFlowName, new DataFlowResource(new DataFlow()));
-            await createOperation.WaitForCompletionAsync();
+            // Non-disposable as we'll be deleting it ourselves


I'm not sure what this comments means. I think you're saying "This resource is disposable but we're not going to dispose it so that we can control deletion." - am I close?

It was "we're getting the raw resource and not using the Disposable for a reason here. If that wasn't clear, maybe I should just nuke the comment.

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/DisposableDataFlow.cs

jsquire · 2021-01-20T14:23:39Z

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/LinkedServiceClientLiveTests.cs

        [Test]
        public async Task TestGetLinkedService()
        {
-            await foreach (var expectedLinkedService in LinkedServiceClient.GetLinkedServicesByWorkspaceAsync())
+            LinkedServiceClient client = CreateClient ();


nit: Extra space before parenthesis.

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/LinkedServiceClientLiveTests.cs

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/SqlScriptClientLiveTests.cs

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/TriggerRunClientLiveTests.cs

jsquire · 2021-01-20T14:40:07Z

sdk/synapse/Azure.Analytics.Synapse.Shared/tests/SynapseTestExtensions.cs

+        /// <typeparam name="T">The item type.</typeparam>
+        /// <param name="items">The sequence of items.</param>
+        /// <returns>A list of all the items in the sequence.</returns>
+        public static async Task<IList<T>> ToListAsync<T>(this IAsyncEnumerable<T> items)


I think there may be something in the test framework that does this....

I found a few copies, but none in a common location:

PS C:\Users\chhamo\Programming\azure-sdk-for-net\sdk> git grep ToListAsync | Select-String Task batch/Microsoft.Azure.Batch.FileStaging/tests/FileStagingIntegrationTests.cs: List<CloudTask> tasksFromService = await jobOperations.ListTasks(jobId).ToListAsync().ConfigureAwait(false); batch/Microsoft.Azure.Batch/src/PagedEnumerableExtensions.cs: public static async Task<List<T>> ToListAsync<T>(this IPagedEnumerable<T> source, CancellationToken cancellationToken = default(CancellationToken)) batch/Microsoft.Azure.Batch/tests/IntegrationTests/AddTaskCollectionIntegrationTests.cs: List<CloudTask> tasksFromService = await jobOperations.ListTasks(jobId).ToListAsync().ConfigureAwait(false); batch/Microsoft.Azure.Batch/tests/UnitTests/PagedCollectionUnitTests.cs: public async Task IfSequenceIsNull_ThenToListAsyncThrowsArgumentNullException() batch/Microsoft.Azure.Batch/tests/UnitTests/PagedCollectionUnitTests.cs: public async Task ToListAsyncCollectsAllElementsOfSequence() batch/Microsoft.Azure.Batch/tests/UnitTests/PagedCollectionUnitTests.cs: public async Task ToListAsyncCorrectlyHandlesEmptyCollections() batch/Microsoft.Azure.Batch/tests/UnitTests/PagedCollectionUnitTests.cs: public async Task ToListAsyncSupportsCancellation() search/Azure.Search.Documents/tests/Utilities/TestExtensions.cs: public static async Task<List<T>> ToListAsync<T>(this AsyncPageable<T> pageable) storage/Azure.Storage.Common/tests/Shared/TestExtensions.cs: public static async Task<IList<T>> ToListAsync<T>(this IAsyncEnumerable<T> items) synapse/Azure.Analytics.Synapse.Shared/tests/SynapseTestExtensions.cs: public static async Task<IList<T>> ToListAsync<T>(this IAsyncEnumerable<T> items)

jsquire · 2021-01-20T14:41:09Z

sdk/synapse/Azure.Analytics.Synapse.Spark/tests/SparkBatchClientLiveTests.cs

                    ValidateSparkBatchJob(expectedSparkJob, actualSparkJob);
                }
                catch (Azure.RequestFailedException)
                {
                }
            }
        }
+
+       internal void ValidateSparkBatchJob(SparkBatchJob expectedSparkJob, SparkBatchJob actualSparkJob)


I keep reading these as SparkleBatchJob

jsquire · 2021-01-20T14:42:30Z

sdk/synapse/Azure.Analytics.Synapse.Spark/tests/SparkTestUtilities.cs

-            string name = test.Recording.GenerateName("dontnetbatch");
-            string file = string.Format("abfss://{0}@{1}.dfs.core.windows.net/samples/java/wordcount/wordcount.jar", test.TestEnvironment.StorageFileSystemName, test.TestEnvironment.StorageAccountName);
+            string name = recording.GenerateName("dontnetbatch");
+            string file = string.Format("abfss://{0}@{1}.dfs.core.windows.net/samples/java/wordcount/wordcount.jar", testEnvironment.StorageFileSystemName, testEnvironment.StorageAccountName);


Does it matter that these are referring to java. Is this URI stable across environments/platforms/clouds?

These tests require the uploaded sample from the team copied onto the shared storage, which can be either java, python, or dotnet.

This code in question was from the client team, and defaulted to java. I'm planning on improving this in a future PR.

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/DataFlowDebugSessionClientLiveTests.cs

sdk/synapse/Azure.Analytics.Synapse.Shared/tests/SynapseTestExtensions.cs

chamons · 2021-01-20T18:04:55Z

Changing the ID generation invalidated all of the recordings. Fixing.

This is the first in a set of PRs intended to complete a basic level of test coverage on Synapse. This one in particular is more extension, and less additive, as it refactors and restructures the existing tests. Some notes: - I've removed the shared testing base class, and moved some test helpers into static methods that declare the needed resources instead of digging into the base class's public variables. I also explicitly instance the test class in each test. - I prefer less magic and more explicit behavior as a rule. I find this easier to maintain. - I also updated test and sample namespaces to match the guidance from the template Foo.Bar.Tests or Foo.Bar.Samples - A few new tests are included, just to prevent code coverage regressions. Some of them are [Ignored] due to current limitations in the test-resources.json instance, or issues with the service/SDK that I'm looking at. - I am aware that some of the tests I fixed up in the refactor don't assert much, or could be improved further. As I noted above, additional test work will continue after this. - I added a bunch of "stub" test classes just to fill out the set of test files, and to make it explicit what isn't being tested yet.

chamons added 6 commits January 15, 2021 12:04

Remove base class for tests, update namespaces

0b5db3b

Test changes required by base class refactor

1c082e0

Tests rewritten with disposible resources to clean up after themselves

062034c

New tests, some of which are [Ignore]'ed or SYNAPSE_API_ISSUE

9ff4021

Stubs for rest of clients that need tests

67b2709

Test recordings

d7c02c4

chamons requested review from aim-for-better and idear1203 as code owners January 15, 2021 18:48

jsquire approved these changes Jan 20, 2021

View reviewed changes

christothes approved these changes Jan 20, 2021

View reviewed changes

sdk/synapse/Azure.Analytics.Synapse.Artifacts/tests/DataFlowDebugSessionClientLiveTests.cs Outdated Show resolved Hide resolved

sdk/synapse/Azure.Analytics.Synapse.Shared/tests/SynapseTestExtensions.cs Outdated Show resolved Hide resolved

chamons added 3 commits January 20, 2021 11:27

Code review changes

3b191ef

Add link to test infra issue

a37df51

Missed one save

bb915ea

Update recordings

d160905

chamons merged commit ecc44d6 into Azure:master Jan 21, 2021

chamons deleted the synapse_test_coverage_smash branch January 21, 2021 15:33

chamons mentioned this pull request Jan 25, 2021

[Synapse] Test Coverage Improvements #18166

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Synapse] Rework and extend unit tests #18010

[Synapse] Rework and extend unit tests #18010

chamons commented Jan 15, 2021 •

edited

Loading

jsquire left a comment

jsquire Jan 20, 2021

chamons Jan 20, 2021

chamons Jan 20, 2021

jsquire Jan 20, 2021

chamons Jan 20, 2021

jsquire Jan 20, 2021

jsquire Jan 20, 2021

chamons Jan 20, 2021

jsquire Jan 20, 2021

jsquire Jan 20, 2021

chamons Jan 20, 2021

chamons commented Jan 20, 2021

[Synapse] Rework and extend unit tests #18010

[Synapse] Rework and extend unit tests #18010

Conversation

chamons commented Jan 15, 2021 • edited Loading

jsquire 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

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

chamons commented Jan 20, 2021

chamons commented Jan 15, 2021 •

edited

Loading