Set up spector tests generation for http-client-csharp-mgmt - #54206
Wei Hu (live1206) with Copilot wants to merge 12 commits into
Conversation
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Wei Hu (live1206)
left a comment
There was a problem hiding this comment.
should rerun Generate.ps1 to update launchSetting.json file, CI is failing due to this
| <IsClientLibrary>true</IsClientLibrary> | ||
| <IsTestProject>false</IsTestProject> |
There was a problem hiding this comment.
we can just rename TestProjects.Spector.Tests.csproj to start with Azure.Generator., so that it will be client library and not a test project.
There was a problem hiding this comment.
Renamed to Azure.Generator.Spector.Tests.csproj in 02a7753.
| <!-- | ||
| Add any shared properties you want for the projects under this package directory that need to be set before the auto imported Directory.Build.props | ||
| --> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory).., Directory.Build.props))\Directory.Build.props" /> |
There was a problem hiding this comment.
if we remove the propertyGroup above, may this file is not needed anymore.
There was a problem hiding this comment.
Removed the file in 02a7753 since it's no longer needed with the renamed csproj.
|
Copilot take a look at the comments above |
… reorder package.json, update launchSettings.json, fix lint issues Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Reran |
|
Copilot can you set up spector generated code as in https://github.com/Azure/azure-sdk-for-net/tree/main/eng/packages/http-client-csharp/generator/TestProjects/Spector/http/authentication/api-key? And also, some corresponding verification tests as in https://github.com/Azure/azure-sdk-for-net/blob/main/eng/packages/http-client-csharp/generator/TestProjects/Spector.Tests/Http/Authentication/ApiKey/ApiKeyTests.cs |
…r specs Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Added generated spector code and verification tests in 9cf5891. The setup includes:
Note: Some specs (common-properties, non-resource, resources) have build errors in the generated code that will need generator fixes. |
|
Copilot skip the generation for |
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Skipped generation for |
There was a problem hiding this comment.
Pull request overview
This PR sets up the Spector test infrastructure for the http-client-csharp-mgmt generator, mirroring the existing setup in http-client-csharp. This enables testing of the management SDK generator against Azure resource-manager specifications from @azure-tools/azure-http-specs.
Key Changes:
- Added PowerShell scripts for Spector test generation and execution
- Created test infrastructure including server management and test attributes
- Generated initial SDK code for several resource-manager specs
- Added test project with model serialization verification tests
Reviewed changes
Copilot reviewed 32 out of 262 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
eng/scripts/Spector-Helper.psm1 |
Helper module to enumerate and manage resource-manager specs |
eng/scripts/Test-Spector.ps1 |
Test runner that regenerates, tests, and restores each spec |
eng/scripts/Generate.ps1 |
Updated to generate Spector test projects alongside existing Mgmt-TypeSpec projects |
eng/scripts/Generation.psm1 |
Removed deprecated plugin-name option |
TestProjects/Spector.Tests/Azure.Generator.Spector.Tests.csproj |
Test project renamed to follow Azure client library naming convention |
TestProjects/Spector.Tests/Infrastructure/*.cs |
Test infrastructure for server management, attributes, and model serialization |
TestProjects/Spector.Tests/Http/Azure/ResourceManager/OperationTemplates/OrderDataTests.cs |
Verification test for operation-templates spec |
TestProjects/Spector/http/azure/resource-manager/* |
Generated SDK code and configuration for resource-manager specs |
launchSettings.json |
Updated with spector profiles for resource-manager specs |
| "http-azure-resource-manager-common-properties": { | ||
| "commandLineArgs": "$(SolutionDir)/../dist/generator/Microsoft.TypeSpec.Generator.dll $(SolutionDir)/TestProjects/Spector/http/azure/resource-manager/common-properties -g MgmtStubGenerator", | ||
| "commandName": "Executable", | ||
| "executablePath": "dotnet" | ||
| }, |
There was a problem hiding this comment.
[nitpick] Profile names should use kebab-case consistently. The profile 'http-azure-resource-manager-common-properties' is already in kebab-case which is correct, but this should be verified against other profiles for consistency.
| return nodeModulesDirectory; | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"Cannot find 'node_modules' in parent directories of {typeof(SpectorServer).Assembly.Location}."); |
There was a problem hiding this comment.
Error message references SpectorServer type but this is in the TestServerBase class. The type should be TestServerBase or this.GetType() for accuracy.
| throw new InvalidOperationException($"Cannot find 'node_modules' in parent directories of {typeof(SpectorServer).Assembly.Location}."); | |
| throw new InvalidOperationException($"Cannot find 'node_modules' in parent directories of {typeof(TestServerBase).Assembly.Location}."); |
| { | ||
| if (kebabCaseDirectories) | ||
| { | ||
| return ToKebabCase().Replace(part.StartsWith("_", StringComparison.Ordinal) ? part.Substring(1) : part, "-$1").ToLowerInvariant(); |
There was a problem hiding this comment.
[nitpick] Use part[1..] instead of part.Substring(1) for consistency with modern C# range syntax, as seen on line 96.
| return ToKebabCase().Replace(part.StartsWith("_", StringComparison.Ordinal) ? part.Substring(1) : part, "-$1").ToLowerInvariant(); | |
| return ToKebabCase().Replace(part.StartsWith("_", StringComparison.Ordinal) ? part[1..] : part, "-$1").ToLowerInvariant(); |
| } | ||
|
|
||
| # Extract the relative path after "specs/" and normalize slashes | ||
| $relativePath = ($specFile -replace '[\\\/]', '/').Substring($_.FullName.IndexOf($pattern) + $pattern.Length) |
There was a problem hiding this comment.
Using IndexOf without checking if the pattern exists first could result in -1 + pattern.Length if pattern is not found, causing an invalid substring index.
| $relativePath = ($specFile -replace '[\\\/]', '/').Substring($_.FullName.IndexOf($pattern) + $pattern.Length) | |
| $normalizedFullName = $_.FullName -replace '[\\\/]', '/' | |
| $patternIndex = $normalizedFullName.IndexOf($pattern) | |
| if ($patternIndex -ge 0) { | |
| $relativePath = ($specFile -replace '[\\\/]', '/').Substring($patternIndex + $pattern.Length) | |
| } else { | |
| Write-Host "Warning: Pattern '$pattern' not found in '$normalizedFullName'. Skipping this item." -ForegroundColor Yellow | |
| $relativePath = "" | |
| } |
| } | ||
|
|
||
| internal static object? InvokeMethod(object obj, string methodName, params object[] args) | ||
| => InvokeMethodInternal(obj.GetType(), obj, methodName, [], |
There was a problem hiding this comment.
[nitpick] Empty array literal [] for genericArgs parameter is passed but could be clarified with Array.Empty<Type>() for better readability.
| => InvokeMethodInternal(obj.GetType(), obj, methodName, [], | |
| => InvokeMethodInternal(obj.GetType(), obj, methodName, Array.Empty<Type>(), |
| $testFilter += "._$segment" | ||
| $testPath = Join-Path $testPath "_$segment" | ||
| } | ||
| else{ |
There was a problem hiding this comment.
[nitpick] Missing space before opening brace on line 42. Should be } else { for consistency with PowerShell style guidelines.
| else{ | |
| else { |
|
|
||
| namespace Azure.ResourceManager.Resources | ||
| { | ||
| internal partial class ChangeTrackingDictionary<TKey, TValue> : IDictionary<TKey, TValue>, IReadOnlyDictionary<TKey, TValue> |
There was a problem hiding this comment.
Shouldn't we be checking in the stubbed versions of the libraries?
| } | ||
| $command += " --option @azure-typespec/http-client-csharp-mgmt.emitter-output-dir=$generationDir" | ||
| $command += " --option @azure-typespec/http-client-csharp-mgmt.save-inputs=true" | ||
| if ($generateStub) { |
There was a problem hiding this comment.
Reverted the changes to Generation.psm1 in e04b06d.
There was a problem hiding this comment.
Copilot after reverting, regenerate the spector tests as well
There was a problem hiding this comment.
Regenerated the spector tests after the revert. The generated code remains unchanged because the Spector generation uses -g MgmtStubGenerator flag directly via launchSettings profiles, which is not affected by the plugin-name option in Generation.psm1.
There was a problem hiding this comment.
Regenerated the spector tests after the revert. The generated code remains unchanged because the Spector generation uses
-g MgmtStubGeneratorflag directly via launchSettings profiles, which is not affected by theplugin-nameoption in Generation.psm1.
Can we update the flag from MgmtStubGenerator to StubGenerator and make the regen work with stubbed version?
There was a problem hiding this comment.
Updated to use AzureStubGenerator in a646e35. Changed the -g flag from MgmtStubGenerator to AzureStubGenerator in Generate.ps1 and regenerated the launchSettings.json.
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
|
Copilot merge from latest main branch and resolve the conflicts |
Merged latest main branch in e89e815. Resolved conflicts in package.json by updating TypeSpec package versions to match main (1.7.0) while keeping the @typespec/spec-api and @typespec/spector additions. Regenerated package-lock.json with npm install --legacy-peer-deps to work around peer dependency version mismatch. |
|
Hi Copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
2 similar comments
|
Hi Copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Hi Copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Description
Sets up spector tests infrastructure for http-client-csharp-mgmt, mirroring the existing setup in http-client-csharp. This enables testing the management SDK generator against azure-http-specs resource-manager scenarios.
Changes
Scripts (
eng/packages/http-client-csharp-mgmt/eng/scripts/)Spector-Helper.psm1- Helper module to enumerate resource-manager specs from@azure-tools/azure-http-specsTest-Spector.ps1- Test runner that regenerates, tests, and restores each specGenerate.ps1- Updated to generate Spector test projects alongside existing Mgmt-TypeSpec (usesAzureStubGeneratorfor spector profiles)Test Infrastructure (
generator/TestProjects/Spector.Tests/)Azure.Generator.Spector.Tests.csproj- Test project (renamed fromTestProjects.Spector.Tests.csprojto follow client library naming convention)SpectorTestBase.cs- Base class with test helpersInfrastructure/- Server management, test attributes, model serialization testsHttp/Azure/ResourceManager/OperationTemplates/OrderDataTests.cs- Verification test for operation-templates specGenerated Spector Code (
generator/TestProjects/Spector/)common-properties,large-header,non-resource,operation-templates,resourceslarge-header,operation-templates)Dependencies (
package.json)@typespec/spectorand@typespec/spec-apidevDependencies (in alphabetical order)Launch Settings
launchSettings.jsonwith spector profiles for resource-manager specs usingAzureStubGeneratorResource Manager Specs Covered
common-properties,large-header,non-resource,operation-templates,resourcesSkipped Specs
method-subscription-id- Skipped due to "Some file paths are too long" error in CIcommon-properties,non-resource,resources- Have build errors in the generated code that need to be fixed in the generator (commented out in csproj)Merge
package.jsonandpackage-lock.jsonpackage-lock.jsonusingnpm install --legacy-peer-depsto work around peer dependency version mismatchUsage
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.