Integrate apis.guru directory of APIs to provide smoke test for library#220
Integrate apis.guru directory of APIs to provide smoke test for library#220darrelmiller merged 11 commits intomasterfrom
Conversation
|
|
||
| return array; | ||
| } | ||
|
|
| Diagnostic.Errors.Add( | ||
| new OpenApiError("", $"{nodeName} must be a map/object at " + Context.GetLocation())); | ||
| throw new OpenApiException($"{nodeName} must be a map/object"); | ||
| //Diagnostic.Errors.Add( |
| { | ||
| Diagnostic.Errors.Add( | ||
| new OpenApiError("", $"{nodeName} must be a map/object at " + Context.GetLocation())); | ||
| throw new OpenApiException($"{nodeName} must be a map/object"); |
There was a problem hiding this comment.
why change to throw exception? #Resolved
There was a problem hiding this comment.
Because it caused a cascade of errors as following code attempted to handle the null node. #Resolved
|
|
||
| public virtual List<IOpenApiAny> CreateListOfAny() | ||
| { | ||
| throw new OpenApiException("Cannot create list of any from here"); |
There was a problem hiding this comment.
Maybe it's better to throw "NotSupported" #WontFix
There was a problem hiding this comment.
It definitely could use a better description. However, I want this error to be aggregated in the errors collection. #Resolved
There was a problem hiding this comment.
I've cleaned up the descriptions a bit so they are consistent. #Resolved
| try | ||
| { | ||
| return _currentDocument.ResolveReference(reference) as T; | ||
| } catch(OpenApiException ex) |
There was a problem hiding this comment.
separate to different line #Resolved
| o.Components.RequestBodies = n.CreateMapWithReference(ReferenceType.RequestBody, p => | ||
| { | ||
| var parameter = LoadParameter(p, evenBody: true); | ||
| if (parameter.In == null) { |
There was a problem hiding this comment.
separate to different line #Resolved
| { | ||
| case ReferenceType.Schema: | ||
| return this.Components.Schemas[reference.Id]; | ||
| return this.Components.Schemas[reference.Id]; |
| @@ -0,0 +1,107 @@ | |||
| using Microsoft.OpenApi.Readers; | |||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Net; | ||
| using System.Net.Http; |
|
|
||
| public ApisGuruTests(ITestOutputHelper output) | ||
| { | ||
|
|
There was a problem hiding this comment.
remove empty line #Resolved
| AutomaticDecompression = DecompressionMethods.GZip | ||
| }); | ||
|
|
||
| private readonly ITestOutputHelper output; |
There was a problem hiding this comment.
name it as "_output", same as _httpClient? #Resolved
| [Collection("DefaultSettings")] | ||
| public class ApisGuruTests | ||
| { | ||
| private static HttpClient _httpClient = new HttpClient(new HttpClientHandler() { |
There was a problem hiding this comment.
If you have the static constructor, I think it's better to move the assignment to the static constructor. #Resolved
|
|
||
| public static IEnumerable<object[]> GetSchemas() | ||
| { | ||
|
|
There was a problem hiding this comment.
remove empty line #Resolved
| return jToken; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
remove unused empty line #Resolved
| { | ||
| output.WriteLine($"Errors parsing {url}"); | ||
| output.WriteLine(String.Join("\n", diagnostic.Errors)); | ||
| // Assert.True(false); // Uncomment to identify descriptions with errors. |
There was a problem hiding this comment.
I'd like to leave that. Once we ensure that the smoketests are not run during the automatic build, I think we might re-enable this. #WontFix
| [MemberData(nameof(GetSchemas))] | ||
| public async Task EnsureThatICouldParse(string url) | ||
| { | ||
| var stopwatch = new Stopwatch(); |
There was a problem hiding this comment.
move the declaration & assignment at the first using of this stopwatch. #Resolved
| var response = await _httpClient.GetAsync(url); | ||
| if (!response.IsSuccessStatusCode) | ||
| { | ||
| output.WriteLine($"Couldn't load {url}"); |
There was a problem hiding this comment.
It's a test case. it can't understand to output the error message and returns directly.
Please use the assert clause to test the status code.
#WontFix
There was a problem hiding this comment.
These are smoke tests and shouldn't be treated the same way as integration tests. If a URL cannot be resolved from apis.guru that is not a failure of our library, but an issue with an external directory, so I wouldn't want to flag it as an error. #Closed
|
|
||
| stopwatch.Start(); | ||
|
|
||
| var reader = new OpenApiStreamReader(new OpenApiReaderSettings() { |
There was a problem hiding this comment.
what's the meaning of empty {} #Resolved
There was a problem hiding this comment.
Removed. #Resolved
| }); | ||
| var openApiDocument = reader.Read(stream, out var diagnostic); | ||
|
|
||
| if (diagnostic.Errors.Count > 0) |
There was a problem hiding this comment.
same as above, it's a test case, please use the assert clause to test the result. #WontFix
There was a problem hiding this comment.
Asserting on the existence of errors in the collection would give erroneous results as many of the OpenAPI descriptions contain spec violations. #Closed
There was a problem hiding this comment.
Hi @darrelmiller - we use multiple validators on the APIs,guru collection, so if this applies to us, we'd certainly appreciate feedback on the spec violations when you have time. #Closed
There was a problem hiding this comment.
@MikeRalphson The two main errors that I am seeing are invalid characters in the component keys and schemas that have a type as an array. I'll get you a more complete list. Also, I know many of the Azure documents have description properties as peers of a $ref. #Resolved
There was a problem hiding this comment.
@darrelmiller many thanks! #Resolved
| // Resolve References if requested | ||
| switch (_settings.ReferenceResolution) | ||
| { | ||
| case ReferenceResolutionSetting.ResolveAllReferences: |
There was a problem hiding this comment.
maybe to use the "default: throw ..." is better. #WontFix
There was a problem hiding this comment.
This chunk of code is going to change fairly significantly when adding support for resolving external files, so I'll review this in a future PR. #Closed
|
|
||
| public override List<IOpenApiAny> CreateListOfAny() | ||
| { | ||
| var yamlSequence = _nodeList; |
There was a problem hiding this comment.
yamlSequence is not necessary. Why can't use the _nodeList directly? #Resolved
|
@PerthCharern Can I get a quick review? I'd like to merge this into the release for @scott-lin |
3 participants
This PR also includes a bunch of fixes that prevent any of the Apis.guru descriptions from throwing exceptions.
Fixes #151