-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[csharp-netcore][httpclient] Issues managing error statuses #9389
Conversation
@lucamazzanti thanks for the PR. Can you please also update the samples so that the CI can verify the change? |
Sorry now is completely done. |
Suggestion: what about adding a test or 2 with |
I'll do it right now |
I added two samples to test the ApiException in case of 404, one with HttpInfo with the ExceptionFactory disabled to read the result, one simple that throws ApiException. I tried to mantain the same code sintax. Then, to test all the basic features, I rewritten for my company the test class, adding more tests on the not async part etc, ... |
EndGlobalSection | ||
GlobalSection(ExtensibilityGlobals) = postSolution | ||
SolutionGuid = {6AC7DED5-0F52-4187-9BC1-74437AAEE42D} | ||
EndGlobalSection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm....why there are changes to this file? Did you open this solution with VS 16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS 15 is Visual Studio 2017
VS 16 is Visual Studio 2019
I opened it with the latest one patch of 2019.
But the commit after i rolledback it running the scripts.
I'd say remove the GetAwaiter.GetResult and just make the methods async. The reason they weren't made async originally is because they were still being used by the RestSharp code. As this now lives inside its own library folder we can just go ahead and turn the deserializer into async methods which should solve the issue with your aggregate exceptions |
Appveyor CI failed with the following error messages (partial):
Ref: https://ci.appveyor.com/project/WilliamCheng/openapi-generator/builds/39001191 |
It seems a transient error during the integration test, the previous test was fine and nothing changes of iimpacting, the message also is clear. I will test it locally to ensure it is right to propagate an HttpRequestException or convert it to an ApiException, comparing to the RestApi version. On wich server they run? The public official one or during tests the global configuration changes to a temporary server? |
I need help I probably don't understand the complete cycle of CI. master branch
this PR
I see in the history that both appveyor configuration and test project are not changed recently so probably I'm missing something or the public server used for the integration tests changed forcing the https in those days. |
The public API server won't work with our tests. You will need to setup run your petstore server locally:
add add the following in the host table:
|
Sorry I read now the guide: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests |
Thank you I found the two issues I put inside the test project and rolledback. |
Can you please undo changes to the following file?
|
Done |
👍 I'll merge it tomorrow before the v5.1.1 release if no one has further feedback on this PR. |
Actually there is the class test PetApiTestsV2, maybe i can remove it and open a future PR to refactor the tests if useful. |
Sorry I don't understand. Why remove it? These are new tests that you added as part of this PR. |
I don't know, maybe are not so similar to others or too much, and the class name is terrible. i don't want to add noise to the project. they were useful to me to certify the new client for our product. |
We can always improve the tests (naming, code style, etc) later with another PR. |
Comparing the behavior with Restsharp I found an issue.
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
,5.1.x
,6.0.x
@Blackclaws