[Core] Verify test Generation in both autorest.csharp upgrading pipeline of sdk repo and Autorest.CSharp itself

changlong-liu commented 2 years ago

This is not a feature-request, but it's for asking opinion from Azure Core and EngSys team that whether I can start to work on these features :)

API Change Needed

No

Background and motivation

Background

Right now, there is test generation feature in mgmt autorest.CSharp. The generated tests can be seen at https://github.com/Azure/autorest.csharp/tree/d6dd66df44128e4c7b09025e1debd824e2482468/test/TestProjects/MgmtKeyvault/tests/Generated/Mock

Currently these generated tests are mock tests that can be execute under the mock-service-host(https://github.com/Azure/azure-sdk-tools/tree/main/tools/mock-service-host). And meanwhile, we are in the procedure to generate live test based on swagger api scenario (https://github.com/Azure/azure-rest-api-specs/tree/main/documentation/api-scenario).

Motivation

We want the test Generation feature can be verified in 1) sdk repo when upgrading autorest.CSharp & 2) autorest.csharp TestProjects.

API / Feature Proposal

Feature1: verify Test Generation in SDK repo when upgrading autorest.CSharp

The "dotnet build /t:GenerateTest" has been supported in Autorest.CSharp, we want it's invoked in autorest.CSharp pipeline for all released Packages just like that now we are using "dotnet build /t:GenerateCode" to verify generated source code. (There is flag to control whether we enabled testGeneration for a single RP)

Feature 2: verify Test Generation result in TestProjects in Autorest.CSharp

We want to create some testProjects in AutorestCSharp which can verify generated api-scenario test. Since there will be ARM-Template invocations in generated test, we come up two options to make these testProjects compilable in autorest.CSharp:

option 1: adding Faked Azure.ResourceManager.Resources and TestFramework classes in Autorest.CSharp, like we did here https://github.com/Azure/autorest.csharp/tree/feature/v3/src/assets/TestFramework
option 2: Release TestFramework into nuget, and adding real Azure.ResourceManager.Resources and TestFramework as dependencies of Autorest.CSharp. Since these packages in turn depend on Autorest.CSharp, there may be potential cyclic dependency issues doing this.

Would you please share your opinion on Feature1 and which option should I go for Feature2? Thanks!

Time Constraints and Dependencies

Sooner is better.

Stakeholders

@m-nash , @ArcturusZhang, @ArthurMa1978 , @lirenhe

jsquire commented 2 years ago

Thank you for the well-written issue. Transferring this over to the Autorest repository so that it can be tracked as part of CodeGen work.

changlong-liu commented 2 years ago

Hi @jsquire and reviewers,

Allow me to clarify that this actually is not a feature-request, but it's for asking opinion from Azure Core and EngSys team. I will start these work if you think they are good ideas (may need help on TestFramework part).

The two features are ideas I comes up together with @m-nash and @ArcturusZhang. Since the Feature1 is adding feature in SDK pipeline and Feature2 is common part of Autorest.CSharp, we want to ask your opinion that whether these are right direction. Looking forward to any suggestions. Thanks.

jsquire commented 2 years ago

Thank you for the clarification, @changlong-liu. I've reclassified and reached out to stakeholders for discussion.

christothes commented 2 years ago

A few questions: Feature 1:

In the SDK repo, the generated code validation is to ensure that generated code is not out of sync with the referenced swagger. Could you explain what the value of this is for test code?

Feature 2:

Could you clarify what we intend to validate with these tests specifically? In the current form, they appear to just validate that the various API calls don't throw.
Should these mock tests be marked with the RecordedTest attribute and inherit from RecordedTestBase? I'd expect mock tests to not be recorded.
- If yes, is it intentional that the tests are hard coded to Record mode?

nits: I believe ServicePointManager.ServerCertificateValidationCallback only applies to full framework. Also, you may want to look at HttpPipelineTransportOptions

changlong-liu commented 2 years ago

Hi @christothes , thanks for taking look at the test generation.

Below are my thinking: Feature 1:

Adding test validation to SDK repo can ensure the generated test are good in new autorest.CSharp for all released packages.

Both Feature1 and Feature2 intend to prevent the test generation from falling behind the source code generation. While Feature1 provide full coverage on released package, and Feature2 provide short development circle with TestProjects.

Feature2:

Yes, currently it is mainly for validating that the serialization and deserialization don't throw. And since these test are generated with swagger examples and api scenario, we potentially can add response asserts in these generated tests.
With RecordedTest, these mock test can be executed in pipeline without a mock-service-host started in pipeline. And we have code to execute mock test in Playback mode when mock-service-host can't be found https://github.com/Azure/azure-sdk-for-net/blob/main/common/ManagementTestShared/Redesign/MockTestBase.cs#L18.

I believe ServicePointManager.ServerCertificateValidationCallback only applies to full framework....

Thank you, Christopher! That's exactly as your said, the HttpPipelineTransportOptions will really help!

m-nash commented 2 years ago

@christothes we want to treat test generation the same as code generation in that you should never directly modify the generated code and only modify via customization outside the generated folder. To achieve this we want to follow the exact same practice we have for source code which is to always generate and compare during CI. When we add this initially there will be functionally no change since no projects adopt the auto-generated tests yet, but in the near future projects will opt into this and having this in place for day one would be a huge advantage.

Azure / autorest.csharp