Adding ActivityLogAlerts and ActionGroups API calls, PATCH operations, and the corresponding unit tests by gucalder · Pull Request #3520 · Azure/azure-sdk-for-net

gucalder · 2017-07-27T21:19:46Z

Adding ActivityLogAlerts and ActionGroups API calls, PATCH operations, and the corresponding unit tests

Description

Adding ActivityLogAlerts and ActionGroups API calls.
Adding PATCH operations
Adding unit tests for the patch and the new API calls.

This is a retry of the closed PR #3514 which included changes from vs17dev, which is not used any longer.

This checklist is used to make sure that common guidelines for a pull request are followed.

I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

… and the corresponding unit tests

shahabhijeet · 2017-07-27T22:11:33Z

@azuresdkci test this please

shahabhijeet · 2017-07-27T23:21:55Z

@gucalder can you add link to the swagger PR

shahabhijeet

@gucalder we need basic scenario tests that will test your service.
This is a requirement and we have asked in the past to add these tests.
Please add recorded tests.
Please use latest Resource Manager nuget to record your tests

gucalder · 2017-07-27T23:44:23Z

This is the Swagger PR: Azure/azure-rest-api-specs#1481

… into psSdkJson6 # Conflicts: # src/SDKs/Monitor/Management.Monitor/generate.cmd

shahabhijeet · 2017-08-02T23:53:59Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+	  <!-- PackageReference Update="Microsoft.NET.Test.Sdk" Version="15.0.0" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.8" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure.TestFramework" Version="1.7.2" / -->
+    <PackageReference Include="Newtonsoft.Json" Version="6.0.8" />


@gucalder all SDK tests have dependency on newtonsoft.json via clientruntime. Why are you introducing a downgraded version for your tests? What is the reason for doing this?

That downgraded version comes from AzSdk.reference.props, the item group for net452. It uses 9.0.1 for netstandard1.4. My tests are targeting net452.
Without the explicit reference in the test project all my tests fail in VS with an error that says: Could not load file or assembly 'Newtonsoft.Json, Version=6.0.0.0.... I think this is so since the clientruntime depends on 6.0.8 for net452 (and on 9.0.1 for netstandard1.4.)

@gucalder any reason you don't have tests that supports .NETStandard1.4

shahabhijeet · 2017-08-02T23:54:42Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <Reference Include="System.Net.Http" />


Similarly why do you need a separate System.NET.Http dependency as it is already part of your management client.
Any reason for adding this?

I get several compilation errors for net452 if I do not include it.

shahabhijeet · 2017-08-02T23:55:34Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+  <ItemGroup>
+	  <!-- PackageReference Update="Microsoft.NET.Test.Sdk" Version="15.0.0" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.8" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure.TestFramework" Version="1.7.2" / -->


@gucalder remove all commented code.
Also use latest Resource manager 1.6.0 at least, you are still targeting 1.1.0 resource manager
Record all your tests using latest resource manager.

That version of resource manager comes from AzSdk.test.reference.props, which is including it unconditionally. I have no explicit reference to this package/dll.
I am going to need guidance here:
I can add an explicit reference to the package called:

That does not exclude the old reference to 1.1.0-preview. The build contains two dlls:
Microsoft.Azure.Management.ResourceManager.dll
Microsoft.Azure.ResourceManager.dll

Internally, the dlls are similar, but the namespaces and files are different.

@gucalder if you see the difference between those two nugets, you will see the namespace is different.
So all I was trying to say is to use the correct namespace and types from the 1.6.0 version

shahabhijeet · 2017-08-08T13:15:21Z

@azuresdkci test this please

shahabhijeet · 2017-08-11T22:21:52Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+  <ItemGroup>
+	  <!-- PackageReference Update="Microsoft.NET.Test.Sdk" Version="15.0.0" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.8" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure.TestFramework" Version="1.7.2" / -->


@gucalder if you see the difference between those two nugets, you will see the namespace is different.
So all I was trying to say is to use the correct namespace and types from the 1.6.0 version

shahabhijeet · 2017-08-11T22:26:22Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+	  <!-- PackageReference Update="Microsoft.NET.Test.Sdk" Version="15.0.0" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.8" / >
+	  <PackageReference Update="Microsoft.Rest.ClientRuntime.Azure.TestFramework" Version="1.7.2" / -->
+    <PackageReference Include="Newtonsoft.Json" Version="6.0.8" />


@gucalder any reason you don't have tests that supports .NETStandard1.4

…e Resource Manager dll. Enabling tests for netcoreapp1.1

gucalder · 2017-08-15T21:25:14Z

Making sure I am referencing and using Resource Manager 1.6.0.
Enabling tests for netcoreapp1.1 (TestFramework is incompatible with .Net Standard 1.4)

gucalder · 2017-08-16T00:37:19Z

Netcoreapp1.1 tests failed because the scenario tests cannot find the SessionRecords files.
Locally, the tests cannot be discovered.
So I removed the netcoreapp1.1 target from the test project. I need more time to figure out what is happening there, but I am running out of time with this PR.

shahabhijeet · 2017-08-16T22:16:25Z

src/SDKs/Monitor/Management.Monitor/generate.cmd

+   set specFile1="%2"
+)
+
+if  "%3" == "" (


@gucalder you do not need all this code. Any reason you are updating this file.
Simply executing this file will create a _meta file that is what the new way to generate the code.
All this change has already been done for you, you only need to execute generate.cmd the way it is.
Please revert this change.
Once you execute generate.cmd, it will create a .txt file under _metadata directory like below
https://github.com/Azure/azure-sdk-for-net/tree/psSdkJson6/src/SDKs/_metadata

shahabhijeet · 2017-08-21T05:27:41Z

src/SDKs/Monitor/Monitor.Tests/Monitor.Tests.csproj

+		<VersionPrefix>1.0.0-preview</VersionPrefix>
+	</PropertyGroup>
+	<PropertyGroup>
+		<TargetFrameworks>net452</TargetFrameworks>


@gucalder any reasons you are doing this.
If you are not able to discover tests that is because of VS 15.3 build.
I had sent an email about installing 15.2 and provided link to install 15.2.
Once you install 15.2 you should be good. Please revert all these changes in your test project.
These changes makes it difficult to make sure all SDKs are created and tested consistently with respect to target framework and updating common references.

My version is 15.1 at the moment. I'll try with 15.2.
The tests are discovered and run fine for .net452. That's how I re-recorded them.
It is only netcoreapp1.1 that does not detect any tests locally. However, in your side, during build, the system apparently discovers them and tries to run the tests for netcoreapp1.1 but fails to find the dll, it looks in a folder where, I assume, the dll is not installed.

shahabhijeet · 2017-08-23T03:00:56Z

@azuresdkci test this please

…, and the corresponding unit tests (Azure#3520) * Adding ActivityLogAlerts and ActionGroups API calls, PATH operations, and the corresponding unit tests * Generating code from the 'current' branch of the Swagger specs repo * Adding scenario tests * Improving scenario tests and re-recording them * Making sure the scenario tests point to the more recent version of the Resource Manager dll. Enabling tests for netcoreapp1.1 * Reverting netcoreapp1.1 test target since it fails to find the records. * Returning generate.md to its Azure\psSdkJson6 state * Re-enabling tests for netcoreapp1.1

Adding ActivityLogAlerts and ActionGroups API calls, PATH operations,…

d4a02cd

… and the corresponding unit tests

msftclas added the cla-not-required label Jul 27, 2017

shahabhijeet suggested changes Jul 27, 2017

View reviewed changes

shahabhijeet added the needs-revision label Jul 27, 2017

Generating code from the 'current' branch of the Swagger specs repo

ee4015d

gucalder added 2 commits August 2, 2017 12:00

Adding scenario tests

e5681e5

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

82ca8e6

… into psSdkJson6 # Conflicts: # src/SDKs/Monitor/Management.Monitor/generate.cmd

shahabhijeet suggested changes Aug 2, 2017

View reviewed changes

Improving scenario tests and re-recording them

d94578e

shahabhijeet suggested changes Aug 11, 2017

View reviewed changes

Making sure the scenario tests point to the more recent version of th…

fc7af1e

…e Resource Manager dll. Enabling tests for netcoreapp1.1

Reverting netcoreapp1.1 test target since it fails to find the records.

9db008c

shahabhijeet suggested changes Aug 16, 2017

View reviewed changes

Returning generate.md to its Azure\psSdkJson6 state

45a0144

shahabhijeet suggested changes Aug 21, 2017

View reviewed changes

Re-enabling tests for netcoreapp1.1

6129471

shahabhijeet approved these changes Aug 23, 2017

View reviewed changes

shahabhijeet merged commit 0f85678 into Azure:psSdkJson6 Aug 23, 2017

Conversation

gucalder commented Jul 27, 2017

Description

Uh oh!

shahabhijeet commented Jul 27, 2017

Uh oh!

shahabhijeet commented Jul 27, 2017

Uh oh!

shahabhijeet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gucalder commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gucalder commented Aug 15, 2017

Uh oh!

gucalder commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shahabhijeet left a comment •

edited

Loading