-
Notifications
You must be signed in to change notification settings - Fork 176
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
#384 Fixes async basic auth (http_async.py) #385
Conversation
Signed-off-by: Ruslan Gainanov <gromrx1@gmail.com>
Thanks! Needs a test and a CHANGELOG entry, please? |
@dblock ok, thanks for review. I'll add this to changelog. But I don't know how designed tests and CI in the project. It's very hard to me fix them. Also I found these tests - opensearch-py/test_opensearchpy/test_async/test_connection.py Lines 184 to 191 in 707373e
It seems like project already have tests for http_auth. |
Signed-off-by: Ruslan Gainanov <gromrx1@gmail.com>
You would need to dig into this and write a test that would fail without this change. Reading the description of your fix it seems that async basic auth doesn't work. So write a test that shows in what way it doesn't work, make sure it fails without the fix. Then add the fix and make sure the test passes. Without a test, there's no guarantee we won't accidentally re-introduce a regression and break your fix. The test you linked is testing |
I don't know how to run tests. I was trying clone a repo and run them with By the way I write a script that approve the error.
Running
|
Hello @GRomR1, please go through the developer guide to know more about testing. |
@saimedhi thank for your advice. I've already pass through this guide. If I run the command
But it doesn't help me to understand next things:
I'm sorry if I ask a stupid questions :) |
Hello @GRomR1, I will take a look at warnings. I have one more suggestion, please run the same tests on main branch and then on your branch. If warnings are also present in main branch then it isn't an issue related to your PR. And also I notice that tests are skipped for all python versions other than 3.9 due to missing python interpreters. Please make sure you test for all python versions. Thanks. |
Ok. When I checkout to the
When I run
|
Ideally, we need to install all python versions. But I suggest to test for 2.7 and some other python version greater than 3.5.
Please use
Please go through all the tests related to http_auth. Let us know if you find that your change is covered or tested in any of the current tests. Write brief tests that are similar to the ones already in place if you see that your modifications have not been tested. |
Didn't notice anything missing. |
Description
Fix async basic auth
Issues Resolved
Closes #384