Skip to content
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

Fix async client without http auth #297

Conversation

liran-cohen-hs
Copy link

Description

Fix for async connection that is used without http auth

Issues Resolved

Closes #283

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: liran <liran@hiredscore.com>
Signed-off-by: liran <liran@hiredscore.com>
nhtruong
nhtruong previously approved these changes Feb 22, 2023
CHANGELOG.md Show resolved Hide resolved
Signed-off-by: liran <liran@hiredscore.com>
saimedhi
saimedhi previously approved these changes Feb 23, 2023
@dblock
Copy link
Member

dblock commented Feb 28, 2023

Makes sense. This needs a test, please?

@liran-cohen-hs
Copy link
Author

liran-cohen-hs commented Mar 1, 2023

Makes sense. This needs a test, please?

I tried to see where I can add a specific test
it seems that RequestsHttpConnection and AIOHttpConnection classes has a lot of tests which is great, but the bug was on AsyncHttpConnection which inherits AIOHttpConnection but has no tests
so I am not sure how to approach this (rather than adding a lot of tests for AsyncHttpConnection which not sure I have the time to invest in :))

@dblock
Copy link
Member

dblock commented Mar 1, 2023

Makes sense. This needs a test, please?

I tried to see where I can add a specific test it seems that RequestsHttpConnection and AIOHttpConnection classes has a lot of tests which is great, but the bug was on AsyncHttpConnection which inherits AIOHttpConnection but has no tests so I am not sure how to approach this (rather than adding a lot of tests for AsyncHttpConnection which not sure I have the time to invest in :))

Add just enough tests for AsyncHttpConnection that would fail for the bug you fix and write TODO: for everything else?

Signed-off-by: liran <liran@hiredscore.com>
@liran-cohen-hs
Copy link
Author

Makes sense. This needs a test, please?

I tried to see where I can add a specific test it seems that RequestsHttpConnection and AIOHttpConnection classes has a lot of tests which is great, but the bug was on AsyncHttpConnection which inherits AIOHttpConnection but has no tests so I am not sure how to approach this (rather than adding a lot of tests for AsyncHttpConnection which not sure I have the time to invest in :))

Add just enough tests for AsyncHttpConnection that would fail for the bug you fix and write TODO: for everything else?

added another commit with the test
I focused on the E2E rather on the mock tests, since the mock can potentially skip the fixed code

@dblock
Copy link
Member

dblock commented Mar 6, 2023

Makes sense. This needs a test, please?

I tried to see where I can add a specific test it seems that RequestsHttpConnection and AIOHttpConnection classes has a lot of tests which is great, but the bug was on AsyncHttpConnection which inherits AIOHttpConnection but has no tests so I am not sure how to approach this (rather than adding a lot of tests for AsyncHttpConnection which not sure I have the time to invest in :))

Add just enough tests for AsyncHttpConnection that would fail for the bug you fix and write TODO: for everything else?

added another commit with the test I focused on the E2E rather on the mock tests, since the mock can potentially skip the fixed code

I commented out the code change and none of the tests failed - I was running ./.ci/run-tests true 2.3.0. Also is there an easier way to run these?

@saimedhi
Copy link
Collaborator

saimedhi commented Jun 2, 2023

Hello @liran-cohen-hs, httpbin has been replaced with http server in the following PR. Please use http server instead of httpbin for testing. Thanks :)

@dblock
Copy link
Member

dblock commented Jun 6, 2023

Right on! @liran-cohen-hs this should be a lot easier to finish now, hope you have time to pick it up! Happy to help.

@saimedhi
Copy link
Collaborator

@liran-cohen-hs, Kindly complete the pull request. Thanks for raising the initial PR :)

@dblock
Copy link
Member

dblock commented Jul 25, 2023

@dannosaur - was this fixed by #424?

@dannosaur
Copy link
Contributor

@dblock yup.

@dblock dblock closed this Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] initializing an async client without http_auth results in an error
5 participants