Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET smoke Test Sample #6652

Merged
merged 19 commits into from
Jul 12, 2019
Merged

.NET smoke Test Sample #6652

merged 19 commits into from
Jul 12, 2019

Conversation

JonathanCrd
Copy link
Member

Performs functionalities with Key Vault, Identity, Event Hubs and Blob Storage Track 2 SDKs.

Performs functionalities with Key Vault, Identity, Event Hubs and Blob Storage Track 2 SDKs.
Copy link
Member

@danieljurek danieljurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great first cut! This review covers stylistic and a few things dealing with the general logic of the app without diving too far into program design (except the use of static). To that end we'll probably have another review where, after we get the style discussions out of the way, we can dig into the exciting details of Object Oriented design.

samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/BlobStorage.cs Outdated Show resolved Hide resolved
samples/SmokeTest/SmokeTest/Program.cs Outdated Show resolved Hide resolved
The names were changed to match the name convention for environment variables.
A gitignore file was created in the SmokeTest folder, and now all launchSettings.json files are being ignored.
Work based on the reviews from the PR #6652. The public methods are now in PascalCase, the methods does not longer return English strings, instead they return booleans or Exceptions.  And Exit Codes handling was implemented.
Class names were changed to match the following pattern: <service>Test. Example: BlobStorage -> BlobStorageTest.
XML comments were added and some unnecesary comments were deleted.
Use of higher order functions to dry up the code.
Drying up the code by using high order functions.
Tests done with Mirosoft.Azure.DocumentDB SDK.
@weshaggard
Copy link
Member

@JonathanCrd I'd suggest having a look at #6678 which is doing something very similar to what you are doing and so the pattern might be interesting here as well.

@pakrym
Copy link
Contributor

pakrym commented Jul 2, 2019

Is the intent here to have a set of samples or to have a set of tests?

It looks like it's the later, any reason not to use unit testing framework then? What's the benefit over the live tests?

@weshaggard
Copy link
Member

Is the intent here to have a set of samples or to have a set of tests?

The intent is actually to run a set of samples for each of the services in a single application. For now we are just writing samples independently but the goal is to eventually pull in all the samples from the library directory into this single application.

@pakrym
Copy link
Contributor

pakrym commented Jul 2, 2019

The way it's written right now looks like an attempt to reinvent testing framework.

@weshaggard
Copy link
Member

The way it's written right now looks like an attempt to reinvent testing framework.

I agree and we should definitely avoid duplicating a testing framework. I kind of like what @jsquire did in #6678 which is why I pointed to that as an example.

@pakrym
Copy link
Contributor

pakrym commented Jul 2, 2019

I wonder if we can build on that idea but avoid ISample all together. Like, have every file be a self-contained copy-pastable app with Program.Main but also have a common runner that is able to run multiple samples.

@jsquire
Copy link
Member

jsquire commented Jul 2, 2019

I wonder if we can build on that idea but avoid ISample all together. Like, have every file be a self-contained copy-pastable app with Program.Main but also have a common runner that is able to run multiple samples.

I used an interface just to enforce structure; in the context of Event Hubs, I wanted to ensure that every sample had a consistent method to execute which accepted a common set of connection parameters (to avoid ambient context and the need to set environment variables; I wanted to enable the F5 experience in Visual Studio.) I could have made implicit assumptions about the structure to avoid the interface, but preferred to make it explicit in case there were community contributions.

With respect to a generic approach that would span client libraries, I'd expect that to be tough without ending up with the need for ambient environment requirements again. In my opinion, having an approachable "it just works" experience for being able to launch the debugger is the more desirable side of that trade off.

@weshaggard
Copy link
Member

I agree it is something to build on and I'm fine making them standalone but we still need a way to aggregate them together.

@pakrym
Copy link
Contributor

pakrym commented Jul 2, 2019

With respect to a generic approach that would span client libraries, I'd expect that to be tough without ending up with the need for ambient environment requirements again. In my opinion, having an approachable "it just works" experience for being able to launch the debugger is the more desirable side of that trade off.

I wonder if we can use System.CommandLine (https://github.com/dotnet/command-line-api) inside sample apps themselves making them explicit and directly callable but also easy to aggregate.

The samples not longer need the "TestBase" class to run.
This text file is not going to be needed any more since the test Blob is being created within the code.
All classes are now static.
@JonathanCrd JonathanCrd merged commit 0355bd1 into Azure:master Jul 12, 2019
rileymckenna pushed a commit to rileymckenna/azure-sdk-for-net that referenced this pull request Jul 16, 2019
* .NET smoke Test Sample

Performs functionalities with Key Vault, Identity, Event Hubs and Blob Storage Track 2 SDKs.

* Env Variables names changed

The names were changed to match the name convention for environment variables.

* Update .gitignore

* gitignore updated

A gitignore file was created in the SmokeTest folder, and now all launchSettings.json files are being ignored.

* Exit Codes, PascalCase and refactoring of the static methods.

Work based on the reviews from the PR Azure#6652. The public methods are now in PascalCase, the methods does not longer return English strings, instead they return booleans or Exceptions.  And Exit Codes handling was implemented.

* Class names changed

Class names were changed to match the following pattern: <service>Test. Example: BlobStorage -> BlobStorageTest.

* Comments

XML comments were added and some unnecesary comments were deleted.

* Higher order functions

Use of higher order functions to dry up the code.

* Test classes refactured

Drying up the code by using high order functions.

* Create CosmosDBTest.cs

* Cosmos DB implementation

Tests done with Mirosoft.Azure.DocumentDB SDK.

* Update CosmosDBTest.cs

* Update CosmosDBTest.cs

* Unnecessary blank lines removed

* ExecuteTest typo

* Base class deleted

The samples not longer need the "TestBase" class to run.

* BlobTestSource.tst deleted

This text file is not going to be needed any more since the test Blob is being created within the code.

* Static Classes

All classes are now static.

* Pair programming comments
JonathanCrd added a commit that referenced this pull request Aug 8, 2019
* .NET smoke Test Sample

Performs functionalities with Key Vault, Identity, Event Hubs and Blob Storage Track 2 SDKs.

* Env Variables names changed

The names were changed to match the name convention for environment variables.

* Update .gitignore

* gitignore updated

A gitignore file was created in the SmokeTest folder, and now all launchSettings.json files are being ignored.

* Exit Codes, PascalCase and refactoring of the static methods.

Work based on the reviews from the PR #6652. The public methods are now in PascalCase, the methods does not longer return English strings, instead they return booleans or Exceptions.  And Exit Codes handling was implemented.

* Class names changed

Class names were changed to match the following pattern: <service>Test. Example: BlobStorage -> BlobStorageTest.

* Comments

XML comments were added and some unnecesary comments were deleted.

* Higher order functions

Use of higher order functions to dry up the code.

* Test classes refactured

Drying up the code by using high order functions.

* Create CosmosDBTest.cs

* Cosmos DB implementation

Tests done with Mirosoft.Azure.DocumentDB SDK.

* Update CosmosDBTest.cs

* Update CosmosDBTest.cs

* Unnecessary blank lines removed

* ExecuteTest typo

* Base class deleted

The samples not longer need the "TestBase" class to run.

* BlobTestSource.tst deleted

This text file is not going to be needed any more since the test Blob is being created within the code.

* Static Classes

All classes are now static.

* Pair programming comments

* Concurrency support

* License headers added

* License header added

* Adapted for concurrency

* Use of string interpolation

* Use of string interpolation

* Update EventHubsTest.cs
pull bot pushed a commit to test-repo-billy/azure-sdk-for-net that referenced this pull request Aug 8, 2019
* .NET smoke Test Sample

Performs functionalities with Key Vault, Identity, Event Hubs and Blob Storage Track 2 SDKs.

* Env Variables names changed

The names were changed to match the name convention for environment variables.

* Update .gitignore

* gitignore updated

A gitignore file was created in the SmokeTest folder, and now all launchSettings.json files are being ignored.

* Exit Codes, PascalCase and refactoring of the static methods.

Work based on the reviews from the PR Azure#6652. The public methods are now in PascalCase, the methods does not longer return English strings, instead they return booleans or Exceptions.  And Exit Codes handling was implemented.

* Class names changed

Class names were changed to match the following pattern: <service>Test. Example: BlobStorage -> BlobStorageTest.

* Comments

XML comments were added and some unnecesary comments were deleted.

* Higher order functions

Use of higher order functions to dry up the code.

* Test classes refactured

Drying up the code by using high order functions.

* Create CosmosDBTest.cs

* Cosmos DB implementation

Tests done with Mirosoft.Azure.DocumentDB SDK.

* Update CosmosDBTest.cs

* Update CosmosDBTest.cs

* Unnecessary blank lines removed

* ExecuteTest typo

* Base class deleted

The samples not longer need the "TestBase" class to run.

* BlobTestSource.tst deleted

This text file is not going to be needed any more since the test Blob is being created within the code.

* Static Classes

All classes are now static.

* Pair programming comments

* Concurrency support

* License headers added

* License header added

* Adapted for concurrency

* Use of string interpolation

* Use of string interpolation

* Update EventHubsTest.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants