Improve template to include tests, live resources, recordings etc #11540

pakrym · 2020-04-23T19:28:05Z

No description provided.

pakrym · 2020-04-23T19:28:43Z

eng/ApiListing.targets

@@ -20,7 +20,13 @@
      <GenAPIAdditionalParameters>--exclude-attributes-list "$(MSBuildThisFileDirectory)ApiListing.exclude-attributes.txt" $(GenAPIAdditionalParameters)</GenAPIAdditionalParameters>
      <GenAPITargetPath>$(_RefSourceOutputPath)$(AssemblyName).$(TargetFramework).cs</GenAPITargetPath>
    </PropertyGroup>
-    <Delete Files="$(GenAPITargetPath)" />
+    <ItemGroup>
+      <!-- Remove all files that don't start with $(AssemblyName) and the current target -->


Fixes an issue where copying Azure.Template and re-running Export-API leaves the original API listing around and confuses people.

pakrym · 2020-04-23T19:29:22Z

sdk/core/Azure.Core/tests/TestFramework/RecordEntry.cs

@@ -75,11 +76,11 @@ private static byte[] DeserializeBody(IDictionary<string, string[]> headers, in
            {
                if (property.ValueKind == JsonValueKind.Object)
                {
-                    var arrayBufferWriter = new ArrayBufferWriter<byte>();
-                    using var writer = new Utf8JsonWriter(arrayBufferWriter);
+                    using var memoryStream = new MemoryStream();


Don't need to have IVT from lib to tests or manually include ArrayBufferWriter into test project anymore.

pakrym · 2020-04-23T19:45:47Z

cc @timtay-microsoft @drwill-ms

Do you remember any snags that you've hit during bootstrapping of the library?

drwill-ms · 2020-04-23T20:34:03Z

@pakrym Files we needed to include appear in the root of the csproj, but we don't want them cluttering up the root with our own files, and have someone accidentally edit them.

One thing we did was hide them under the same folder as other imports from the props file. However, it would have been nicer if these were included from that props file automatically, and under the same folder.

  <!-- Common source from Azure.Core -->
  <ItemGroup>
    <Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)ClientDiagnostics.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)HttpMessageSanitizer.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)OperationHelpers.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
    <Compile Include="$(AzureCoreSharedSources)Argument.cs">
      <LinkBase>Shared\Azure.Core</LinkBase>
    </Compile>
  </ItemGroup>

drwill-ms · 2020-04-23T20:35:40Z

@pakrym Another small thing in porting our existing code into this project was finding out what to remove, things we didn't need. We had lots of nuget package properties that are filled in automatically. It would be nice if the template pulled all the nuget-specific properties and put them in their own property group, with a comment that says these, and only these, should be supplied.

drwill-ms · 2020-04-23T20:38:14Z

@pakrym I also found I got build errors until I included these packages. If they are required, perhaps they could be included by the props file by default.

  <!-- required by TestFramework.props -->
  <ItemGroup>
    <PackageReference Include="NUnit" />
    <PackageReference Include="NUnit3TestAdapter" />
    <PackageReference Include="Castle.Core" />
  </ItemGroup>

drwill-ms · 2020-04-23T20:40:31Z

@pakrym For a while, I had to include this file to avoid a build error. Once we started having tests that inherited from ClientTestBase, etc. then I got an error when I did include it.

    <!-- Not sure why we don't get this file with TestFramework.props, but I have to add it to avoid a build error. -->
    <Compile Include="$(RepoRoot)\sdk\core\Azure.Core\src\Shared\ArrayBufferWriter.cs">
      <LinkBase>TestFramework</LinkBase>
    </Compile>

pakrym · 2020-04-23T22:15:15Z

@pakrym For a while, I had to include this file to avoid a build error. Once we started having tests that inherited from ClientTestBase, etc. then I got an error when I did include it.

Fixed as part of this PR.

@pakrym I also found I got build errors until I included these packages. If they are required, perhaps they could be included by the props file by default.
@pakrym Files we needed to include appear in the root of the csproj, but we don't want them cluttering up the root with our own files, and have someone accidentally edit them.

Tracked by #11544

tg-msft

Very nice!

sdk/template/Azure.Template/src/MiniSecretsClientOptions.cs

pakrym · 2020-04-23T22:24:21Z

@pakrym Files we needed to include appear in the root of the csproj, but we don't want them cluttering up the root with our own files, and have someone accidentally edit them.

Tracked by #9775

heaths

Should also add a CONTRIBUTING.md file - at least with a section like Key Vault and Search now have that refer back to the TestResources/README.md file.

heaths · 2020-04-23T22:27:32Z

sdk/template/test-resources.json

+          }
+    ],
+    "outputs": {
+        "KEYVAULT_URL": {


AZURE_KEYVAULT_URL

Because that's what the guidelines state, and what we're discussing in another bug.

Guidelines are about client libraries not test infrastructure.

We're asking contributors to set environment variables. These scripts aren't just for us. The point is to make PII easy to identify and clean up, rather than spread sprawl them across an environment block to the point someone may miss PII and secrets leak. That's concerning. Grouping them with a common prefix makes them easy to identify and remove.

We should group them but do it in a common place in the New-TestEnvironment script not throughout tens of test-resources.json files

I don't see how that has anything to do with naming them with a common prefix. Yes, a common prefix should be done consistently in all locations as you pointed out in the other issue.

We control how these outputs are turned into environment variables in a New-TestEnvironment script. With one change there we can make all the environment variables it creates compliant with any rules we want and not require prefixes in templates at all.

And BTW this follows the guidelines for test variables precisely: https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki/2/Azure-DevOps-Pipeline-Guidance?anchor=environment-variables

Environment variables should follow the convention of <service>_<variable>. For example, if you are running live tests and need an environment variable for an Event Hubs instance name, the name would be EVENTHUB_NAME. The parts are Service: EVENTHUB Variable: NAME

pakrym · 2020-04-23T23:14:28Z

I'll follow up with more doc changes in other PRs.

Improve template to include tests, live resources, recordings etc

c83b9f1

pakrym requested review from tg-msft, christothes, heaths and weshaggard April 23, 2020 19:28

pakrym requested review from AlexGhiondea, chidozieononiwu, danieljurek, KrzysztofCwalina and mitchdenny as code owners April 23, 2020 19:28

pakrym commented Apr 23, 2020

View reviewed changes

Add and use API version

7087aa4

pakrym added 2 commits April 23, 2020 12:49

Autorest.md

cdfd1ba

Snippet

74c7d3d

Export API

51134f2

tg-msft approved these changes Apr 23, 2020

View reviewed changes

sdk/template/Azure.Template/src/MiniSecretsClientOptions.cs Outdated Show resolved Hide resolved

FB

43d4d19

heaths approved these changes Apr 23, 2020

View reviewed changes

Again

7a49c5b

pakrym merged commit cebda74 into Azure:master Apr 23, 2020

pakrym deleted the pakrym/improve-template branch April 23, 2020 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve template to include tests, live resources, recordings etc #11540

Improve template to include tests, live resources, recordings etc #11540

pakrym commented Apr 23, 2020

pakrym Apr 23, 2020

pakrym Apr 23, 2020

pakrym commented Apr 23, 2020 •

edited

Loading

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

pakrym commented Apr 23, 2020

tg-msft left a comment

pakrym commented Apr 23, 2020

heaths left a comment

heaths Apr 23, 2020

pakrym Apr 23, 2020

heaths Apr 23, 2020

pakrym Apr 23, 2020

heaths Apr 23, 2020

pakrym Apr 23, 2020

heaths Apr 23, 2020

pakrym Apr 23, 2020

pakrym Apr 23, 2020

pakrym commented Apr 23, 2020

Improve template to include tests, live resources, recordings etc #11540

Improve template to include tests, live resources, recordings etc #11540

Conversation

pakrym commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Apr 23, 2020 • edited Loading

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

drwill-ms commented Apr 23, 2020

pakrym commented Apr 23, 2020

tg-msft left a comment

Choose a reason for hiding this comment

pakrym commented Apr 23, 2020

heaths 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

pakrym commented Apr 23, 2020

pakrym commented Apr 23, 2020 •

edited

Loading