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

aiomoto #766

Closed
wants to merge 2 commits into from
Closed

aiomoto #766

wants to merge 2 commits into from

Conversation

dazza-codes
Copy link
Contributor

@dazza-codes dazza-codes commented Feb 19, 2020

Description of Change

Fix #583
Fix #665
Fix #753
Fix #755
Fix getmoto/moto#2706

This most likely replaces #759 and that PR should not be merged.

Notes

  • will not work in py3.5 and that's intentional
  • no idea yet about how to integrate this into the rest of aiobotocore tests, so this PR simply uses nested test directories and namespaced fixtures to try to avoid conflicts, but they might need to be resolved (either in this PR or something following this).
  • the MotoService does not provide a clean backend across tests, by default, so the server fixtures in this PR add and use a MotoService.reset option to clear the backends

@dazza-codes
Copy link
Contributor Author

dazza-codes commented Feb 20, 2020

Pushed up a revision to add a MotoService.reset option to clear moto.backend.BACKENDS for the service required. It seems to work, in the context of other non-aio-moto clients that need to create fixtures, like an s3-bucket.


$ pytest -v tests/aws/
============ test session starts ===================
platform linux -- Python 3.6.7, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- /opt/conda/envs/aiobotocore/bin/python
cachedir: .pytest_cache
rootdir: /home/dlweber/src/aiobotocore
plugins: cov-2.8.1, asyncio-0.10.0, forked-1.1.3, xdist-1.31.0
collected 20 items                                                                                                                                                                  

tests/aws/test_aws_fixtures.py::test_aws_credentials PASSED                      [  5%]
tests/aws/test_aws_fixtures.py::test_aws_batch_client PASSED                     [ 10%]
tests/aws/test_aws_fixtures.py::test_aws_ec2_client PASSED                       [ 15%]
tests/aws/test_aws_fixtures.py::test_aws_ecs_client PASSED                       [ 20%]
tests/aws/test_aws_fixtures.py::test_aws_iam_client PASSED                       [ 25%]
tests/aws/test_aws_fixtures.py::test_aws_logs_client PASSED                      [ 30%]
tests/aws/test_aws_fixtures.py::test_aws_s3_client PASSED                        [ 35%]
tests/aws/test_aws_s3.py::test_aws_bucket_access PASSED                          [ 40%]
tests/aws/aio/test_aio_aws_s3.py::test_aio_aws_bucket_access                     [ 45%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_session_credentials PASSED   [ 50%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_batch_client PASSED          [ 55%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_ec2_client PASSED            [ 60%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_ecs_client PASSED            [ 65%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_iam_client PASSED            [ 70%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_logs_client PASSED           [ 75%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_s3_client PASSED             [ 80%]
tests/aws/aio/test_aiomoto_clients.py::test_aio_aws_client PASSED                [ 85%]
tests/aws/aio/test_aiomoto_service.py::test_moto_service PASSED                  [ 90%]
tests/aws/aio/test_aiomoto_service.py::test_moto_batch_service PASSED            [ 95%]
tests/aws/aio/test_aiomoto_service.py::test_moto_s3_service PASSED               [100%]

=========== 20 passed in 10.49s ====

Notes

  • added fixtures and tests that create and access an s3-bucket
  • the moto.backends.BACKENDS["s3"]["global"] model might need closer attention to detail about when and how a model instance is created and retained, but that's probably out of scope here

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #766 into master will decrease coverage by 34.11%.
The diff coverage is 41.94%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #766       +/-   ##
===========================================
- Coverage   90.83%   56.71%   -34.12%     
===========================================
  Files          12       16        +4     
  Lines         611      878      +267     
===========================================
- Hits          555      498       -57     
- Misses         56      380      +324     
Impacted Files Coverage Δ
aiobotocore/aiomoto/aiomoto_services.py 27.83% <27.83%> (ø)
aiobotocore/aiomoto/aiomoto_fixtures.py 42.85% <42.85%> (ø)
aiobotocore/aiomoto/utils.py 46.66% <46.66%> (ø)
aiobotocore/aiomoto/aws_fixtures.py 63.15% <63.15%> (ø)
aiobotocore/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
aiobotocore/_endpoint_helpers.py 39.13% <0.00%> (-60.87%) ⬇️
aiobotocore/parsers.py 6.25% <0.00%> (-46.88%) ⬇️
aiobotocore/eventstream.py 64.70% <0.00%> (-35.30%) ⬇️
aiobotocore/response.py 66.00% <0.00%> (-34.00%) ⬇️
aiobotocore/args.py 67.74% <0.00%> (-29.04%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 904332b...178a481. Read the comment docs.

@dazza-codes
Copy link
Contributor Author

I can't reproduce the travis test failures locally, it seems to be some problem with the virtualenv not getting all the project packages installed or found for the tests. Unfortunately, it works for me.

@thehesiod
Copy link
Collaborator

wow awesome, will take a look if someone doesn't get to this before me

setup.py Outdated Show resolved Hide resolved
tests/aws/utils.py Outdated Show resolved Hide resolved
@thehesiod
Copy link
Collaborator

btw the idea for the tests, in regard to buckets is that each test gets a unique bucket, see https://github.com/aio-libs/aiobotocore/blob/master/tests/conftest.py#L30

@thehesiod
Copy link
Collaborator

thehesiod commented Feb 20, 2020

It's been awhile but I think unfortunately moto shares global state across its services. Which is a good but also bad thing. Good because it allows cross service linkages (ex SNS messages triggering lambdas), bad because it means you can't reliably run multiple instances of the same service in the same process. Really it should have a better way of doing this but c'est la vie. So I think we need to re-structure the moto service fixture to ensure that each function scope gets a service helper process, and then each service for the function scope runs in said helper process. This will require a bit of infra around the MotoService class. This will ensure separation of services across tests and allow for cross service linkages for each test.

@thehesiod
Copy link
Collaborator

Actually ignore above rec for separate process per test. pytest-xdist already separates each test into a separate process

@terricain
Copy link
Collaborator

This looks great 😄 Might have to loot some of it for aioboto3 once its merged 😉

@lgtm-com
Copy link

lgtm-com bot commented Feb 20, 2020

This pull request introduces 1 alert when merging da1c4d6 into fcf7cd4 - view on LGTM.com

new alerts:

  • 1 for Binding a socket to all network interfaces

@dazza-codes
Copy link
Contributor Author

Need to consider options to parametrize the server and client fixtures to address the code-cov problem.

@dazza-codes dazza-codes changed the title [WIP] aiomoto aiomoto Feb 23, 2020
@lgtm-com
Copy link

lgtm-com bot commented Feb 23, 2020

This pull request introduces 2 alerts when merging 1c24619 into 904332b - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a call
  • 1 for Binding a socket to all network interfaces

@dazza-codes
Copy link
Contributor Author

dazza-codes commented Feb 23, 2020

The PR was rebased on master and seems to pass unit tests OK.

  • make mototest works
  • make coverage does not pass everything (add -m moto?)
  • test coverage doesn't seem to capture everything
    • maybe test coverage can't handle asyncio tracing or it can't test code that is called in pytest fixtures
    • e.g. aiobotocore/aiomoto/aiomoto_services.py should be fully tested already by tests/aiomoto/test_aiomoto_service.py

It's in a state that might be "good enough to go", although there might be additional feature requests that subsequent PRs can address. I have enough code to continue with the "real" work that needs some of this and my focus needs to shift to that. Hope this helps a bit. I did not add any documentation about this, because it could be premature to advertise it.

@chesstrian
Copy link

Hi @dazza-codes, do you know if there are any plans to merge this?, I'm able to reproduce the issue AttributeError: 'AWSResponse' object has no attribute 'raw_headers' testing some DynamoDB operations.

Thanks in advance.

@dazza-codes
Copy link
Contributor Author

My focus on this has moved to https://gitlab.com/dazza-codes/aio-aws and I've lost track of where this PR is at in the context of this project. I doubt it's in the right form exactly to be merged for this project. I don't know if I have time and/or enough feedback to get it into the right shape for this repo. I understand there are some other branches with various refactoring that might conflict with this, so it's probably not worth the time right now to resolve anything until there's a new release. Maybe the project maintainers can just cherrypick patches from this into the main line. I know it took a lot of time and reading to understand the problem and put together this test suite, so let's see what happens as the project refactoring evolves. At least this PR is visible as one possible solution to the test problems.

@chesstrian
Copy link

Thanks @dazza-codes, I forked and merged your PR and it didn't work for DynamoDB, need to change the approach to test.


@pytest.fixture
def aws_port():
return os.getenv("AWS_PORT", AWS_PORT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use a random port or else you can't run tests in parallel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a new commit to try to use a free port, although I'm not sure the patch will be used in moto.

@thehesiod
Copy link
Collaborator

sorry for taking so long to review this, been super busy

@dazza-codes
Copy link
Contributor Author

I haven't been able to keep up with this and I'm a bit blocked on it since the move to Pipfile (I've been using poetry and avoiding anything pipenv); I don't recall the details of when/why I switched from pipenv to poetry, but I vaguely recall a bunch of issues that totally wrecked my python setups. So far, despite a few minor hiccups with poetry, it's been a good move.

@thehesiod
Copy link
Collaborator

nice, we were on pipenv, then got frustrated due to slowness/inability to resolve certain scenarios so moved to poetry, then found another blocking bug on it so switched back :). I initially tried the new pipenv release but it was completely busted so waiting a bit more to try the latest pipenv again

@rajeshn96
Copy link

hello, @dazza-codes I too am reproducing this error 'AWSResponse' object has no attribute 'raw_headers' when testing with moto. I tried using the patched code solution suggested in https://github.com/aio-libs/aiobotocore/issues/755#issuecomment-586117397 but it doesn't seem to affect the issue. Maybe I missed it in this thread, but is there a better way to deal with this error?

@aio-libs aio-libs deleted a comment from CLAassistant Nov 21, 2020
Copy link

This pull request has been marked as stale because it has been inactive for more than 60 days. Please update this pull request or it will be automatically closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 20, 2024
@thehesiod
Copy link
Collaborator

btw new version of moto does not require an endpoint per service, all services are serviced by the same endpoint so will not be needed. I started working on that here: master...thehesiod/aio-checksum-streaming-moto-bump bunch of tests to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants