8000 test(docker): mocked unittests external calls by slettner · Pull Request #1958 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

test(docker): mocked unittests external calls #1958

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 4, 2021
Merged

test(docker): mocked unittests external calls #1958

merged 8 commits into from
May 4, 2021

Conversation

slettner
Copy link
Contributor

Patched or moved unit tests in the docker module to avoid external calls.
Some tests are now in the integration/hub_usage/test_hub_usage.py module.

Fixes #1640

@slettner slettner requested a review from a team as a code owner February 16, 2021 21:54
@jina-bot jina-bot added size/S area/testing This issue/PR affects testing labels Feb 16, 2021
@codecov
Copy link
codecov bot commented Feb 16, 2021 •
edited
Loading

Codecov Report

Merging #1958 (4113aab) into master (89f5843) will decrease coverage by 0.31%.
The diff coverage is n/a.

❗ Current head 4113aab differs from pull request most recent head 354f27c. Consider uploading reports for the commit 354f27c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1958      +/-   ##
==========================================
- Coverage   90.30%   89.98%   -0.32%     
==========================================
  Files         230      230              
  Lines       12135    12135              
==========================================
- Hits        10958    10920      -38     
- Misses       1177     1215      +38
Flag Coverage Δ
daemon 50.38% <ø> (ø)
jina 90.10% <ø> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/docker/hubapi/remote.py 88.05% <0.00%> (-5.98%) ⬇️
jina/docker/hubapi/local.py 83.51% <0.00%> (-5.50%) ⬇️
jina/docker/hubio.py 77.85% <0.00%> (-5.48%) ⬇️
jina/parsers/hub/__init__.py 95.00% <0.00%> (-2.50%) ⬇️
jina/peapods/runtimes/zmq/zed.py 90.06% <0.00%> (-1.33%) ⬇️
jina/peapods/pods/__init__.py 91.36% <0.00%> (-1.08%) ⬇️

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 4c4571b...354f27c. Read the comment docs.

JoanFM
JoanFM reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor
@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Really nice work! Thank you for the contribution!!

@JoanFM
Copy link
Contributor
JoanFM commented Feb 16, 2021

recheckcla

@github-actions
Copy link
github-actions bot commented Feb 16, 2021 •
edited
Loading

Latency summary

Current PR yields:

  • 🐎🐎🐎🐎 index QPS at 1066, delta to last 3 avg.: +11%
  • 🐎🐎 query QPS at 16, delta to last 3 avg.: +10%

Breakdown

Version Index QPS Query QPS
current 1066 16
1.2.1 937 14
1.2.0 973 14

Backed by latency-tracking. Further commits will update this comment.

@slettner
Copy link
Contributor Author

Thanks :)
I will have a look at the failing tests in the evening 👍🏼

@nan-wang
Copy link
Member

Jina CLA check

❤️ Thank you for your pull request. It looks like this is your first contribution to an open source project maintained by Jina AI Limited. Before we can look at your pull request, we kindly ask that you sign our Contributor License Agreement. You can sign it by commenting in format below.

I have read the CLA Document and I hereby sign the CLA

@slettner
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@slettner
Copy link
Contributor Author

I have seen that the CI times out after 15 minutes, I think this causes two modules to fail (drivers and flow).

For the docker module, apparently, the right images are not available leading to the IndexError.

    def docker_image():
        def _filter_repo_tag(image, image_name='jinahub/pod.dummy_mwu_encoder'):
            tags = image.attrs['RepoTags'] if 'RepoTags' in image.attrs else None
            if tags:
                return tags[0].startswith(image_name)
            else:
                return False
    
        client = docker.from_env()
        images = client.images.list()
>       image_name = list(filter(lambda image: _filter_repo_tag(image), images))[0]
E       IndexError: list index out of range

Are these images built by other tests? Might the Error come from the execution order of tests?

@JoanFM
Copy link
Contributor
JoanFM commented Feb 25, 2021

I have seen that the CI times out after 15 minutes, I think this causes two modules to fail (drivers and flow).

For the docker module, apparently, the right images are not available leading to the IndexError.

    def docker_image():
        def _filter_repo_tag(image, image_name='jinahub/pod.dummy_mwu_encoder'):
            tags = image.attrs['RepoTags'] if 'RepoTags' in image.attrs else None
            if tags:
                return tags[0].startswith(image_name)
            else:
                return False
    
        client = docker.from_env()
        images = client.images.list()
>       image_name = list(filter(lambda image: _filter_repo_tag(image), images))[0]
E       IndexError: list index out of range

Are these images built by other tests? Might the Error come from the execution order of tests?

I think u may be able to find in the CI how the images are built before

@florian-hoenicke
Copy link
Member

@slettner Can you please resolve the merge conflicts

@slettner slettner force-pushed the test-docker-1640 branch 3 times, most recently from 9f99238 to 5b12412 Compare March 10, 2021 10:50
@slettner
Copy link
Contributor Author

Hi @florian-hoenicke,

I updated the PR after a rebase, sorry for the inactivity.

The tests look better now but the test_hub_build_level fails now with an IndexError while fetching the local docker images.
After debugging, I found that the images in my environment are named jinahub/pod.encoder.dummy_mwu_encoder instead of the current jinahub/pod.dummy_mwu_encoder causing the index error.

Unfortunately, this change only fixes the first test of the module, the second one produces no fail-levels at all, where
expected_failed_levels = [BuildTestLevel.POD_DOCKER, BuildTestLevel.FLOW] is expected.

Do you have an idea what might be the cause?

JoanFM
JoanFM suggested changes Mar 10, 2021
View reviewed changes
@florian-hoenicke
Copy link
Member

@slettner would you like to continue working on this issue? Or should someone from us take over?

@slettner
Copy link
Contributor Author
slettner commented May 3, 2021

I can pick it up today, sorry for the inactivity :)

@slettner slettner force-pushed the test-docker-1640 branch from 5f8d715 to 57355ad Compare May 3, 2021 10:47
@jina-bot jina-bot added size/M and removed size/S labels May 3, 2021
@slettner slettner force-pushed the test-docker-1640 branch 3 times, most recently from 4036397 to 19cd8a1 Compare May 3, 2021 10:59
JoanFM
JoanFM suggested changes May 3, 2021
View reviewed changes
@slettner slettner force-pushed the test-docker-1640 branch from 7a20dd5 to 7301267 Compare May 4, 2021 07:50
@slettner slettner force-pushed the test-docker-1640 branch from 4113aab to 354f27c Compare May 4, 2021 09:48
florian-hoenicke
florian-hoenicke approved these changes May 4, 2021
View reviewed changes
Copy link
Member
@florian-hoenicke florian-hoenicke left a comment

Choose a reason for hiding this comment

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

Good work!

@slettner slettner merged commit 0f70d99 into jina-ai:master May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-hoenicke florian-hoenicke florian-hoenicke approved these changes

@JoanFM JoanFM JoanFM approved these changes

@cristianmtr cristianmtr Awaiting requested review from cristianmtr

Assignees
No one assigned
Labels
area/testing This issue/PR affects testing size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

move docker tests doing external calls to integration and use mocking for unit tests
5 participants
@slettner @JoanFM @nan-wang @florian-hoenicke @jina-bot
0