Skip to content

Fixes for OpenAPI document generation with M.E.ApiDescription.Server #57096

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Fixes for OpenAPI document generation with M.E.ApiDescription.Server #57096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e9ec8d
Get full path of OpenApiDocumentsDirectory
martincostello Jul 31, 2024
5e2df1b
Avoid warning about invalid OpenAPI version
martincostello Jul 31, 2024
5a4e44f
Fix TargetTest tests
martincostello Jul 31, 2024
d383e08
Fix tests
martincostello Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...nsions.ApiDescription.Client/test/Microsoft.Extensions.ApiDescription.Client.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
Link="ProcessHelpers\DotNetMuxer.cs" />
<Compile Include="$(ToolSharedSourceRoot)TestHelpers\Temporary*.cs" LinkBase="TestHelpers" />

<Content Include="..\src\build\**\*" LinkBase="build" />
<Content Include="..\src\buildMultiTargeting\**\*" LinkBase="buildMultiTargeting" />
<Content Include="TestProjects\**\*" />
<Content Include="..\src\build\**\*" LinkBase="build" CopyToOutputDirectory="PreserveNewest" />
<Content Include="..\src\buildMultiTargeting\**\*" LinkBase="buildMultiTargeting" CopyToOutputDirectory="PreserveNewest" />
<Content Include="TestProjects\**\*" CopyToOutputDirectory="PreserveNewest" />

<Reference Include="Microsoft.Extensions.ApiDescription.Client" />
</ItemGroup>
Expand Down
8 changes: 4 additions & 4 deletions src/Tools/Extensions.ApiDescription.Client/test/TargetTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public async Task AddsExpectedItems()

Assert.Equal(0, process.ExitCode);
Assert.Empty(process.Error);
Assert.Contains($"Compile: {Path.Combine("obj", "azureMonitorClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine(_temporaryDirectory.Root, "obj", "azureMonitorClient.cs")}", process.Output);
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiousity, what change required this update to the tests?

I didn't think that ApiDescription.Client shared any of the codepaths with ApiDescription.Server to expect this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I run the tests on my machine the MSBuild(?) output contains full paths to the files in the temp folder where the scratch project the tests use gets put. These asserts were looking for short relative paths so they failed.

I think the column in Test Explorer was truncated when I ran them, so I didn't spot they were the Client rather than Server tests, but they failed so I went looking as to why as they had "Targets" in the name and I'd touched a target file...

Copy link
Member

Choose a reason for hiding this comment

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

OK -- I think I figured out what the fishiness going on here is.

It appears that these tests were quarantined in this PR (#50362) when we updated the repo to the .NET 9 TFM back in the fall. The issue linked from the QuarantinedTest attribute isn't super descriptive about what the actual test failures were (#50662).

I took a peek at the quarantined test logs and it appears that the tests have had a 0% pass rate for the last few months with an exception similar to what you probably saw locally:

Assert.Contains() Failure
Not found: Compile: obj�zureMonitorClient.cs
In value:    Determining projects to restore...
             Restored C:hwB2C30A21	dotnet-tool-tests8dbd6a7755a942b5bcf5ae73f46f1f0d	est.csproj (in 157 ms).
           C:hwB2C30A21pdotnet-clisdk9.0.100-preview.7.24371.4SdksMicrosoft.NET.Sdk	argetsMicrosoft.NET.RuntimeIdentifierInference.targets(326,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [C:hwB2C30A21	dotnet-tool-tests8dbd6a7755a942b5bcf5ae73f46f1f0d	est.csproj]
             GenerateNSwagCSharp C:hwB2C30A21	dotnet-tool-tests8dbd6a7755a942b5bcf5ae73f46f1f0diles�zureMonitor.json Class: 'test.azureMonitorClient' FirstForGenerator: 'true' Options: '' OutputPath: 'obj�zureMonitorClient.cs'
             Compile: C:hwB2C30A21	dotnet-tool-tests8dbd6a7755a942b5bcf5ae73f46f1f0dobj�zureMonitorClient.cs
             FileWrites: obj�zureMonitorClient.cs
             test -> C:hwB2C30A21	dotnet-tool-tests8dbd6a7755a942b5bcf5ae73f46f1f0d�inDebug
et9.0	est.dll
           
           Build succeeded.
               0 Warning(s)
               0 Error(s)
           
           Time Elapsed 00:00:02.47

So, it seems like the upgrade to .NET 9 caused this test failure, the test was quarantined as part of the upgrade, and the underlying issue wasn't resolved because it wasn't explicitly tracked as its own failure. It's not clear to me why the transition to .NET 9 would've started causing issues in the test though 🤔

I suspect that once we merge this PR, we'll see the pass rate for this test move up to 100% and we can eventually unquarantine it.

This was a good find, Martin! TY for helping discover this.

Assert.Contains($"FileWrites: {Path.Combine("obj", "azureMonitorClient.cs")}", process.Output);
Assert.DoesNotContain("TypeScriptCompile:", process.Output);
}
Expand Down Expand Up @@ -120,9 +120,9 @@ public async Task AddsExpectedItems_WithMultipleFiles()

Assert.Equal(0, process.ExitCode);
Assert.Empty(process.Error);
Assert.Contains($"Compile: {Path.Combine("obj", "azureMonitorClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine("obj", "NSwagClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine("obj", "swashbuckleClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine(_temporaryDirectory.Root, "obj", "azureMonitorClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine(_temporaryDirectory.Root, "obj", "NSwagClient.cs")}", process.Output);
Assert.Contains($"Compile: {Path.Combine(_temporaryDirectory.Root, "obj", "swashbuckleClient.cs")}", process.Output);
Assert.Contains($"FileWrites: {Path.Combine("obj", "azureMonitorClient.cs")}", process.Output);
Assert.Contains($"FileWrites: {Path.Combine("obj", "NSwagClient.cs")}", process.Output);
Assert.Contains($"FileWrites: {Path.Combine("obj", "swashbuckleClient.cs")}", process.Output);
Expand Down
4 changes: 3 additions & 1 deletion ...nsions.ApiDescription.Server/src/build/Microsoft.Extensions.ApiDescription.Server.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,11 @@
Text="OpenAPI document generation is not supported when targeting netcoreapp2.0 or earlier. Disable the feature or move to a later target framework." />

<PropertyGroup>
<_DotNetGetDocumentOutputPath>$(OpenApiDocumentsDirectory.TrimEnd('\'))</_DotNetGetDocumentOutputPath>
<_DotNetGetDocumentOutputPath>$([System.IO.Path]::GetFullPath('$(_DotNetGetDocumentOutputPath)'))</_DotNetGetDocumentOutputPath>
<_DotNetGetDocumentCommand>dotnet "$(MSBuildThisFileDirectory)../tools/dotnet-getdocument.dll" --assembly "$(TargetPath)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand>$(_DotNetGetDocumentCommand) --file-list "$(_OpenApiDocumentsCache)" --framework "$(TargetFrameworkMoniker)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand>$(_DotNetGetDocumentCommand) --output "$(OpenApiDocumentsDirectory.TrimEnd('\'))" --project "$(MSBuildProjectName)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand>$(_DotNetGetDocumentCommand) --output "$(_DotNetGetDocumentOutputPath)" --project "$(MSBuildProjectName)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand Condition=" '$(ProjectAssetsFile)' != '' ">$(_DotNetGetDocumentCommand) --assets-file "$(ProjectAssetsFile)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand Condition=" '$(PlatformTarget)' != '' ">$(_DotNetGetDocumentCommand) --platform "$(PlatformTarget)"</_DotNetGetDocumentCommand>
<_DotNetGetDocumentCommand Condition=" '$(PlatformTarget)' == '' AND '$(Platform)' != '' ">$(_DotNetGetDocumentCommand) --platform "$(Platform)"</_DotNetGetDocumentCommand>
Expand Down
5 changes: 4 additions & 1 deletion src/Tools/GetDocumentInsider/src/Commands/GetDocumentCommandWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,10 @@ private string GetDocument(
}
else
{
_reporter.WriteWarning(Resources.FormatInvalidOpenApiVersion(_context.OpenApiVersion));
if (!string.IsNullOrWhiteSpace(_context.OpenApiVersion))
{
_reporter.WriteWarning(Resources.FormatInvalidOpenApiVersion(_context.OpenApiVersion));
}
arguments = [documentName, writer, OpenApiSpecVersion.OpenApi3_0];
}
}
Expand Down
Loading