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.

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

Conversation

martincostello
Copy link
Member

Fixes for OpenAPI document generation tool

Fix two issues with the M.E.ApiDescription.Server package.

Description

  • Convert $(OpenApiDocumentsDirectory) to a full path before passing it to the dotnet-getdocument tool.
  • Do not emit warning about an invalid OpenAPI version in the dotnet-getdocument tool if no explicit value was passed to the tool via the --openapi-version argument.
  • Fix broken tests for the MSBuild targets.

Relates to #57044

Convert `$(OpenApiDocumentsDirectory)` to a full path before passing it to the `dotnet-getdocument` tool.
Relates to dotnet#57044.
Do not emit warning about an invalid OpenAPI version in the `dotnet-getdocument` tool if no explicit value was passed to the tool via the `` argument.
Relates to dotnet#57044.
Fix tests failing locally in Visual Studio due to test assets not being copied to the output directory.
Fix assertions looking for relative paths instead of absolute paths.
@ghost ghost added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Jul 31, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 31, 2024
@@ -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.

@captainsafia captainsafia merged commit 24e0d10 into dotnet:main Jul 31, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-rc1 milestone Jul 31, 2024
@martincostello martincostello deleted the gh-57044 branch July 31, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI community-contribution Indicates that the PR has been added by a community member feature-openapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants