-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
removed argument requestId from loopbackserver.cs #58678
Conversation
Thanks for the contribution! |
i will remove the argument requestId in the abstract methods in GenericLoopbackServer.cs |
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.
All inherited classes need similar changes.
src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs
Outdated
Show resolved
Hide resolved
…er.cs Co-authored-by: Karel Zikmund <[email protected]>
ok @karelz . i will make similar changes in all inherited classes and then test locally and will make a commit soon |
@AnudeepGunukula just curious if you were able to make further progress, or if you got stuck somewhere ... |
Hi @karelz i am working on it and will make commit as soon as possible |
hi @karelz . i had removed requestId argument from all the Inherited classes |
Hi @ManickaP |
Hi @AnudeepGunukula, I see in the checks that there are failing tests due to the change:
They need to be resolved as well. You can run the tests for System.Net.Http locally like this: https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/testing.md#running-tests-for-a-single-library From the quick look, it seems like the change in HttpClientHandlerTest.Http2.cs is not correct. Since the test is not reading request via In this particular case, you need to somehow pass the stream id to the loopback connection. |
Hi @AnudeepGunukula, |
Hi @ManickaP |
Hi @ManickaP . I had created new function in http2 but still some checks are failing. |
@AnudeepGunukula did you look at all the failed legs? From a cursory look at couple of failed legs are failing on test
Did you try to reproduce the failure on your machine and debug it? |
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
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.
please try to revert the line where _lastStreamId
was passed before
…tion.cs Co-authored-by: Katya Sokolova <[email protected]>
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.
Looks like we are almost done with the change. I added 2 nits with extra space and 1 deduplication of code.
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
…tion.cs Co-authored-by: Karel Zikmund <[email protected]>
…tion.cs Co-authored-by: Karel Zikmund <[email protected]>
…tion.cs Co-authored-by: Karel Zikmund <[email protected]>
Hi , i had commited the suggested changes . |
@AnudeepGunukula I think you missed my comment to remove 'async' from the method: #58678 (comment) @greenEkatherine can you please help here to move this further? Thank you! |
@AnudeepGunukula thank you for your contribution! Kindly let me take over this PR and add the necessary change |
Thanks @AnudeepGunukula for your contribution! |
Closes #58239
i had removed the requestId arguement from the loopbackserver.cs
i removed at the places where argument is not necessary.
This pr will solve the above issue.