NOTISSUES: Add kinesis tests, refactor doc and refactor code style #654
Conversation
Codecov Report
@@ Coverage Diff @@
## master #654 +/- ##
==========================================
+ Coverage 93.89% 93.93% +0.04%
==========================================
Files 9 9
Lines 557 561 +4
==========================================
+ Hits 523 527 +4
Misses 34 34
Continue to review full report at Codecov.
|
|
You cannot remove test patches, do you understand its purpose? If it fails it means a human needs to validate the upstream changes. There may be new feature support needed. |
|
@thehesiod, Yes, I understand. But now this test only hinders, for example #653 the result is that all the functionality is working, according to the tests. But due to the fact that the hash has not converged, we get fail https://travis-ci.com/aio-libs/aiobotocore/jobs/164023523 I do not understand why this is, if the functionality works. Even if the user receives an error, after the update, due to the fact that we have not covered all the tests, can use roll back the version and create issues. |
|
that is not how releases work, we must try to guarantee that with each release we've done everything possible to ensure that the release works and behaves like the botocore equivalent. If aiobotocore were to accept turning off the patches I would start working from a fork. The patches ensure that if any changes are made to the botocore super classes a human will verify if any changes are necessary in aiobotocore. Without it we'll start to have failures creep in over time. Long term we have other strategies to remove this requirement. Please see the other open issues |
|
Who should make the decision on updating super classes? |
|
The super classes are in botocore, we need to mirror their changes in aiobotocore. I think we need to expand the readme to explain this better. I'll try to write something up |
|
@thehesiod but it is not. there is still a main class aiobotocore and aiohttp. What classes should be in this test? I will return, but I think this is an extra measure to check. |
|
@vir-mir Like I said, this is not a standard library, you cannot write unittests for functionality you do not know exists. A new botocore can add new functionality for which we have not added tests, and the patches test will catch this. Sorry I haven't had a chance to write this doc yet. Hopefully soon. |
|
@thehesiod you look the changes, I returned the test |
|
sorry for delay, finally updated the doc: https://github.com/aio-libs/aiobotocore/blob/master/CONTRIBUTING.rst @vir-mir let me know what you think |
|
@vir-mir cool, I'll try to review this, I hope the new docs make sense, if you still think its wrong lets discuss here before this gets approved. |
|
|
||
| 3. Make sure all tests passed | ||
|
|
||
| 4. Make sure the coverage doesn't get worse. |
There was a problem hiding this comment.
this is enforced via the automated tests, do we need to state this? Also for 5, you can't just commit it, you need to open a PR and have it reviewed before you can commit.
There was a problem hiding this comment.
I wanted to say here. You can check the coverage before use pull request
There was a problem hiding this comment.
there is no reference for what the current coverage levels are, I think this is only checkable via the CI no?
|
|
||
| .. code-block:: shell | ||
|
|
||
| $ cd aiobotocore |
There was a problem hiding this comment.
I think a better strategy going forward is using tox, so you can test against multiple python versions easily. Thoughts?
There was a problem hiding this comment.
Start to "tox" is good for "CI", for development it is desirable to check on the new version. Here I agree with Andrey in his report https://www.youtube.com/watch?v=7KgihdKTWY4
There was a problem hiding this comment.
sorry, I don't understand what you're trying to say? I don't understand russian either so that video doesn't help. Is that the aiohttp author by chance, asvetlov?
| rm -rf build | ||
| rm -rf cover | ||
| rm -rf dist | ||
| @rm -rf `find . -name __pycache__` |
There was a problem hiding this comment.
can just do all this via a git clean no?
There was a problem hiding this comment.
I do not know removes git clone python pyc..., did not find something in google
There was a problem hiding this comment.
try git clean -fx, it should remove the need for this big list
|
first thank you for your submission! I know it's a lot of work putting something like this together and that it's taking awhile to get movement on this :) ok I've personally mostly reviewed it, could you update your description as things have changed and it has incomplete sentences (this test prevents him ...?). Also given the scope of this I think @jettify should review as well. Perhaps splitting it up into multiple PRs and this touches a lot of different areas. Lets get some consensus as I'm also just a contributor. |
|
@thehesiod @jettify Thanks for the criticism, I answered all the questions. I will make another change. Since I actively use this library in my work, I would like to help in its development, I hope we will work together. |
@thehesiod Thx! I would only add motivation before the description. in order to immerse in context before reading. Hashes of Botocore Code (important)We do not support all Botocore unit tests and so we decided to do the following. Because of the way aiobotocore is implemented (see Background section), it is very tightly coupled with botocore |
|
re last cmt, feel free to update contributing in this PR |
|
@thehesiod Hi. I made changes according to the discussion.
|
|
cool! I'll try to get to this review asap, if someone else didn't get to it before me |
|
|
||
| 2. Make a change | ||
|
|
||
| 3. Make sure all tests passed |
There was a problem hiding this comment.
| 3. Make sure all tests passed | |
| 3. Make sure all tests pass |
| * Name commit | ||
|
|
||
| If you set the date and version, it will be the last and will be released | ||
| Version must be raised in your last committee |
There was a problem hiding this comment.
FYI, I've created templates for logging new issues and PRs. Could you also in here update the template so the two match each other?
| @@ -151,8 +149,8 @@ def paginate(self, **kwargs): | |||
| documented_paginator_cls = type( | |||
| paginator_class_name, (Paginator,), {'paginate': paginate}) | |||
|
|
|||
| operation_model = self._service_model.\ | |||
| operation_model(actual_operation_name) | |||
| operation_model = self._service_model.operation_model( | |||
There was a problem hiding this comment.
does this change match the botocore impl?
| response_dict, operation_model.output_shape) | ||
| response_dict, | ||
| operation_model.output_shape | ||
| ) |
There was a problem hiding this comment.
do the above changes match the botocore impl?
btw, adding to steps should be, making sure overriden code matches formatting from botocore impl as well as file names
| self._previous_next_token == self._next_token: | ||
|
|
||
| is_not_none = self._previous_next_token is not None | ||
| if is_not_none and self._previous_next_token == self._next_token: |
There was a problem hiding this comment.
does these changes match botocore formatting?
| from botocore.waiter import Waiter, WaiterError, logger, xform_name | ||
|
|
||
| # WaiterModel is required for client.py import | ||
| WaiterModel = import_module('botocore.waiter').WaiterModel |
| async with client('kinesis', loop, **kwargs) as kinesis: | ||
| shards = await get_shards(kinesis, stream_name, save_shards) | ||
| while True: | ||
| await asyncio.sleep(0.2) |
There was a problem hiding this comment.
is there not a waiter you can use instead of sleep?
| @@ -0,0 +1,3 @@ | |||
| aiohttp==3.3.2 | |||
There was a problem hiding this comment.
this should be open ended, it was before: aiohttp>=3.3.1
| @@ -128,7 +129,7 @@ | |||
|
|
|||
| @pytest.mark.xfail(raises=NotImplementedError) | |||
| @pytest.mark.asyncio | |||
| async def test_result_key_iters(s3_client, bucket_name,): | |||
| async def test_result_key_iters(s3_client, bucket_name, ): | |||
There was a problem hiding this comment.
i think the extra comma at the end was a typo, mind removing?
|
ok added a few comments that need to be resolved, otherwise looks good |
|
This hasn't been touched for over a year, is there anything of value we want to incorperate from this pr? |
|
|
||
| import botocore |
There was a problem hiding this comment.
I think I understand what you're doing here, separating built-in modules from third party modules, I've been adding a comment before each section to make this clear in my code. Could you please do the same?
| @@ -199,11 +200,24 @@ def sns_server(): | |||
| @pytest.yield_fixture(scope="session") | |||
| def sqs_server(): | |||
| host = "localhost" | |||
| port = 5004 | |||
| port = get_free_tcp_port() | |||
There was a problem hiding this comment.
if you merge from master I've fixed this
|
@terrycain ya there are new tests, normalizing of include ordering, more docs, etc. But there are still a lot of questionable changes, outdated changes, and other misc problems. Either @vir-mir should split out these changes into separate PRs or we start picking out ideas from this PR and giving credit. |
|
Alexey Firsov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
a lot has changed :) can you please updated, thanks! |
test_patches, dependency management throughpyup-bot, this test prevents himmakefile, for testing locally or via AWScommitbranchCHANGES.txtisortpyup-bot@jettify review please